Closed Bug 885086 Opened 7 years ago Closed 6 years ago

Overflow panel looks kinda wild when wide items get in there

Categories

(Firefox :: Toolbars and Customization, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jaws, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M9][Australis:P1])

Attachments

(5 files, 2 obsolete files)

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
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P2]
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).
Flags: needinfo?(jaws)
No spec, just has to look pretty :)
Flags: needinfo?(jaws)
Assignee: nobody → mdeboer
Blocks: 879974
Not sure if I need to remember the beforeNode too here.
Attachment #797868 - Flags: review?(jaws)
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?
Attachment #797871 - Flags: feedback?(jaws)
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?
(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?
Flags: needinfo?(bmcbride)
Status: NEW → ASSIGNED
(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?
Flags: needinfo?(bmcbride)
Attachment #797867 - Flags: review?(jaws)
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 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)?
Attachment #797871 - Flags: feedback?(jaws) → feedback+
(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?
The current state is messy enough that I think this is probably P1.
Whiteboard: [Australis:M?][Australis:P2] → [Australis:M?][Australis:P1]
(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.
Attachment #797868 - Attachment is obsolete: true
Attached image Zoom Controls at 28px
Attached image Zoom Controls at 36px
Duplicate of this bug: 885077
Attachment #797871 - Attachment is obsolete: true
Attachment #803607 - Flags: review?(jaws)
Blocks: 893849
No longer blocks: 893849
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.
Flags: needinfo?(gijskruitbosch+bugs)
(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).
Flags: needinfo?(gijskruitbosch+bugs)
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" ?
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 ;)
(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.
(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.
Blocks: 916953
Blocks: 916964
Whiteboard: [Australis:M?][Australis:P1] → [Australis:M9][Australis:P1]
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?
Attachment #805899 - Flags: review?(jaws) → review+
Landed with proposal in comment 25 applied.

https://hg.mozilla.org/projects/ux/rev/36faf02c5fb4
Whiteboard: [Australis:M9][Australis:P1] → [Australis:M9][Australis:P1][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/36faf02c5fb4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P1][fixed-in-ux] → [Australis:M9][Australis:P1]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.