1389713 - Added webextension's toolbar button at the right-end goes into overflow menu in compact mode

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[atlanto]

STR:
1. Install any webextension having toolbar-button such as uBlock Origin or Zoom Page WE (I tried with it) or ...
2. Customize Density to Compact

AR:
The right-end button is captured into overflow menu.

ER:
All buttons are visible.

mozregression:
 2:21.77 INFO: Last good revision: 5322c03f4c8587fe526172d3f87160031faa6d75
 2:21.77 INFO: First bad revision: d0068f9051be88eb4c97e28ec0a4f101ca4ff147
 2:21.77 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5322c03f4c8587fe526172d3f87160031faa6d75&tochange=d0068f9051be88eb4c97e28ec0a4f101ca4ff147

I believe this was triggered by bug 1388457. There must be insufficiency.

Comment 1

[Steven S.]

Somebody posted a workaround for this to MozillaZine, http://forums.mozillazine.org/viewtopic.php?p=14760820&sid=f11031d66046ab313483a1558621fb5b#p14760820

"To have the uBlock/uMatrix icons displayed with the number of blocked requests, requires an inner-padding of 6 (compact or not). Less than 6 and the icons will be pushed to the overflow menu (compact or not)."

This issue also happens with Bitwarden. It will stay in the toolbar if there are no available passwords, but disappears to the Overflow Menu immediately after navigating to a site that has an available password, which adds a numbered flag to the icon.

Comment 2

[:Gijs (he/him)]

This feels like there's an overflow event that's generated on the button rather than on the toolbar that then triggers the button being moved, but the code here: https://dxr.mozilla.org/mozilla-central/rev/5322c03f4c8587fe526172d3f87160031faa6d75/browser/components/customizableui/CustomizableUI.jsm#4265-4272 is already meant to deal with that type of issue.

A minimal testcase here (or, even better, checking if my suspicion is near the mark and/or how we would update that code so it doesn't get confused by the layers of XBL bindings...) would be helpful. It seems unlikely that we will want to update inner padding on these buttons, but even though I don't think this really is, at its heart, a theme bug, I'll ping Dão to see if I'm missing something.

Comment 3

[timbugzilla]

This happens for me as well. Seems to be when a number of objects blocked notification is on the ublock origin button.

Comment 4

[Dão Gottwald [::dao]]

(In reply to timbugzilla from comment #3)
> This happens for me as well. Seems to be when a number of objects blocked
> notification is on the ublock origin button.

So I guess this negative margin makes the toolbar overflow:
http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/toolkit/themes/linux/global/toolbarbutton.css#121

Comment 5

[Ronan Jouchet]

Workaround: manually inserting a toolbar item (e.g. "Bookmarks")
between the addon icons you want in the toolbar, and the overflow menu,
prevents icons being shoved to the overflow menu.

Put differently:

<URL Bar> <uBlock Origin, which I want in the toolbar> **<Bookmarks icon>** <Overflow icon>

... will leave uBlock Origin in the toolbar, but

<URL Bar> <uBlock Origin, which I want in the toolbar> <Overflow icon>

... will result in uBlock being shoved in the Overflow icons. 

Note: coming here from https://www.reddit.com/r/firefox/comments/6vqm8l/addons_stuck_in_overflow_menu/

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Gijs: There is an overflow event generated on the button, but that gets correctly ignored.  I added some console logging, and this is what it shows:

  CustomizableUI:Overflow!!! overflow { target: <box.toolbarbutton-animatable-box>, isTrusted: true, view: ChromeWindow → browser.xul, detail: 2, layerX: 0, layerY: 0, pageX: 0, pageY: 0, which: 0, rangeOffset: 0, SCROLL_PAGE_UP: -32768 }  CustomizableUI.jsm:4296

  CustomizableUI:Overflow!!! overflow { target: <hbox#nav-bar-customization-target.customization-target>, isTrusted: true, view: ChromeWindow → browser.xul, detail: 1, layerX: 0, layerY: 0, pageX: 0, pageY: 0, which: 0, rangeOffset: 0, SCROLL_PAGE_UP: -32768 }  CustomizableUI.jsm:4296

  CustomizableUI:Still Overflow!!!

(The first event, for the button, doesn't get past the return statement.  The second one, for the nav-bar, does.)

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Attachment: Bug 1389713 - Push the badge a little closer to the button in compact mode to avoid overflow. , ui-r=abenson

Review commit: https://reviewboard.mozilla.org/r/175058/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175058/

Comment 8

[Dão Gottwald [::dao]]

Comment on attachment 8903266 [details]
Bug 1389713 - Push the badge a little closer to the button in compact mode to avoid overflow. , ui-r=abenson

https://reviewboard.mozilla.org/r/175058/#review180344

::: browser/themes/shared/toolbarbuttons.inc.css:96
(Diff revision 1)
>  
> +/* Add an extra pixel of padding on the end to contain the badge and
> + * prevent it from overflowing the toolbar and being pushed into the
> + * overflow menu. */
> +toolbar .toolbarbutton-1.badged-button {
> +  padding-inline-end: 3px;

Instead of this, would it make sense to adjust .toolbarbutton-badge's margin-inline-end in compact mode?

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Sure.  Are we happy with the 1px extra overlap in compact mode?

Compact: https://cl.ly/1M0F0L273T2w
Normal: https://cl.ly/1p410Q1d3l1T

Comment 10

[Aaron Benson]

This looks good to me!

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8903266 [details]
Bug 1389713 - Push the badge a little closer to the button in compact mode to avoid overflow. , ui-r=abenson

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175058/diff/1-2/

Comment 12

[Dão Gottwald [::dao]]

Comment on attachment 8903266 [details]
Bug 1389713 - Push the badge a little closer to the button in compact mode to avoid overflow. , ui-r=abenson

https://reviewboard.mozilla.org/r/175058/#review180506

::: browser/themes/shared/toolbarbuttons.inc.css:120
(Diff revision 2)
>  
>  toolbar .toolbarbutton-1 > .toolbarbutton-icon {
>    /* horizontal padding + actual icon width */
>    max-width: calc(2 * var(--toolbarbutton-inner-padding) + 16px);
>  }
>  

Can you please move the new rule to this spot?

::: browser/themes/shared/toolbarbuttons.inc.css:187
(Diff revision 2)
>    color: inherit;
>  }
>  
> +/* Remove a pixel of margin on the end so that the badge doesn't
> + * overflow the toolbar and push the button into the overflow menu. */
> + :root[uidensity=compact] .toolbarbutton-badge {

nit: remove space before :root

Thanks!

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8903266 [details]
Bug 1389713 - Push the badge a little closer to the button in compact mode to avoid overflow. , ui-r=abenson

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175058/diff/2-3/

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8903266 [details]
Bug 1389713 - Push the badge a little closer to the button in compact mode to avoid overflow. , ui-r=abenson

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175058/diff/3-4/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e625bc399af9
Push the badge a little closer to the button in compact mode to avoid overflow. r=dao, ui-r=abenson

Comment 16

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e625bc399af9

Comment 18

[Vlad Bacia-Mociran [:VladB]]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20170926220106

This issue has been verified on latest Firefox Nightly Build ID: 20170926220106 on Windows 8.1 x64, Mac OS 10.12 and Ubuntu 14.04 and I cannot reproduce it. Now, webextensions that are added in the browser toolbar at the right-end and they don't go into overflow menu when switching to compact mode.