885086 - Overflow panel looks kinda wild when wide items get in there

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Overflow panel looks kinda wild when wide items get in there

    http://i.imgur.com/ymMnjoO.png

    and the opposite: http://cl.ly/image/162j1H3j1i3j

    and it doesn't go away (yesterday's nightly) http://cl.ly/image/2i0H2U2y000V

Comment 1

[Mike de Boer [:mikedeboer]]

Jared, is there a UX spec that shows how the overflow panel should look like when wide items get in there (I suspect not, but asking you to be sure).

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

No spec, just has to look pretty :)

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: trigger overflow calc after building toolbar

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: remember and restore toolbar item position before and after overflow, respectively

Not sure if I need to remember the beforeNode too here.

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Part 3: correct styling of toolbar items in overflow panel

I tossed some CSS around here... some of the things I'm not sure about. Like, where should we put ALL the overflow panel styles? Throw it back into OS-specific browser.js? Move it all to shared/menupanel.inc.css?

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 797867 [details] [diff] [review]
Patch 1: trigger overflow calc after building toolbar

Review of attachment 797867 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +340,5 @@
>      this.endBatchUpdate();
> +
> +    // When a toolbar is built, it may be overflowing. Check and correct here.
> +    if (aToolbar.overflowable)
> +      document.defaultView.setTimeout(() => aToolbar.overflowable._onOverflow(), 100);

Where did 100ms come from?

Put another way: Why not Task.spawn?

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Blair McBride [:Unfocused] from comment #6)
> Where did 100ms come from?
> 
> Put another way: Why not Task.spawn?

The 100ms timeout is indeed arbitrary, something I wanted to discuss here. The problem that needs solving is that toolbar overflow needs to be checked after a toolbar is built for a window that just opened. I tried using a setTimeout(..., 0), but that only worked on OSX; the other OSes needed a longer wait to gather their wits and detect an overflowed toolbar. I currently lack the knowledge of possible events to listen for that might offer a more robust solution.

What would Task.spawn offer?

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> The 100ms timeout is indeed arbitrary, something I wanted to discuss here.
> The problem that needs solving is that toolbar overflow needs to be checked
> after a toolbar is built for a window that just opened. I tried using a
> setTimeout(..., 0), but that only worked on OSX; the other OSes needed a
> longer wait to gather their wits and detect an overflowed toolbar. I
> currently lack the knowledge of possible events to listen for that might
> offer a more robust solution.
> 
> What would Task.spawn offer?

Nothing, in this case, if setTimeout(..., 0) didn't work :)

Looking at it again, this looks more like a bug in OverflowableToolbar. If you're having to wait, OverflowableToolbar *should* have already handled it - as soon as it's constructed (a few lines up from your change), it adds a listener for the "overflow" event, and queues any update needed. Maybe OverflowableToolbar.init() isn't being run/is throwing?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 797868 [details] [diff] [review]
Patch 2: remember and restore toolbar item position before and after overflow, respectively

Review of attachment 797868 [details] [diff] [review]:
-----------------------------------------------------------------

Why is this patch needed? How does this solve the problem of wide items looking weird when in the overflow panel? How does the current method of only using appendChild not work?

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +2321,5 @@
>  
>      while(child && this._target.clientWidth < this._target.scrollWidth) {
>        let prevChild = child.previousSibling;
>  
> +      if (!(child.getAttribute("nooverflow") == "true")) {

This style is very confusing. Please change this to:
if (child.getAttribute("nooverflow") != "true") {

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 797871 [details] [diff] [review]
Part 3: correct styling of toolbar items in overflow panel

Review of attachment 797871 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> I tossed some CSS around here... some of the things I'm not sure about.
> Like, where should we put ALL the overflow panel styles? Throw it back into
> OS-specific browser.js? Move it all to shared/menupanel.inc.css?

I'm not sure here, but a cross-platform shared solution seems the most desirable.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +398,5 @@
> +  background-position: 0 center;
> +}
> +
> +#widget-overflow-list > toolbarbutton,
> +#widget-overflow-list > toolbarbutton > toolbarbutton {

How common is it to have a toolbarbutton inside of another toolbarbutton?

@@ +399,5 @@
> +}
> +
> +#widget-overflow-list > toolbarbutton,
> +#widget-overflow-list > toolbarbutton > toolbarbutton {
> +  -moz-box-align: center;

I think we should default the alignment to the start position instead of centering.

@@ +410,5 @@
> +
> +#widget-overflow-list > toolbarbutton > .toolbarbutton-text,
> +#widget-overflow-list > toolbarbutton > toolbarbutton > .toolbarbutton-text {
> +  text-align: start;
> +  -moz-padding-start: 5px;

Other places use a padding-start of .5em, but this one uses 5px.

@@ +415,5 @@
> +}
> +
> +#widget-overflow-list > #edit-controls,
> +#widget-overflow-list > #zoom-controls {
> +  min-height: 28px;

Why the lesser min-height for the edit and zoom controls (28px) when compared to other controls (36px)?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> The problem that needs solving is that toolbar overflow needs to be checked
> after a toolbar is built for a window that just opened.

Why is the current toolbar overflow detection unreliable/need adjusting?

Comment 12

[:Gijs (he/him)]

The current state is messy enough that I think this is probably P1.

Comment 13

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] from comment #10)
> I think we should default the alignment to the start position instead of
> centering.

Well, since the default is 'start' already, I changed it to 'center', because it didn't look great other wise.

> Why the lesser min-height for the edit and zoom controls (28px) when
> compared to other controls (36px)?

Because it looks much better that way... I don't have another reason than that.

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Zoom Controls at 28px

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Zoom Controls at 36px

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Part 3.1: correct styling of toolbar items in overflow panel

Comment 18

[Mike de Boer [:mikedeboer]]

Gijs, why would you remove something I intentionally put there? In patch 3.1 I touch findbar styles that are needed to make it work properly in the overflow panel. The menu panel is also a panel. I'd like to work through this bug first to get styles in a consistent state and then tackle the broken state of the search bar in the menu panel.

Comment 19

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #18)
> Gijs, why would you remove something I intentionally put there? In patch 3.1
> I touch findbar styles that are needed to make it work properly in the
> overflow panel. The menu panel is also a panel. I'd like to work through
> this bug first to get styles in a consistent state and then tackle the
> broken state of the search bar in the menu panel.

I gave my rationale in bug 893849. I don't think this is an actual dependency, given my understanding of what bug 893849 is about. I also looked in patch 3.1, but I don't see any searchbar (I presume that's what you mean, not findbar?) styles, except urlbar-search-splitter, which will now (since bug 913780) no longer be in the overflow panel (so there's no need to style it for that case).

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 803607 [details] [diff] [review]
Part 3.1: correct styling of toolbar items in overflow panel

Review of attachment 803607 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +408,5 @@
> +  background-position: 0 center;
> +}
> +
> +#widget-overflow-list > toolbarbutton,
> +#widget-overflow-list > toolbarbutton > toolbarbutton {

From my previous review, when do we have a toolbarbutton inside of a toolbarbutton? Did you intend to have "#widget-overflow-list > toolbaritem > toolbarbutton" ?

Comment 21

[Mike de Boer [:mikedeboer]]

Gijs, thanks for explaining that! I didn't read your comment in bug 893849 yet, because bugmail. If I had, I wouldn't have commented ;)

Comment 22

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] from comment #20)
> From my previous review, when do we have a toolbarbutton inside of a
> toolbarbutton? Did you intend to have "#widget-overflow-list > toolbaritem >
> toolbarbutton" ?

I *believe* I saw this with the Downloads and Bookmarks (star) button. I will investigate some more and report facts here.

Comment 23

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #22)
> (In reply to Jared Wein [:jaws] from comment #20)
> > From my previous review, when do we have a toolbarbutton inside of a
> > toolbarbutton? Did you intend to have "#widget-overflow-list > toolbaritem >
> > toolbarbutton" ?
> 
> I *believe* I saw this with the Downloads and Bookmarks (star) button. I
> will investigate some more and report facts here.

The Downloads button shouldn't nest toolbarbuttons. The Bookmarks button's inner toolbarbutton has a specific class -- you should use it.

Comment 24

[Mike de Boer [:mikedeboer]]

Attachment: Part 3.2: correct styling of toolbar items in overflow panel

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 805899 [details] [diff] [review]
Part 3.2: correct styling of toolbar items in overflow panel

Review of attachment 805899 [details] [diff] [review]:
-----------------------------------------------------------------

Please address the following issue before landing. Otherwise this looks good to me.

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +53,4 @@
>    -moz-margin-end: 0;
>  }
> +#edit-controls@inAnyPanel@ > toolbarbutton:not(:first-child):not(:last-child),
> +#zoom-controls@inAnyPanel@ > toolbarbutton:not(:first-child):not(:last-child) {

When is this rule necessary? If we have a :not(:first-child) rule that sets margin-start:0 and a :not(:last-child) rule that sets margin-end:0, doesn't that cancel out the need for this rule? Applying the union of those two rules would equal this rule (margin-left:0; margin-right:0), right?

Comment 26

[Mike de Boer [:mikedeboer]]

Landed with proposal in comment 25 applied.

https://hg.mozilla.org/projects/ux/rev/36faf02c5fb4

Comment 27

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/36faf02c5fb4