Adding many items to overflow panel in customization mode breaks customize mode layout, makes it impossible to exit customize mode with the 'done' button

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
2 months ago
20 days ago

People

(Reporter: arai, Assigned: Gijs)

Tracking

(Blocks: 2 bugs, {regression})

unspecified
Firefox 56
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 months ago
Created attachment 8878807 [details]
whole window screenshot after step 4

Steps to reproduce:
  1. open Nightly 56.0a1 (2017-06-17) (64-bit) on OSX
  2. click hamburger button
  3. click "Customize..."
  4. add all items to overflow panel

Actual result:
  "Done" button gets scrolled away

Expected result:
  there should be scrollbar on the overflow panel and the bottom area (where "Done", "Restore Defaults" etc are) should always be there
(Assignee)

Comment 1

2 months ago
Ouch, this is pretty bad. :-\

Just in case people try this: If you get in this state and need to get out, you can still hit [esc] on your keyboard or switch to a different tab - but it might be better to just remove things from the overflow menu again. :-|
Blocks: 1352693
Keywords: regression
Summary: Adding many items to overflow panel in customization mode makes it impossible to exit → Adding many items to overflow panel in customization mode breaks customize mode layout, makes it impossible to exit customize mode with the 'done' button
Whiteboard: [photon-structure][triage]

Updated

2 months ago
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-structure][triage] → [photon-structure]
(Assignee)

Comment 2

2 months ago
Created attachment 8880613 [details] [diff] [review]
WIP patch

Dan, could you help me out with something here? I'm trying to use flex box to have the 'panel' item in customize mode (the bit encased in blue graph paper style background on the right) that basically only takes up the space it needs, but equally doesn't end up making the window bigger / putting the footer off-screen.

I'm struggling quite a bit - some of the flexbox stuff doesn't seem to want to do what it should in XUL. This WIP patch has 2 issues:

- the panel grows beyond the size of the items it contains. This is a consequence of the height: 0 and flex-grow: 1, AIUI, but without the height: 0 it never gets smaller if the window gets smaller, and without the flex-grow it always stays minimum height (but with padding, which is a bit odd).
- when this happens, the items end up at the bottom. I tried using align-items or justify-content to fix this, but that didn't seem to help. The only thing that seems to help is the position:relative styling that gets applied on hover (for unrelated reasons, we can probably get rid of it with photon, it had to do with how we position items in the old grid-style menu), but that feels like a weird hack.

Any idea what I should be doing differently?


artifact trypush with patch (should have builds in the next half hour or so):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=991df33fa25edd30e1523e339e9b07806b8b90f3
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #8880613 - Flags: feedback?(dholbert)
(Assignee)

Updated

2 months ago
Iteration: --- → 56.1 - Jun 26
Flags: qe-verify? → qe-verify+
Priority: P2 → P1
(Assignee)

Comment 3

2 months ago
(There are also other issues with the faked arrow popup styles being messed up, but mixing normal XUL layout and flexbox seemed like it would be even more problematic...)
Comment hidden (obsolete)
Oh, sorry -- disregard comment 4. I'm now seeing that the element in question (in your WIP patch) has "overflow-y:auto" which should already be making us treat min-height:auto as min-height:0... so clearly that's not the solution.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #2)
> - when this happens, the items end up at the bottom. I tried using
> align-items or justify-content to fix this, but that didn't seem to help.

Devtools show that this is because of a "::before" pseudo-element with "height:100%".  This absorbs all of the free space and pushes the other items to the end. Specifically, it's from this CSS:

=========
#main-window[customize-entered] .customization-target:not(:-moz-any(#PanelUI-contents, #TabsToolbar, #toolbar-menubar))::before {
  /* Prevent jumping of tabs when switching a window between inactive and active (bug 853415). */
  -moz-box-ordinal-group: 0;
  content: "";
  display: -moz-box;
  height: 100%;
  left: 0;
  outline-offset: -2px;
  pointer-events: none;
  position: absolute;
  top: 0;
  width: 100%;
}
=========

If I change that pseudo-element to "display:none", then the other children all snap to the start (and successfully position themselves with "justify-content" etc.)

If you can get rid of that pseudo-element, things might behave more predictably. (Having said that, I don't see why we're setting aside space for that pseudo-element -- it's "position:absolute" so it shouldn't steal any space away from real flex items, ever since bug 874718.)

Updated

2 months ago
QA Contact: gwimberly
Created attachment 8880937 [details] [diff] [review]
WIP v2

Gijs, here's a modified version of your patch which seems to do what you want (I think) -- can you give this a try?
Flags: needinfo?(gijskruitbosch+bugs)
Created attachment 8880958 [details] [diff] [review]
WIP v3

(removed a few unnecessary changes from my prior WIP)
Attachment #8880937 - Attachment is obsolete: true
(Assignee)

Comment 9

2 months ago
Comment on attachment 8880613 [details] [diff] [review]
WIP patch

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

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Created attachment 8880937 [details] [diff] [review]
> WIP v2
> 
> Gijs, here's a modified version of your patch which seems to do what you
> want (I think) -- can you give this a try?

Yes! This is great. Thanks so much. I will have to make some more changes to make the panel arrow stuff work, but this is great. Thanks for the update and the helpful comments!

Which of these changes ends up getting rid of the issue in comment #6 ? Or is it just that the vbox#widget-overflow-fixed-list itself now doesn't grow anymore, and so it stops happening? Is it worth filing a bug on the issue identified there (ie abspos element shouldn't influence the rest of the layout) ?
Attachment #8880613 - Flags: feedback?(dholbert)
(Assignee)

Updated

2 months ago
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #9)
> Which of these changes ends up getting rid of the issue in comment #6 ? Or
> is it just that the vbox#widget-overflow-fixed-list itself now doesn't grow
> anymore, and so it stops happening?

I think that's exactly it, yeah.

> Is it worth filing a bug on the issue
> identified there (ie abspos element shouldn't influence the rest of the
> layout) ?

Yeah -- I'll file a followup and take a look soon.
See Also: → bug 1376040
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8880613 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8880958 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

2 months ago
mozreview-review
Comment on attachment 8881365 [details]
Bug 1373968 - size overflow panel appropriately in customize mode and align arrow with overflow button,

https://reviewboard.mozilla.org/r/152540/#review157640

::: browser/components/customizableui/content/customizeMode.inc.xul
(Diff revision 3)
> -      <html:style html:type="text/html" scoped="scoped">
> -        @import url(chrome://global/skin/popup.css);
> -      </html:style>

Even with this styling, the popup doesn't look right after the flex adjustments, so rather than building on top of that hack which we don't want anyway (see bug 1291515), I just cut us loose.

::: browser/themes/shared/customizableui/customizeMode.inc.css:483
(Diff revision 3)
> +#customization-panelWrapper > .panel-arrowcontent {
> +  color: var(--arrowpanel-color);
> +  background: var(--arrowpanel-background);
> +  background-clip: padding-box;
> +  border: 1px solid var(--arrowpanel-border-color);
> +%ifdef XP_MACOSX
> +  box-shadow: 0 0 0 1px var(--arrowpanel-border-color);
> +  -moz-appearance: none;
> +  border-radius: var(--arrowpanel-border-radius);
> +%else
> +  box-shadow: 0 0 4px hsla(0,0%,0%,.2);
> +%endif

This block copied from the respective popup.css styles, of course. The variables are defined on the root from global.css, so they work.

::: browser/themes/shared/customizableui/customizeMode.inc.css:500
(Diff revision 3)
> +}
> +
> +#customization-panelWrapper > .panel-arrowbox {
> +  position: relative;
> +  height: 10px;
> +%ifndef XP_MACOSX

I could in theory factor this all out into the separate windows/linux/osx stylesheets, but keeping everything in 1 place seems both simpler and easier to follow.

::: browser/themes/shared/customizableui/customizeMode.inc.css:552
(Diff revision 3)
> +  margin-bottom: -1px;
> +}
> +
> +#customization-container:not([photon]) #customization-panelWrapper > .panel-arrowbox > .panel-arrow[side="top"] {
> +  margin-inline-end: 0;
> +  margin-inline-start: 22.35em;

This is @menuPanelWidth@, but that isn't available here (defined in panelUI.inc.css). Because this is going away in 1 release anyway, I don't think it's worth refactoring here.

Updated

2 months ago
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1376560

Comment 16

2 months ago
mozreview-review
Comment on attachment 8881365 [details]
Bug 1373968 - size overflow panel appropriately in customize mode and align arrow with overflow button,

https://reviewboard.mozilla.org/r/152540/#review158084

Thanks, this looks good! Dragging items around feels a bit choppy, but that's not this patch's fault. Something to look at in the future, though!

::: browser/components/customizableui/content/customizeMode.inc.xul
(Diff revision 3)
> -      <html:style html:type="text/html" scoped="scoped">
> -        @import url(chrome://global/skin/popup.css);
> -      </html:style>

\o/ happy to be rid of it.
Attachment #8881365 - Flags: review?(mdeboer) → review+

Comment 17

2 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1cac0f305c70
size overflow panel appropriately in customize mode and align arrow with overflow button, r=mikedeboer

Comment 18

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1cac0f305c70
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1377242

Comment 19

2 months ago
I have successfully reproduced this bug with Nightly 56.0a1 (2017-06-17) (32-bit) on windows 10(32bit)

this bug is verified fix with  latest nightly 56.0a1 (2017-07-04) (32-bit)

Build ID: 20170704030203
Mozilla/5.0 (Windows NT 10.0; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [Bugday-20170705]
I have reproduced this bug with nightly 56.0a1 (2017-06-17) on "Linux Mint (64 Bit).

The bug's fix is now verified on Latest Nightly 56.0a1

Build ID 	20170705100248
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0

[Bugday-20170705]
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.