Closed Bug 1657790 Opened 4 years ago Closed 4 years ago

Local one-offs are hidden if all engine one-offs are hidden

Categories

(Firefox :: Address Bar, defect, P2)

defect
Points:
3

Tracking

()

RESOLVED FIXED
85 Branch
Iteration:
85.1 - Nov 16 - Nov 29
Tracking Status
firefox85 --- fixed

People

(Reporter: adw, Assigned: adw)

References

Details

Attachments

(1 file)

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.

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.

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?

Flags: needinfo?(mverdi)
Assignee: adw → nobody
Status: ASSIGNED → NEW
Iteration: 81.1 - July 27 - Aug 09 → ---
Keywords: blocked-ux

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.

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.

Flags: needinfo?(mverdi)
Keywords: blocked-ux
Priority: P2 → P3
Keywords: blocked-ux

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.

Keywords: blocked-ux
Priority: P3 → P2

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...

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.

Keywords: blocked-ux

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.

Keywords: blocked-ux

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: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 84.1 - Oct 19 - Nov 01
Points: 2 → 3
Iteration: 84.1 - Oct 19 - Nov 01 → 84.2 - Nov 2 - Nov 15
  • 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
Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a7a369259f74 Allow local search mode buttons to be toggled separately from engines. r=mak,ntim
Blocks: 1678770
Iteration: 84.2 - Nov 2 - Nov 15 → 85.1 - Nov 16 - Nov 29
Regressions: 1678773
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch
Regressions: 1681818

: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?

Flags: needinfo?(adw)

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?

Flags: needinfo?(adw)

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.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: