Closed Bug 1452970 Opened 4 years ago Closed 4 years ago

Add "auto-hide" option to Download toolbar icon context menu

Categories

(Firefox :: Toolbars and Customization, enhancement, P3)

60 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: sevaan, Assigned: Oriol)

References

Details

Attachments

(2 files)

Users can set the Download toolbar icon to auto-hide when in customize mode. This option should be surfaced in the Download icon's context menu as well.
Blocks: 1397447
Priority: -- → P3
See Also: → 1399873
Yes, the current behavior is so annoying and discovering how to disable it is so difficult. In bug 1399873 comment 5 I already said a context menu entry was needed. I don't know much about toolbar buttons, but I might take a look at this when I have time.
Comment on attachment 8967939 [details]
Bug 1452970 - Add auto-hide option to Download toolbar icon context menu

https://reviewboard.mozilla.org/r/236628/#review242408

This only adds the context menu while the button is in the toolbar. I'm not convinced that's right - can you clarify with UX what they want?

::: browser/base/content/browser.xul:336
(Diff revision 1)
>                       oncommand="SidebarUI.hide()"/>
>      </panel>
>  
>      <menupopup id="toolbar-context-menu"
> -               onpopupshowing="onViewToolbarsPopupShowing(event, document.getElementById('viewToolbarsMenuSeparator'));">
> +               onpopupshowing="onViewToolbarsPopupShowing(event, document.getElementById('viewToolbarsMenuSeparator')); UpdateDownloadsAutoHide(this)">
> +      <menuitem oncommand="gCustomizeMode.onDownloadsAutoHideChange(event)"

Can't this just set the pref without touching customize mode? We don't need to move the button anywhere when this context menu item is used, because it must already be in the toolbar, and we might not be in customize mode.

::: browser/base/content/browser.xul:338
(Diff revision 1)
>  
>      <menupopup id="toolbar-context-menu"
> -               onpopupshowing="onViewToolbarsPopupShowing(event, document.getElementById('viewToolbarsMenuSeparator'));">
> +               onpopupshowing="onViewToolbarsPopupShowing(event, document.getElementById('viewToolbarsMenuSeparator')); UpdateDownloadsAutoHide(this)">
> +      <menuitem oncommand="gCustomizeMode.onDownloadsAutoHideChange(event)"
> +                type="checkbox"
> +                label="&customizeMode.autoHideDownloadsButton.label;"

This needs its own label and an access key (that doesn't clash with everything else).

::: browser/components/customizableui/CustomizeMode.jsm:2451
(Diff revision 1)
> -    let checkbox = event.target.ownerDocument.getElementById(kDownloadAutohideCheckboxId);
> -    Services.prefs.setBoolPref(kDownloadAutoHidePref, checkbox.checked);
> +    let checked = event.target.getAttribute("checked");
> +    Services.prefs.setBoolPref(kDownloadAutoHidePref, checked);

Should force this to be a bool - getAttribute returns null or a string (could be empty string)
Attachment #8967939 - Flags: review?(gijskruitbosch+bugs)
Sevaan, can you elaborate on how you think this needs to work, keeping in mind the previous discussion in bug 1399873 ?
Flags: needinfo?(sfranks)
(In reply to :Gijs (he/him) from comment #4)
> This only adds the context menu while the button is in the toolbar. I'm not
> convinced that's right - can you clarify with UX what they want?

I think the button is only autohidden when it's in the toolbar, it doesn't happen e.g. when it's in the overflow menu.
Is Sevaan from the UX team or who should I ask?
(In reply to Oriol Brufau [:Oriol] from comment #6)
> (In reply to :Gijs (he/him) from comment #4)
> > This only adds the context menu while the button is in the toolbar. I'm not
> > convinced that's right - can you clarify with UX what they want?
> 
> I think the button is only autohidden when it's in the toolbar, it doesn't
> happen e.g. when it's in the overflow menu.

Right. It does on other toolbars and in the customize mode palette, though. Also, in the other toolbars, if you used that entry in customize mode it would move the button to the main toolbar. Your version doesn't do that. It's not clear what the desired behaviour is, but I would expect that it should do the same thing as ticking it in customize mode.

> Is Sevaan from the UX team or who should I ask?

Yes, Sevaan works in UX.
Comment on attachment 8967939 [details]
Bug 1452970 - Add auto-hide option to Download toolbar icon context menu

https://reviewboard.mozilla.org/r/236628/#review242700

Going to clear review for now. It's not clear to me what the desired behavior here is, let's get some feedback from Sevaan (please don't re-request review before that's happened).

FWIW, it would be a good idea to add some tests for this. There's already a test that tests the existing behavior, I think - it should be possible to build on that.
Attachment #8967939 - Flags: review?(gijskruitbosch+bugs)
Test is at browser/components/downloads/test/browser/browser_downloads_autohide.js

It may be possible to integrate with that test, or you could create a new one that borrows heavily.
(In reply to :Gijs (he/him) from comment #9)
> Right. It does on other toolbars and in the customize mode palette, though.

About other toolbars, my context menu entry appears in the tabbar, menu bar and bookmark bar. So not sure what's the problem.

About the customize mode palette, yeah, let's see what Sevaan says.

> Also, in the other toolbars, if you used that entry in customize mode it
> would move the button to the main toolbar.

Not sure if we have a different notion of "other toolbars", but I can't repro this. Only for the customize mode palette.

> Your version doesn't do that.

Yeah, because you said "Can't this just set the pref without touching customize mode? We don't need to move the button anywhere when this context menu item is used".

> FWIW, it would be a good idea to add some tests for this.

OK
(In reply to Oriol Brufau [:Oriol] from comment #12)
> (In reply to :Gijs (he/him) from comment #9)
> > Right. It does on other toolbars and in the customize mode palette, though.
> 
> About other toolbars, my context menu entry appears in the tabbar, menu bar
> and bookmark bar. So not sure what's the problem.
>
> About the customize mode palette, yeah, let's see what Sevaan says.

The menuitem doesn't appear when using the context menu if the item is in the customize mode palette, right (different menu)? But the little "[x] auto-hide" popup does currently appear there. Also, I'm not sure whether the fact that the little popup doesn't appear in the menu means we also shouldn't have an entry in the context menu, if that's the way we're going. So yeah, hence asking for UX feedback. I don't think there's enough detail in how this bug got filed for it to be fixed right now.

> > Also, in the other toolbars, if you used that entry in customize mode it
> > would move the button to the main toolbar.
> 
> Not sure if we have a different notion of "other toolbars", but I can't
> repro this. Only for the customize mode palette.

Hm, yeah, that bool is only used if the item was in the palette, sorry for the confusion.

> > Your version doesn't do that.
> 
> Yeah, because you said "Can't this just set the pref without touching
> customize mode? We don't need to move the button anywhere when this context
> menu item is used".

In the toolbar context menu this is true. It wouldn't be for the other ones, though this whole feature is pretty confusing so I don't think it's super clear what the expected behavior(s) is/are.

I can't say I'm super enthusiastic about adding this context menu item. My understanding was always that the auto-hide behavior was a stepping stone to removing the button entirely (and providing different, non-button UI for downloads). Spending more energy on making the current thing work differently doesn't seem a good use of time to me, but perhaps UX has changed their long-term plans - I don't know.
Hi all,

I didn't know I was coming in to so much history here, thanks for all the details. It was just something I thought was missing and wanted to make a note of. I spoke with some of the other designers working on tangential issues and it turns out that Bryan Bell and Stephen Horlander are working on a fairly comprehensive spec that should address this and the issues mentioned in Bug 1399873. I would imagine new work would be tracked in a new bug and would reference these bugs.
Flags: needinfo?(sfranks)
Just wanted to clarify my original thinking for when I posted this. This bug is not about the rest of customize mode, but just about the Download icon in the browser toolbar.

If a user has placed the Download icon in the toolbar and autohide is OFF in customize mode, then the context menu when right-clicking on the icon should read:


> Pin to Overflow menu
> Turn Auto-hide On
> Remove from Toolbar
> --
> Bookmarks Toolbar
> --
> Customize

If a user selects it, then treat the button as if auto-hide is on (meaning if a download is in progress it stays in the bar, and if there is no download in progress it can be hidden).

If auto-hiding has previously been turned ON in customize mode, and the user has downloaded a file returning the Download icon to the toolbar, then when they right-click on it the context menu should read:

> Pin to Overflow menu
> Turn Auto-hide Off
> Remove from Toolbar
> --
> Bookmarks Toolbar
> --
> Customize

If the user hits the auto-hide option, auto-hide is turned off and the icon remains in the toolbar always. The context menu changes the next time the user right-clicks on it to say Turn Auto-hide Off.
Oriol, would it be possible for you to update your patch based on comment 15? I think the only thing that it needs compared to the current state is 2 different labels (instead of checkbox state), and ideally a test. But it wouldn't need extending to the other context menus, so it's most of the way there? :-)
Flags: needinfo?(oriol-bugzilla)
OK, I will do it
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Flags: needinfo?(oriol-bugzilla)
This is the patch, but I think it's bad UX. In order to know whether the context menu item enables or disables auto-hide, I must read all the text until the end. I think a checkbox would be better, or at least say "Enable Auto-hide"/"Disable Auto-hide", then just reading the first word you already know what will happen.
(In reply to Oriol Brufau [:Oriol] from comment #19)
> or at least say "Enable
> Auto-hide"/"Disable Auto-hide", then just reading the first word you already
> know what will happen.

I like Enable Auto-hide and Disable. Let's do that.
Meridel, could you weigh in on the best wording here?
Flags: needinfo?(mwalkington)
> I like Enable Auto-hide and Disable. Let's do that.
Done!


Bill his name, your complaint about "Pin to overflow menu" doesn't seem related to this auto-hide functionality, so probably you should file a new bug.
Bill, we're not reviewing all the other items in the context menu here (which in any case aren't unique to the downloads button - the context menu is the same for all the other items), nor what they do. File separate bugs if you think their behavior or labeling needs changes.

Also, please be respectful of other people doing work here - comments along the lines of the UX being "dishonest", deliberately obscure, or suggesting no thought at all ("shaken in a sack" etc.) went into what is there already aren't OK.

My understanding is the UX team is working on a more comprehensive overhaul of customize mode, but until that concept is finished (and then implemented) this will be a good step to make the current behavior easier to control for users. This bug isn't a good place for philosophising about an entirely new way to do the whole thing.
Comment on attachment 8967939 [details]
Bug 1452970 - Add auto-hide option to Download toolbar icon context menu

https://reviewboard.mozilla.org/r/236628/#review244446

This looks OK to me. Besides the 1 nit below, please wait with landing for feedback from Meridel (and make any changes to the string as requested). :-)

::: browser/base/content/browser.js:6302
(Diff revision 5)
>      menuItem.setAttribute("checked", isSelected);
>    }
>  }
>  
> +function UpdateDownloadsAutoHide(popup) {
> +  let autoHide = this.window.DownloadsButton.autoHideDownloadsButton;

`this` is a bit confusing here, because this function isn't a method nor does it get called as one (so I think it'll just equal `window`). Anyway, browser.js runs in the window scope, so I think you can just omit `this.window.` and reference `DownloadsButton.autoHideDownloadsButton` directly.
Attachment #8967939 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8967939 [details]
Bug 1452970 - Add auto-hide option to Download toolbar icon context menu

https://reviewboard.mozilla.org/r/236628/#review244446

> `this` is a bit confusing here, because this function isn't a method nor does it get called as one (so I think it'll just equal `window`). Anyway, browser.js runs in the window scope, so I think you can just omit `this.window.` and reference `DownloadsButton.autoHideDownloadsButton` directly.

True, I think I copied this from somewhere else but then forgot to remove `this.window`.
Bill - 

Bugzilla is our professional working environment, as much as it is our issue tracker, and consequently we put a very high premium on bug reports and followup participation that is both focused and concise as well as respectful and professional.

These comments have been marked off-topic in this bug because they are off-topic to the resolution of this bug, and those decisions will stand. While we're grateful for the time and effort our contributors put in to trying to make Firefox better, no amount of effort from any single contributor outweighs the importance of civility, professionalism and focus among Mozillians as developers and participants in the greater Mozilla community. Digressions that stray into rehashing design decisions and bug reports about other issues, much less patronizing distractions like "Such is pedantry", do not meet the bar we've set for ourselves, do nothing to move these bugs closer to a resolution and, in aggregate, suck up a lot of engineering time and motivation.

You're welcome to take this discussion up with me directly - my email address is mhoye@mozilla.com - but encourage you to consult the Etiquette and Contributor Guidelines linked here:

https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

... and to keep this feedback in mind if you intend to continue participating in this bug.

Thanks you.
Hey all,

I met with Meridel today to talk about this.

Here is the final take!: Checkmarks are back in. We use it when the Bookmarks Toolbar is displayed, so we can use it here.

> Pin to Overflow Menu
> Auto-Hide in Toolbar
> Remove from Toolbar
> --
> Bookmarks Toolbar
> --
> Customize

If Auto-Hide is on, the checkmark is on next to it. If Auto-Hide is off, no checkmark.
Flags: needinfo?(mwalkington)
I have the patch, but the problem is that doCommand does not toggle the checkbox beforehand, and synthesizeMouseAtCenter doesn't seem to work at all. So I don't know how to properly simulate a click on the checkbox, and the test fails.
(In reply to Oriol Brufau [:Oriol] from comment #33)
> I have the patch, but the problem is that doCommand does not toggle the
> checkbox beforehand, and synthesizeMouseAtCenter doesn't seem to work at
> all. So I don't know how to properly simulate a click on the checkbox, and
> the test fails.

Happy to try to help, can you put up the patch and test WIP so I can see what the issue is? It's a bit hard to tell from the description...
Flags: needinfo?(oriol-bugzilla)
Comment on attachment 8967939 [details]
Bug 1452970 - Add auto-hide option to Download toolbar icon context menu

https://reviewboard.mozilla.org/r/236626/#review246968

::: browser/components/downloads/test/browser/browser_downloads_autohide.js:312
(Diff revision 7)
> +  await openContextMenu(button);
> +  is(checkbox.hidden, false, "Auto-hide checkbox is visible");
> +  is(checkbox.getAttribute("checked"), "true", "Auto-hide is enabled");
> +
> +  info("Disable auto-hide via context menu");
> +  checkbox.doCommand();

Unlike a real click, this does not toggle the checkbox. So the checkbox is still checked when `oncommand` runs, and then the auto-hide pref does not change.
Flags: needinfo?(oriol-bugzilla)
Flags: needinfo?(gijskruitbosch+bugs)
Toggling the checkbox manually is so ugly, but it works.
(In reply to Oriol Brufau [:Oriol] from comment #38)
> Toggling the checkbox manually is so ugly, but it works.

Indeed. I think this is fine for now. Triggered autoland on this. Sorry I was a bit slow...
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b12b76bbe1c6
Add auto-hide option to Download toolbar icon context menu r=Gijs
Backed out changeset b12b76bbe1c6 (bug 1452970) for TV failures in browser/components/downloads/test/browser/browser_downloads_autohide.js on a CLOSED TREE

Problematic push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b12b76bbe1c655bdf18b4456add8aa7207d8ac08&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=177648256
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=177648256&repo=autoland&lineNumber=1554
Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bd9355f65718f3eb1dc3d1e913c2943fe3dd8294&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

05:07:58     INFO - TEST-UNEXPECTED-FAIL | browser/components/downloads/test/browser/browser_downloads_autohide.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/downloads/test/browser/browser_downloads_autohide.js:337 - TypeError: contextMenu.showPopup is not a function
Flags: needinfo?(oriol-bugzilla)
Sorry, I was convinced I ran the test after adding the last check. The method is called openPopup, not showPopup.
Flags: needinfo?(oriol-bugzilla)
Try seems green now
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5436da349c53
Add auto-hide option to Download toolbar icon context menu r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5436da349c53
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.