861703 - Constrain the maximum height of menu panel

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

At the moment, it's up to sub-views themselves to not grow too big. In the long-run, we'll need a way to enforce a maximum height, and make the subview scroll if it's taller than that.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

This is true not just for subviews, but for the panel in general (since it can get quite large if the user decides to drop a ton of widgets in the panel).

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block us from landing on m-c.

Comment 4

[Justin Dolske [:Dolske]]

Not sure if there's another bug on this, but P1 based on it being entirely possible to get a panel taller than your screen. We need to do _something_ about that (since it forces the Customize button offscreen too, at which point you're stuck). At a bare minimum we should crop or scroll the excess, until a better UI proposal comes along. :)

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Unassigning myself, in case anybody wants to take this while I'm on vacation.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Aaaaaand re-assigning myself. I guess this wasn't a very sexy bug. :)

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

So this was a lot harder than originally thought, mainly because -moz-box shrinkwraps in a way that makes overflow: auto items (like the list of toolbar buttons) shrink to their absolute minimum size, as opposed to flexing to fill the available space.

According to dholbert, that's kind of a limitation of -moz-box, and he told me it would probably be fine to use display: flex to use the CSS3 flexbox model. I know it's generally frowned upon to mix XUL and display: flex, but to paraphrase gavin, "Whatever works."

Just need to test this on OSX and Linux and then I'll get a review going.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 2

Makes the menu's max-width a little bit wider on OSX to accommodate a possible scrollbar.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

So this patch re-introduces an old problem where the buttons in the menu panel are no longer aligned as per spec, since I've widened the max-width to account for the possibility of a scrollbar.

That's a little more cosmetic-y though, and something I'd consider a P2 or even a P3, and so I'd prefer to take care of that in a follow-up.

Also, I should finally note that this doesn't automatically handle scrolling really tall subviews when their on their own in the toolbar (example, if the history toolbarbutton were on the nav-bar - the panel that opens won't automatically scroll if the panel is very tall). Those panels can probably take care of themselves if they decide to jam more items into their lists.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 774854 [details] [diff] [review]
Patch v1

Not sure how familiar you are with the panel stuff, Gijs - do you feel OK reviewing it?

Comment 11

[:Gijs (he/him)]

Comment on attachment 774854 [details] [diff] [review]
Patch v1

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

Sorry for the delay, and for this: I'm probably not the right person to review this, no. Punting to Jared (I stole one of his other reviews, and I can look if there's other ones that I can take so this balances out a bit...).

I am also a little scared about using CSS3 flexbox on the XUL elements. I take it there wasn't a simpler fix? Have you tested how this plays with XUL elements that have flex attributes (like the search box) ?

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +44,5 @@
>  }
>  
>  #customization-container {
>    background-color: rgb(247,247,247);
> +  overflow: auto;

This I definitely don't understand. This is the container of all of "customization mode". Why does it need overflow: auto?

Comment 12

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #11)
> Comment on attachment 774854 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 774854 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay, and for this: I'm probably not the right person to
> review this, no. Punting to Jared (I stole one of his other reviews, and I
> can look if there's other ones that I can take so this balances out a
> bit...).
> 
> I am also a little scared about using CSS3 flexbox on the XUL elements. I
> take it there wasn't a simpler fix?

Not that I could see. XUL flex was causing the PanelUI-contents vbox to snap shut to its minimum height when I set overflow-y: auto on it. With CSS3 flexbox, it seems to do the right thing - expand to fill in the available space, height-wise.

> Have you tested how this plays with XUL
> elements that have flex attributes (like the search box) ?

This appears to behave the same way it does on m-c.

> 
> ::: browser/themes/shared/customizableui/customizeMode.inc.css
> @@ +44,5 @@
> >  }
> >  
> >  #customization-container {
> >    background-color: rgb(247,247,247);
> > +  overflow: auto;
> 
> This I definitely don't understand. This is the container of all of
> "customization mode". Why does it need overflow: auto?

In the event that the panel is crazy-tall, in customization mode, the panel holder will be crazy-tall too, which pushes the "Restore to Defaults" button out of view and makes it impossible to add things to the bottom of the panel. It also pushes the "Customize" button out of view, which isn't good since that (anecdotally) seems to be the first exit that people go to to leave customization mode.

overflow: auto here allows the customization tab to scroll to reach those items.

Comment 13

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

Comment on attachment 774854 [details] [diff] [review]
Patch v1

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

What are your thoughts about how you might address the presence/lack of the scrollbar if you push it to a follow-up?

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +44,5 @@
>  }
>  
>  #customization-container {
>    background-color: rgb(247,247,247);
> +  overflow: auto;

Why doesn't the #PanelUI-contents { overflow-y: auto; } rule apply here? Based on your comment in the bug, we should only need to apply overflow-y:auto; on the #PanelUI-contents.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +201,5 @@
>    display: none;
>  }
>  
> +#PanelUI-footer {
> +  display: flex;

What is this needed for? When will the footer have a variable size?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jared Wein [:jaws] from comment #13)
> Comment on attachment 774854 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 774854 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What are your thoughts about how you might address the presence/lack of the
> scrollbar if you push it to a follow-up?
> 

My idea was to have the toolbar buttons and wide items flex to fill up the empty space used by the scrollbar.

> ::: browser/themes/shared/customizableui/customizeMode.inc.css
> @@ +44,5 @@
> >  }
> >  
> >  #customization-container {
> >    background-color: rgb(247,247,247);
> > +  overflow: auto;
> 
> Why doesn't the #PanelUI-contents { overflow-y: auto; } rule apply here?
> Based on your comment in the bug, we should only need to apply
> overflow-y:auto; on the #PanelUI-contents.
> 

That...that is a good question. I fiddled with it a ton, and I couldn't seem to get PanelUI-contents to understand that it needed to overflow when in customization mode. I'll take another stab at it though - because you're right, that'd probably make more sense.

> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +201,5 @@
> >    display: none;
> >  }
> >  
> > +#PanelUI-footer {
> > +  display: flex;
> 
> What is this needed for? When will the footer have a variable size?

This is so that the footer fills the horizontal space. Otherwise, it looks like this:  http://i.imgur.com/Wdndixe.png

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 2

Posting this bit up, to prove I'm doing something over here. :) It's painstaking work, but I'm learning a lot about flex.

Some more notes:

Digging into this a bit further - one of the main reasons I'm advocating a switch to using the CSS3 Flexbox is because it plays way better with display: block. We currently use display: block to show the grid of icons in the panel, and until display: grid is out of experimental-mode, I think that's really our only option there.

So we were already mixing layout modes when we started using display: block in XUL. -moz-box-flex and display: block don't work nicely, because if a child of a flex box is display: block, that child will snap shut to its minimum preferred size instead of flexing. Switching to CSS3 flexbox avoids this.

So this WIP patch I've put up makes it so that if we're in customization mode, the footer of the panel can never go out of view - the panel will grow until the container overflows, and once it does, we switch to "overflow mode" where the list of items in the panel is forced to scroll, and footer remains at the bottom.

There's probably room for cleanup and simplification here.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 3

Ok, starting to settle. Going to get this looking OK on OSX and Linux, and then I'll ask for feedback.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 4

A little cleanup here...

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 777357 [details] [diff] [review]
WIP Patch 4

So this gives us most of the behaviour that (I believe) we want. Namely:

1) If there are more items in panel's customization target than can fit on the screen, that bit of the panel should scroll (without the scrollbar causing a shift in the number of columns).
2) If we're in customization mode, and we put more items in the panel than can fit in the window, the bottom of the panel should lock to the bottom of the window, and the customization target should be scrollable. This way, the user can never "lose" the customization and help buttons.

There are a number of problems with this patch:

1) Questionable mix of CSS3 flexbox and XUL - though I've argued above for the reasons why this is likely acceptable.
2) I'm using text-align: center to center align the toolbar items in the panel. This is so the extra space for the scrollbars doesn't cause uneven margins on either side of the menu panel. This, however, causes any row of icons that has less than 3 items in it to be center aligned. So, if you have a single trailing item in the menu, it's centered. :/ A work-around might be to put in placeholders to fill in the extra slots.

I haven't tested this patch with multiple OS font-sizes, though I imagine it's pretty brittle.

I guess I'm asking for a sanity check here. I've been working with this patch for too long, and I think I need another set of eyeballs. Jared, can you try this patch out and see if what I'm doing is sane?

Comment 19

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #18)
> 2) I'm using text-align: center to center align the toolbar items in the
> panel. This is so the extra space for the scrollbars doesn't cause uneven
> margins on either side of the menu panel. This, however, causes any row of
> icons that has less than 3 items in it to be center aligned. So, if you have
> a single trailing item in the menu, it's centered. :/ A work-around might be
> to put in placeholders to fill in the extra slots.

Could we add another box that's centered, with the contents of the box not being centered? Sounds like that'd solve this problem and wouldn't have the extra work for extra boxes...

Comment 20

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #19)
> (In reply to Mike Conley (:mconley) from comment #18)
> > 2) I'm using text-align: center to center align the toolbar items in the
> > panel. This is so the extra space for the scrollbars doesn't cause uneven
> > margins on either side of the menu panel. This, however, causes any row of
> > icons that has less than 3 items in it to be center aligned. So, if you have
> > a single trailing item in the menu, it's centered. :/ A work-around might be
> > to put in placeholders to fill in the extra slots.
> 
> Could we add another box that's centered, with the contents of the box not
> being centered? Sounds like that'd solve this problem and wouldn't have the
> extra work for extra boxes...

Yeah, that sounds like an avenue worth exploring. I'll look into that today.

I'm also going to see if I can get max-height to work in a way that lets me get rid of that OverflowingPanel thing.

Comment 21

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

Comment on attachment 777357 [details] [diff] [review]
WIP Patch 4

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

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +105,3 @@
>  #PanelUI-contents[type="grid"] {
>    display: block;
> +  overflow-x: hidden;

If #PanelUI-contents[type="grid"] is display:block;, then can you do |margin-left: auto; margin-right: auto;| to center the elements? That won't introduce the inherited centering of the child elements.

Also, we don't have a PanelUI-contents that is not of type="grid" so I think we can remove the attribute selector and merge this rule with the one above it.

Comment 22

[:Gijs (he/him)]

Note: I believe the patch is headed to also fixing bug 885060? :-)

Comment 23

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #22)
> Note: I believe the patch is headed to also fixing bug 885060? :-)

Correct!

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 5

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 6

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 7

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 778660 [details] [diff] [review]
WIP Patch 7

So, this fixes the widget centering problem (thanks jaws!) by having a parent of PanelUI-contents do the scrolling.

One strange regression here is that in customize mode, the contents are jammed too far to the left. This is particularly apparent on OSX. I haven't exactly figured out why exactly - it looks like margin-left: auto and margin-right: auto aren't being respected when in customize mode. But I can't figure out why.

I've also bailed out on the OverflowingPanel thing I was doing before. Instead, I just have the panel fill to the bottom of the screen in customize mode.

Anywho, how's this patch looking, jaws?

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Going to see if I can figure out why margin-left/-right:auto is not working in customization mode.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 8

So this seems to fix the centering problem in customization mode. Need to test this on OSX and Linux, then simplify the patch a little. Just checkpointing my work.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 9

Fixed funky zoom button appearance on OSX.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Ok, this patch seems to do the job. Just going to self-review, simplify it where I can, and then I'll request review.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Ok, I think I've got it.

Note that I had to change head.js's simulateItemDrag to remove the synthesizeDragStart call. synthesizeDrop seems to fire all of the events that we need. Firing those two events right after one another results in leftover placeholders in the panel after finishing each test in browser_880382_drag_wide_widgets_in_panel.js. (Run that test on a UX build and watch the dotted boxes move lower and lower to see what I'm talking about).

For some reason, browser_880382_drag_wide_widgets_in_panel.js would start to fail as soon as the extra placeholders caused the panel to overflow, and the placeholders were out of view (the test with description "Dragging the edit-controls to each of the panel placeholders should append the edit-controls to the bottom of the panel." would begin to fail). This might be that the events were getting confused because a drop event was trying to be fired on an item that was not visible.

So I opted to fix that by getting rid of the extra placeholders.

Due to 1.5 weeks of working on this over trial and error, this patch might be a little nutty. All feedback welcome.

Comment 33

[:Gijs (he/him)]

Comment on attachment 779283 [details] [diff] [review]
Patch v1

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

These are mostly questions. Because I like asking annoying questions, and because this was just a feedback request. Lookin' good, though! Feedback++, if I may be so bold!  :-)

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +11,5 @@
>         noautofocus="true">
>    <panelmultiview id="PanelUI-multiView" mainViewId="PanelUI-mainView">
>      <panelview id="PanelUI-mainView" contextmenu="customizationPanelContextMenu">
> +      <vbox id="PanelUI-contents-scroller">
> +        <vbox id="PanelUI-contents" type="grid"/>

Can we drop type="grid" everywhere? Maybe as a followup?

@@ +17,2 @@
>  
> +      <vbox id="PanelUI-mainView-spring"/>

Dumb question time: can't we just have a bottom margin on the scroller? What is this "spring" doing?

::: browser/components/customizableui/test/head.js
@@ -95,5 @@
>  function simulateItemDrag(toDrag, target) {
>    let docId = toDrag.ownerDocument.documentElement.id;
>    let dragData = [[{type: 'text/toolbarwrapper-id/' + docId,
>                      data: {id: toDrag.id, width: toDrag.getBoundingClientRect().width + 'px'}}]];
> -  synthesizeDragStart(toDrag.parentNode, dragData);

I could see the bug you mentioned even without this patch; if this doesn't break anything, rs=me to land this hunk immediately.

::: browser/themes/linux/customizableui/panelUIOverlay.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  %filter substitution
>  %define menuPanelWidth 23em
> +%define menuPanelMainContentsWidth 21em

Out of curiosity, why are the widths different everywhere?

::: browser/themes/osx/browser.css
@@ -1326,5 @@
>  /* zoom controls */
>  
> -#zoom-controls {
> -  -moz-box-align: center;
> -}

Does this not break anything in the toolbar either?

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +38,5 @@
>  }
> +
> +.panel-combined-item[customizableui-areatype="menu-panel"] > toolbarbutton,
> +toolbarbutton[customizableui-areatype="menu-panel"] {
> +  margin: 0;

Why / what is this fixing? Can we not just change our existing rules so things don't have margins? :-)

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +19,4 @@
>    outline: 1px dashed transparent;
>  }
>  
>  #main-window[customizing-movingItem] .customization-target,

Can we improve this top selector as well? (ditto for the preceding rule)

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ -18,1 @@
>  }

Can you explain briefly why this binding had to go? And should it be removed from panelUI.xml?

@@ +20,5 @@
> +#PanelUI-mainView-spring {
> +  flex: auto;
> +}
> +
> +#PanelUI-multiView > .panel-viewcontainer > .panel-viewstack > .panel-mainview > panelview {

Sadfaces at the selector. Do we need anything beyond .panel-mainview > panelview ? Can we set a class on the panelview itself to make it still simpler?

Comment 34

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #33)
> Comment on attachment 779283 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 779283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These are mostly questions. Because I like asking annoying questions, and
> because this was just a feedback request. Lookin' good, though! Feedback++,
> if I may be so bold!  :-)
> 
> ::: browser/components/customizableui/content/panelUI.inc.xul
> @@ +11,5 @@
> >         noautofocus="true">
> >    <panelmultiview id="PanelUI-multiView" mainViewId="PanelUI-mainView">
> >      <panelview id="PanelUI-mainView" contextmenu="customizationPanelContextMenu">
> > +      <vbox id="PanelUI-contents-scroller">
> > +        <vbox id="PanelUI-contents" type="grid"/>
> 
> Can we drop type="grid" everywhere? Maybe as a followup?
> 

Naw, let's just do it now.

> @@ +17,2 @@
> >  
> > +      <vbox id="PanelUI-mainView-spring"/>
> 
> Dumb question time: can't we just have a bottom margin on the scroller? What
> is this "spring" doing?
> 

That's a good question. When switching to a subview, the spring is what'd grow to push the footer down to match the height of the main view to the displayed subview. It might actually be a relic that we don't need anymore, now that we're so flexy. I'll check.

> ::: browser/components/customizableui/test/head.js
> @@ -95,5 @@
> >  function simulateItemDrag(toDrag, target) {
> >    let docId = toDrag.ownerDocument.documentElement.id;
> >    let dragData = [[{type: 'text/toolbarwrapper-id/' + docId,
> >                      data: {id: toDrag.id, width: toDrag.getBoundingClientRect().width + 'px'}}]];
> > -  synthesizeDragStart(toDrag.parentNode, dragData);
> 
> I could see the bug you mentioned even without this patch; if this doesn't
> break anything, rs=me to land this hunk immediately.
> 

Yeah, I'll file a separate bug for it and just land away.

> ::: browser/themes/linux/customizableui/panelUIOverlay.css
> @@ +3,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  %filter substitution
> >  %define menuPanelWidth 23em
> > +%define menuPanelMainContentsWidth 21em
> 
> Out of curiosity, why are the widths different everywhere?
> 

I kinda just tuned them to look relatively nice on each OS.

> ::: browser/themes/osx/browser.css
> @@ -1326,5 @@
> >  /* zoom controls */
> >  
> > -#zoom-controls {
> > -  -moz-box-align: center;
> > -}
> 
> Does this not break anything in the toolbar either?
> 

D'oh, didn't check until now. But thankfully, no!

> ::: browser/themes/osx/customizableui/panelUIOverlay.css
> @@ +38,5 @@
> >  }
> > +
> > +.panel-combined-item[customizableui-areatype="menu-panel"] > toolbarbutton,
> > +toolbarbutton[customizableui-areatype="menu-panel"] {
> > +  margin: 0;
> 
> Why / what is this fixing? Can we not just change our existing rules so
> things don't have margins? :-)
> 

If I remember correctly, this is to override some default toolbarbutton stylings from toolkit. Assuming that's true, I'm not sure how else to solve this.

> ::: browser/themes/shared/customizableui/customizeMode.inc.css
> @@ +19,4 @@
> >    outline: 1px dashed transparent;
> >  }
> >  
> >  #main-window[customizing-movingItem] .customization-target,
> 
> Can we improve this top selector as well? (ditto for the preceding rule)
> 

Maybe - I'll try.

> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ -18,1 @@
> >  }
> 
> Can you explain briefly why this binding had to go? And should it be removed
> from panelUI.xml?
> 

The binding was for the PanelUI-contents list view, which it looks like we're punting for v1. We're removing the distinction between grid and list for now, so I guess I can chuck the binding. And yeah, I guess I can remove it from panelUI.xml. No point leaving useless code around.

> @@ +20,5 @@
> > +#PanelUI-mainView-spring {
> > +  flex: auto;
> > +}
> > +
> > +#PanelUI-multiView > .panel-viewcontainer > .panel-viewstack > .panel-mainview > panelview {
> 
> Sadfaces at the selector. Do we need anything beyond .panel-mainview >
> panelview ? Can we set a class on the panelview itself to make it still
> simpler?

I'll see how much I can reduce this.

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1 (feedback+'d by Gijs)

(In reply to Mike Conley (:mconley) from comment #34)
> (In reply to :Gijs Kruitbosch from comment #33)
> > Comment on attachment 779283 [details] [diff] [review]
> > Patch v1
> > 
> > Review of attachment 779283 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > These are mostly questions. Because I like asking annoying questions, and
> > because this was just a feedback request. Lookin' good, though! Feedback++,
> > if I may be so bold!  :-)
> > 
> > ::: browser/components/customizableui/content/panelUI.inc.xul
> > @@ +11,5 @@
> > >         noautofocus="true">
> > >    <panelmultiview id="PanelUI-multiView" mainViewId="PanelUI-mainView">
> > >      <panelview id="PanelUI-mainView" contextmenu="customizationPanelContextMenu">
> > > +      <vbox id="PanelUI-contents-scroller">
> > > +        <vbox id="PanelUI-contents" type="grid"/>
> > 
> > Can we drop type="grid" everywhere? Maybe as a followup?
> > 
> 
> Naw, let's just do it now.
> 

Fixed.

> > @@ +17,2 @@
> > >  
> > > +      <vbox id="PanelUI-mainView-spring"/>
> > 
> > Dumb question time: can't we just have a bottom margin on the scroller? What
> > is this "spring" doing?
> > 
> 
> That's a good question. When switching to a subview, the spring is what'd
> grow to push the footer down to match the height of the main view to the
> displayed subview. It might actually be a relic that we don't need anymore,
> now that we're so flexy. I'll check.
> 

Straight-up removing it causes the footer to not move down to the bottom of the panel when viewing a tall subview as per spec. I think it should be possible to remove this "spring" element though, now that we're doing more flexing. I've filed bug 896687 for that.

> > ::: browser/components/customizableui/test/head.js
> > @@ -95,5 @@
> > >  function simulateItemDrag(toDrag, target) {
> > >    let docId = toDrag.ownerDocument.documentElement.id;
> > >    let dragData = [[{type: 'text/toolbarwrapper-id/' + docId,
> > >                      data: {id: toDrag.id, width: toDrag.getBoundingClientRect().width + 'px'}}]];
> > > -  synthesizeDragStart(toDrag.parentNode, dragData);
> > 
> > I could see the bug you mentioned even without this patch; if this doesn't
> > break anything, rs=me to land this hunk immediately.
> > 
> 
> Yeah, I'll file a separate bug for it and just land away.

Spoke too soon - my original fix for this problem was based on incorrect assumptions. See bug 896596.


> > ::: browser/themes/shared/customizableui/customizeMode.inc.css
> > @@ +19,4 @@
> > >    outline: 1px dashed transparent;
> > >  }
> > >  
> > >  #main-window[customizing-movingItem] .customization-target,
> > 
> > Can we improve this top selector as well? (ditto for the preceding rule)
> > 
> 
> Maybe - I'll try.
> 

Hm, after looking at it, I wouldn't want to do that in this bug. Seems orthogonal.

> > ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> > @@ -18,1 @@
> > >  }
> > 
> > Can you explain briefly why this binding had to go? And should it be removed
> > from panelUI.xml?
> > 
> 
> The binding was for the PanelUI-contents list view, which it looks like
> we're punting for v1. We're removing the distinction between grid and list
> for now, so I guess I can chuck the binding. And yeah, I guess I can remove
> it from panelUI.xml. No point leaving useless code around.
> 

Removed.

> > @@ +20,5 @@
> > > +#PanelUI-mainView-spring {
> > > +  flex: auto;
> > > +}
> > > +
> > > +#PanelUI-multiView > .panel-viewcontainer > .panel-viewstack > .panel-mainview > panelview {
> > 
> > Sadfaces at the selector. Do we need anything beyond .panel-mainview >
> > panelview ? Can we set a class on the panelview itself to make it still
> > simpler?
> 
> I'll see how much I can reduce this.

D'oh, just using the ID of that panelview works fine.

Comment 36

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

Comment on attachment 779385 [details] [diff] [review]
Patch v1.1 (feedback+'d by Gijs)

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

As I read through these changes, am I correct to believe that this patch associates Windows with at 1.5em-wide scrollbar, OS X with a 3em-wide scrollbar, and Linux with a 2em-wide scrollbar? That seems awkward to me, and it would be better if we used actual scrollbar widths here.

I also don't expect that we would need any extra padding for OS X when the Lion scrollbars are present. You can use http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#659 to get the width of a scrollbar at runtime.

Overall the changes look fine but I am holding back the r+ until I understand the differences between the widths per platform and if we can use nsIDOMWindowUtils.getScrollbarSize.

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +19,5 @@
>    outline: 1px dashed transparent;
>  }
>  
>  #main-window[customizing-movingItem] .customization-target,
> +#PanelUI-contents > .panel-customization-placeholder {

Do we always want the placeholders to have their outline visible? This change will make that so.

@@ +48,5 @@
> +  /* Hack alert - by manually setting the preferred height to 0, we convince
> +     #PanelUI-mainView to shrink when the window gets smaller in customization
> +     mode. Not sure why that is - might have to do with our intermingling of
> +     XUL flex, and CSS3 Flexbox. */
> +  height: 0px;

height: 0;

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +140,5 @@
>  .panel-combined-button[disabled] > .toolbarbutton-icon {
>    opacity: .5;
>  }
>  
> +#PanelUI-contents toolbarbutton > .toolbarbutton-icon,

As these subselectors are read from right-to-left, there is nothing that is discriminating enough to filter many of the other toolbarbuttons. It would be better to remove the descendent selector here even though it will probably introduce quite verbose selectors.

@@ +173,5 @@
>    background-color: rgba(0, 0, 0, 0.05);
>    border-top: 1px solid rgba(0,0,0,.1);
>    padding: 0;
>    margin: 0;
> +  min-height: 42px;

Can we use `em` here?

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.2 (WIP)

Checkpointing work.

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.3 (WIP)

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.4 (WIP)

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.5

Ok, going to give a self-review, and then I'll explain what I've done here.

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Attachment: 861703-v1.6.diff

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 782036 [details] [diff] [review]
861703-v1.6.diff

(In reply to Jared Wein [:jaws] from comment #36)
> Comment on attachment 779385 [details] [diff] [review]
> Patch v1.1 (feedback+'d by Gijs)
> 
> Review of attachment 779385 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As I read through these changes, am I correct to believe that this patch
> associates Windows with at 1.5em-wide scrollbar, OS X with a 3em-wide
> scrollbar, and Linux with a 2em-wide scrollbar? That seems awkward to me,
> and it would be better if we used actual scrollbar widths here.
> 

Good call. So, what I've done is a little crazy (and I think you already know what it is, but I'll mention it here for posterity) - I'm sampling the system scrollbar width by jamming a temporary iframe into the hidden window. This happens asynchronously, so I've had to do some Promises and Task in PanelUI to make all of that work.

Now I know what you're thinking - oh snap, why are we sampling the scrollbar width for *every window* every time we open the panel (or enter customize mode, if that happens first). We should just do it once globally, since the scrollbar size isn't likely to change from window to window! That's a great point - and perfect fodder for a follow-up. :)

> 
> ::: browser/themes/shared/customizableui/customizeMode.inc.css
> @@ +19,5 @@
> >    outline: 1px dashed transparent;
> >  }
> >  
> >  #main-window[customizing-movingItem] .customization-target,
> > +#PanelUI-contents > .panel-customization-placeholder {
> 
> Do we always want the placeholders to have their outline visible? This
> change will make that so.
> 

Youch, you're right. Reverted (and simplified).

> @@ +48,5 @@
> > +  /* Hack alert - by manually setting the preferred height to 0, we convince
> > +     #PanelUI-mainView to shrink when the window gets smaller in customization
> > +     mode. Not sure why that is - might have to do with our intermingling of
> > +     XUL flex, and CSS3 Flexbox. */
> > +  height: 0px;
> 
> height: 0;
> 

Done.

> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +140,5 @@
> >  .panel-combined-button[disabled] > .toolbarbutton-icon {
> >    opacity: .5;
> >  }
> >  
> > +#PanelUI-contents toolbarbutton > .toolbarbutton-icon,
> 
> As these subselectors are read from right-to-left, there is nothing that is
> discriminating enough to filter many of the other toolbarbuttons. It would
> be better to remove the descendent selector here even though it will
> probably introduce quite verbose selectors.
> 

Ok, I think I've simplified these - using the toolbarpaletteitem place and toolbarbutton/item customizableui-areatype attributes.

> @@ +173,5 @@
> >    background-color: rgba(0, 0, 0, 0.05);
> >    border-top: 1px solid rgba(0,0,0,.1);
> >    padding: 0;
> >    margin: 0;
> > +  min-height: 42px;
> 
> Can we use `em` here?

Yep, we can!

Comment 43

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

Comment on attachment 782036 [details] [diff] [review]
861703-v1.6.diff

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

LGTM, I tested it out and it works great!

::: browser/components/customizableui/content/panelUI.js
@@ +165,5 @@
> +        // do a bit of hackery. In particular, we sample the system scrollbar width,
> +        // and then use that to calculate a new width for the scroller.
> +        this._scrollWidth = (yield this._sampleScrollbarWidth()) + "px";
> +        let cstyle = window.getComputedStyle(this.scroller);
> +        let widthStr = cstyle.getPropertyValue("width");

let widthStr = cstyle.width;

@@ +171,5 @@
> +        // the scroller too. We'll use that in our final calculation so
> +        // that if a scrollbar appears, we don't have the contents right
> +        // up against the edge of the scroller.
> +        let paddingLeft = cstyle.getPropertyValue("padding-left");
> +        let paddingRight = cstyle.getPropertyValue("padding-right");

let paddingLeft = cstyle.paddingLeft;
let paddingRight = cstyle.paddingRight;

Comment 44

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

(In reply to Mike Conley (:mconley) from comment #42) 
> Good call. So, what I've done is a little crazy (and I think you already
> know what it is, but I'll mention it here for posterity) - I'm sampling the
> system scrollbar width by jamming a temporary iframe into the hidden window.
> This happens asynchronously, so I've had to do some Promises and Task in
> PanelUI to make all of that work.
> 
> Now I know what you're thinking - oh snap, why are we sampling the scrollbar
> width for *every window* every time we open the panel (or enter customize
> mode, if that happens first). We should just do it once globally, since the
> scrollbar size isn't likely to change from window to window! That's a great
> point - and perfect fodder for a follow-up. :)

I'll buy that for a dollar. Can you file a follow-up bug for this?

Comment 45

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jared Wein [:jaws] from comment #44)
> (In reply to Mike Conley (:mconley) from comment #42) 
> > Good call. So, what I've done is a little crazy (and I think you already
> > know what it is, but I'll mention it here for posterity) - I'm sampling the
> > system scrollbar width by jamming a temporary iframe into the hidden window.
> > This happens asynchronously, so I've had to do some Promises and Task in
> > PanelUI to make all of that work.
> > 
> > Now I know what you're thinking - oh snap, why are we sampling the scrollbar
> > width for *every window* every time we open the panel (or enter customize
> > mode, if that happens first). We should just do it once globally, since the
> > scrollbar size isn't likely to change from window to window! That's a great
> > point - and perfect fodder for a follow-up. :)
> 
> I'll buy that for a dollar. Can you file a follow-up bug for this?

Done! Filed bug 898783. Thanks for the review!

Landed in UX as https://hg.mozilla.org/projects/ux/rev/9001015f448f

Comment 46

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/9001015f448f