Closed Bug 1373968 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.2 - Jul 10
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- verified

People

(Reporter: arai, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [photon-structure])

Attachments

(2 files, 3 obsolete files)

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
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]
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-structure][triage] → [photon-structure]
Attached patch WIP patch (obsolete) — Splinter Review
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)
Iteration: --- → 56.1 - Jun 26
Flags: qe-verify? → qe-verify+
Priority: P2 → P1
(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...)
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.)
QA Contact: gwimberly
Attached patch WIP v2 (obsolete) — Splinter Review
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)
Attached patch WIP v3 (obsolete) — Splinter Review
(removed a few unnecessary changes from my prior WIP)
Attachment #8880937 - Attachment is obsolete: true
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)
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: → 1376040
Attachment #8880613 - Attachment is obsolete: true
Attachment #8880958 - Attachment is obsolete: true
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.
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
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+
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
https://hg.mozilla.org/mozilla-central/rev/1cac0f305c70
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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]
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.