Local one-offs are hidden if all engine one-offs are hidden
Categories
(Firefox :: Address Bar, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox85 | --- | fixed |
People
(Reporter: adw, Assigned: adw)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
If you uncheck all the engines in the one-click search engines section in about:preferences#search, the local one-offs are also hidden. They should be shown. The problem is here: https://searchfox.org/mozilla-central/rev/26b13464c2beb26e0d864d561c30e817a85c348a/browser/components/search/SearchOneOffs.jsm#455
We need to allow the urlbar subclass to override this behavior somehow.
Comment 1•4 years ago
|
||
Are we sure they should be shown? The last time we discussed with Verdi I think we agreed upon keeping the current behavior for now, and collect feedback. Hiding one-offs would become a lot harder if we'd fix this bug, and a group of users would want an option to do so.
Assignee | ||
Comment 2•4 years ago
|
||
It doesn't make sense to me that hiding engines would also cause local one-offs to be hidden... they search different things. Ideally there would be a separate way to hide the local one-offs.
Verdi, should local one-offs be hidden when all engine one-offs are hidden?
Assignee | ||
Updated•4 years ago
|
Comment 3•4 years ago
|
||
I agree with the fact they search different things, though how would a user hide these buttons at that point? I don't think they'd want to remove all the engines AND disable bookmarks/history/tabs for the urlbar.
Provided I also don't agree that we hide these buttons if those options are disabled: I may not want to see open tabs by default among my results, but still be able to search open tabs on-demand, similarly to the fact I can disable search suggestions but still get them when I restrict to search.
I think it's all things to clarify with Verdi during Tuesday's meetings, if it's not at good time for your timezone (as I suspect) ask Harry to bring up the discussion, or drop him an e-mail. Needinfos are more complicate to handle for UX.
We could also say that the only way to hide one-offs is through userchrome, but then we should ensure keyboard doesn't go through them if they are collapsed.
Assignee | ||
Comment 4•4 years ago
|
||
I got clarification from Verdi, and I'll file a new bug related to the preferences UI. Ultimately we want users to be able to toggle the local one-offs separately from the engines, so we'll need to fix this bug at some point.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 5•4 years ago
|
||
I spoke with Verdi and we should list the local one-offs as engines in about:preferences they can be toggled them with the usual checkbox. This is a temporary solution until we redesign about:preferences#search. I'm also going to bump this to a P2 now that it blocks the follow-ups bug.
Assignee | ||
Comment 6•4 years ago
|
||
This isn't so simple due to the fact that the local one-offs aren't actually engines. The visibility state and ordering of the engines in about:preferences and in the one-offs is kept in the search service and reflected in Services.search.getVisibleEngines
. We'll have to keep some separate info on the side -- in addition to the visible engine list in the search service -- in order to track not only which local one-offs are hidden, but also their ordering relative to each other and to the engines, since the user can drag engines around in about:preferences to change their ordering, unless we made it impossible to drag the local one-offs. But that would be weird because why can you reorder the engines but not the locals?
It makes me think we should have implemented the local one-offs as actual engines, even if that means we need to make a new local type of engine...
Assignee | ||
Comment 7•4 years ago
|
||
I guess we need clarification from Verdi here about what the interim about:preferences UI looks like. Are the locals listed in the same table as the engines? If so, the concerns I mentioned apply. If instead they're listed as a separate group of checkboxes that can't be dragged around, then this will be a lot simpler.
Assignee | ||
Updated•4 years ago
|
Comment 8•4 years ago
|
||
It was discussed today, long term the whole UI should be rethought, but in the meanwhile Verdi think we should just add these buttons to the current list and don't allow to drag them or sort engines between them. Nor remove them.
Basically they should appear at the end of the list and only allow to check/uncheck them.
Assignee | ||
Comment 9•4 years ago
|
||
It's going to be weird that they're shown in the same table but they can't be dragged or removed like the others, but OK.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 10•4 years ago
|
||
- Add prefs for each local search mode
- Remove the
update2.localOneOffs
pref since it's not necessary with the three
new prefs (this makes the patch look worse than it is) - Modify preferences UI by adding a new row in the engines tree for each local
search mode - Add
UrlbarUtils.LOCAL_SEARCH_MODES
so we have a single place where local
search modes and their properties are defined - Add a new test file for the preferences UI
- Modify browser_oneOffs.js to test the three new preferences
Assignee | ||
Comment 11•4 years ago
|
||
Assignee | ||
Comment 13•4 years ago
|
||
Assignee | ||
Comment 14•4 years ago
|
||
Comment 15•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Comment 16•4 years ago
|
||
bugherder |
Comment 17•4 years ago
|
||
:adw - I just noticed that in this PR you pulled a ~600 lines of browser.ftl
into preferences.xhtml and only use several messages from it. I filed bug 1684283 to consider adding debug instrumentation to help us catch cases like this, but in the meantime - was it a conscious choice?
Did you consider extracting those messages to a separate file used in browser and preferences?
Assignee | ||
Comment 18•4 years ago
|
||
I'm not sure what you mean. Are you saying I should copy and paste the messages from browser.ftl into preferences.ftl? Or move them into a new ftl file and then import that file everywhere those messages are currently used?
Comment 19•4 years ago
|
||
I'm not sure what you mean. Are you saying I should copy and paste the messages from browser.ftl into preferences.ftl? Or move them into a new ftl file and then import that file everywhere those messages are currently used?
I'd consider the latter for strings that are shared between those two large UIs.
Description
•