1033395 - Panels Should Be Anchored from the Same Height on the Toolbar

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[:Gijs (he/him)]

This is Windows:

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

And this is OS X:

http://i.imgur.com/54aNqVE.png

Note the overlapping of the arrow.

There are technical reasons why this is the case (in particular, we create the button look on Windows by means of padding on the icon, and use the button's padding to enlarge the clickable area - which is not expected on mac, and therefore we don't do it there).

However, Mike wasn't convinced this was 'by design', and therefore asked me to file this.

Stephen, can you clarify which of these screenshots looks correct to you, as far as the arrow's positioning is concerned?

Comment 1

[Stephen Horlander [:shorlander]]

Ideally they would align visually and structurally. My vague recollection from poking at it awhile ago however is that isn't possible because of the way panels work? Mostly it revolves around the way the shadow is constructed.

On Windows I think we are doing something weird with margins and padding to creating the shadow with CSS. On OS X we are using the platform native shadow.

Unless you are talking about the position of the arrow and the slight panel overlap on the toolbar, in which case, yeah it would be nice if that was consistent also ;) Not sure why it isn't.

Comment 2

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #1)
> Unless you are talking about the position of the arrow and the slight panel
> overlap on the toolbar, in which case, yeah it would be nice if that was
> consistent also ;) Not sure why it isn't.

Yes. Which form is preferred, overlap or aligning exactly to the bottom border?

Comment 3

[Stephen Horlander [:shorlander]]

Attachment: Panel Alighnment Offset

(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Stephen Horlander [:shorlander] from comment #1)
> > Unless you are talking about the position of the arrow and the slight panel
> > overlap on the toolbar, in which case, yeah it would be nice if that was
> > consistent also ;) Not sure why it isn't.
> 
> Yes. Which form is preferred, overlap or aligning exactly to the bottom
> border?

Slightly overlapping. 2px offset.

Comment 4

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #3)
> Created attachment 8572833 [details]
> Panel Alighnment Offset
> 
> (In reply to :Gijs Kruitbosch from comment #2)
> > (In reply to Stephen Horlander [:shorlander] from comment #1)
> > > Unless you are talking about the position of the arrow and the slight panel
> > > overlap on the toolbar, in which case, yeah it would be nice if that was
> > > consistent also ;) Not sure why it isn't.
> > 
> > Yes. Which form is preferred, overlap or aligning exactly to the bottom
> > border?
> 
> Slightly overlapping. 2px offset.

This is amusing to me because I meant "compared to the bottom border of the button", not the border of the toolbar. We can't really control the toolbar overlap directly because we position panels based on their anchors - the buttons. I guess we still want to have an overlap with the button because it almost (but not quite, see e.g. devedition) guarantees an overlap with the border of the toolbar?

Comment 5

[Stephen Horlander [:shorlander]]

See bug 1206100 comment 10.

Comment 7

[Amin Al Hazwani [:amin] (Firefox UX)]

Duplicate https://bugzilla.mozilla.org/show_bug.cgi?id=1375211?

Comment 11

[Erica Wright [:ewright]]

Attachment: Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

Review commit: https://reviewboard.mozilla.org/r/179468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179468/

Comment 12

[Erica Wright [:ewright]]

Just using css for this. I'm not sure if you'd prefer an approach which changes the anchors instead, or fixes the root of the problem so that the css edits don't have to be there...

Comment 13

[:Gijs (he/him)]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

https://reviewboard.mozilla.org/r/179468/#review184714

Thanks for taking this on! It's both straightforward ("how hard can it be to just make all of these things match") and, uh, also not so straightforward...

(In reply to Erica Wright [:ewright] from comment #12)
> Just using css for this. I'm not sure if you'd prefer an approach which
> changes the anchors instead, or fixes the root of the problem so that the
> css edits don't have to be there...

Well, I'm not sure what the root of the problem is at this stage... :-(

The icons in the url bar seem to anchor at (not overlap) the bottom of the url bar, which I believe is correct (but it'd be helpful to have that confirmed from Stephen to make sure...).

From a quick check, it looks like the downloads panel overlaps on OS X is just wrong, so removing that is correct, but it otherwise anchors on the icon.

I see e.g. the library button not doing the right thing, which I presume is what the margin changes to #customizationui-widget-panel are fixing? The confusing thing is, the code here: https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/browser/components/customizableui/content/panelUI.js#749-754 should be using the icon already. Looking at it more closely, it appears we're still adding padding to the icons, which is responsible for the gap between the icon and the panel.

That padding is controlled by the --toolbarbutton-inner-padding CSS variable. The value of that variable changes for the compact and touch UI densities. We probably need to take that into account for the anchoring, so hardcoding to 3px won't work right for compact and/or touch mode.

What puzzles me is why the downloads button/panel doesn't behave the same way as the library one. I think it's just a bit of a special snowflake. It has its own overlay and XBL binding...

For all the non-inside-urlbar-icons, I think I would suggest doing the following:

- the patch should ensure that we always anchor on the icon child, if we don't already
- we should then compensate for toolbarbutton-inner-padding where necessary. I'm not sure if the negative top margin will have side effects, like cutting off the top of the panel or its shadow when on OSX - best doublecheck that.
- some of these items are special, which is probably at least the downloads button and maybe the bookmarks menu button and maybe other items (would be especially good to check badged webextension add-ons that also have panels (maybe ublock origin?)). We should override the above for those items.

Alllllllternatively... (and I could be convinced either way. Might make sense to have a chat about this on slack or IRC (tomorrow, it's a bit late here).) we could start anchoring everything on the buttons (instead of their icons), and just define a hardcoded offset somewhere to get to where we think the icon ought to be. If the downloads button vs. library button offset is anything to go by, that might be simpler. But then it's easier for the two to get out of sync. I guess it depends how much of the current issue is a result of "non-standard" buttons or anchoring.

Finally, whichever one we go with, we should check the dupes and/or any other "special" panels we might be forgetting about.

Comment 14

[:Gijs (he/him)]

(In reply to :Gijs from comment #13)
> The icons in the url bar seem to anchor at (not overlap) the bottom of the
> url bar, which I believe is correct (but it'd be helpful to have that
> confirmed from Stephen to make sure...).

--> Stephen?

Comment 15

[Stephen Horlander [:shorlander]]

(In reply to :Gijs from comment #14)
> (In reply to :Gijs from comment #13)
> > The icons in the url bar seem to anchor at (not overlap) the bottom of the
> > url bar, which I believe is correct (but it'd be helpful to have that
> > confirmed from Stephen to make sure...).
> 
> --> Stephen?

They currently do, but they shouldn't. See https://bugzilla.mozilla.org/show_bug.cgi?id=1206100#c10

Comment 17

[Erica Wright [:ewright]]

Attachment: Screen Shot 2017-09-14 at 1.25.38 PM.png

These are essentially the three states before the patch. I have figured that the bookmarks dropdown was the correct one, a slight overlap. However, the above answer has confused me more. are any of these the correct height?

Comment 18

[Stephen Horlander [:shorlander]]

The Panel hanging off of the Menu (Hamburger) is currently correct.

Comment 20

[Erica Wright [:ewright]]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179468/diff/1-2/

Comment 21

[Erica Wright [:ewright]]

Attachment: Screen Shot 2017-09-21 at 3.20.05 PM.png

The results of the current patch. let me know if you know of any more doorhangers I should check ;)

Comment 22

[Erica Wright [:ewright]]

>Alllllllternatively... (and I could be convinced either way. Might make sense to have a chat about this on slack or IRC >(tomorrow, it's a bit late here).) we could start anchoring everything on the buttons (instead of their icons), and just define >a hardcoded offset somewhere to get to where we think the icon ought to be. If the downloads button vs. library button offset is >anything to go by, that might be simpler. But then it's easier for the two to get out of sync. I guess it depends how much of >the current issue is a result of "non-standard" buttons or anchoring.

this is essentially what I did. I tweaked the anchors so they all aligned outside the buttons (16X16 plus 6px padding), then I've shifted them all up by 6px.

The library button is  - as you say  - "a special snowflake" because its menu is hardcoded into it - and doesn't seem to use the anchor pattern, so that one gets shifted by 11px.

Comment 23

[:Gijs (he/him)]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

https://reviewboard.mozilla.org/r/179468/#review187862

At least on OS X, this shows the panel too high on the downloads button and menu button. It does look like it's going in the right direction. I think the adjustments for the downloads button and/or badged buttons (the hamburger panel also has a badged button stack thing going on, for update badges etc.) might need more work, or something.

Comment 24

[Erica Wright [:ewright]]

(In reply to :Gijs from comment #23)
> Comment on attachment 8907781 [details]
> Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.
> 
> https://reviewboard.mozilla.org/r/179468/#review187862
> 
> At least on OS X, this shows the panel too high on the downloads button and
> menu button. It does look like it's going in the right direction. I think
> the adjustments for the downloads button and/or badged buttons (the
> hamburger panel also has a badged button stack thing going on, for update
> badges etc.) might need more work, or something.

Haha, that's very vague, I'm not sure what to do with that information....let me know if you want to chat at some point, I could do with some advice. Also, I don't see the menu nor downloads as being too high, as you can see in the screenshot I posted, I'm also on OS X.

Comment 25

[:Gijs (he/him)]

Attachment: Download + hamburger panel

(In reply to Erica Wright [:ewright] from comment #24)
> (In reply to :Gijs from comment #23)
> > Comment on attachment 8907781 [details]
> > Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.
> > 
> > https://reviewboard.mozilla.org/r/179468/#review187862
> > 
> > At least on OS X, this shows the panel too high on the downloads button and
> > menu button. It does look like it's going in the right direction. I think
> > the adjustments for the downloads button and/or badged buttons (the
> > hamburger panel also has a badged button stack thing going on, for update
> > badges etc.) might need more work, or something.
> 
> Haha, that's very vague, I'm not sure what to do with that
> information....let me know if you want to chat at some point, I could do
> with some advice. Also, I don't see the menu nor downloads as being too
> high, as you can see in the screenshot I posted, I'm also on OS X.

You're absolutely right, sorry for not being more constructive - and I'd missed the screenshot. :-(

I'll be around for a chat on Monday. I've attached a screenshot of what I was seeing...

... however, it looks like it's a result of a caching issue. After restarting with -purgecaches, it's gone away.

I did notice a different issue, which is that the offsets for the in-urlbar buttons are now too short (too much overlap with the icons) in compact density mode. That does stay after a few restarts, so I'm hoping it's not just me.

Do the margins need to be different for different densities? It feels like maybe they do. I'm on mac right now so can't check what this looks like on 'touch', but it might be worth having a look at that on Windows? I would imagine the margins need to incorporate the toolbarbutton padding CSS variable whose value changes per the UI density.

The other question is what this does to items that are on other toolbars. Off-hand, it looks like the margin isn't right for items on the bookmarks toolbar because they have almost no vertical padding at all, and it's probably not exactly right on the tabstrip either (where ui density might also make a difference, or maybe not...).

The tricky thing is that I'm not sure how to vary the margin based on the toolbar, because the margin is set on the panel rather than the node, so there's no way (with pure CSS) to select for the toolbar in which the anchor lives. Ugh, should have thought of that before.

We might need some JS that applies some attribute to the panel when it's being opened? Alternatively, you could move the magical -6 / -11 numbers into JS. openPopup takes x/y offsets, and you could use those to offset where the popup appears, too, and it would be easier there to change the relative offset based on the toolbar. On the other hand, you'd have to adjust more consumers, maybe.

Finally, looking at the code again, I am slightly confused as to why we need the -6/-11 offsets while you're also adjusting the JS to select the inner icons. To me, the panels look like they're pointing to the icons exactly (in normal density). Why do we still need to change the margin, if we're already changing the JS to use the icons as anchors (rather than buttons)?

Sorry, this is probably still pretty rambly (in my defense, I have a cold at the moment) - let's chat more on Monday.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

https://reviewboard.mozilla.org/r/179468/#review187992

::: browser/components/downloads/content/indicator.js:629
(Diff revision 2)
>    },
>  
>    get indicatorAnchor() {
>      let widgetGroup = CustomizableUI.getWidget("downloads-button");
>      if (widgetGroup.areaType == CustomizableUI.TYPE_MENU_PANEL) {
>        return widgetGroup.forWindow(window).anchor;

Do we need to find the icon in here, too? (this gets hit if you put the downloads button in the overflow panel, and it will return the overflow button, I think?)

Comment 27

[Erica Wright [:ewright]]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179468/diff/2-3/

Comment 28

[Erica Wright [:ewright]]

:Gijs, I believe this is going to need special casing on Linux too - will address that so long as you approve of this method. Unfortunately, because the bookmarks Menu `BMB_bookmarksPopup` is built into the widget it does all sorts of funny things in all sorts of conditions, So I've added a few pixels to it here and there depending on the position and the compact/touch mode.

Everything have been tested on Mac - 2 modes and 3 toolbars. And on Windows, 3 modes and 4 toolbars.

Comment 29

[Erica Wright [:ewright]]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179468/diff/3-4/

Comment 30

[Erica Wright [:ewright]]

Now has been tested on Linux. Also tested with an extension that adds an icon with dropdown in the pageActions, and an extension that adds a dropdown on their widget. Linux 3 modes and 3 toolbars.

Comment 31

[:Gijs (he/him)]

I meant to look at this today but I've been a bit swamped. I aim to review it tomorrow. Apologies for the delay.

Comment 32

[:Gijs (he/him)]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

https://reviewboard.mozilla.org/r/179468/#review191926

One thing I noticed in testing: the bookmarks menu button's offset is wrong if you put it in the menubar (on windows 10, compact density).

I need to look at this more, but I am not around tomorrow and likely won't have more time tonight, so I'm leaving you with a WIP review and keeping the r? so I remember to come back to this. Sorry, too much stuff on my plate right now. :-(

I'm still wondering if we could simplify some of the multiple magic numbers things by using toolbarbutton-inner-padding with calc()... but I haven't had time to look into that in enough detail to say for sure.

Thanks for working on this though, the state with the patch looks so much neater already!

::: browser/components/downloads/content/downloads.js:561
(Diff revision 4)
> +      if (anchor.closest("toolbar").id == "PersonalToolbar") {
> +        this.panel.classList.add("bookmarks-toolbar");
> +      } else {
> +        this.panel.classList.remove("bookmarks-toolbar");
> +      }

`classList` actually has a `toggle` method that you could use here:

```js
let onBookmarksToolbar = !!anchor.closest("#PersonalToolbar");
this.panel.classList.toggle("bookmarks-toolbar", onBookmarksToolbar);
```

( https://developer.mozilla.org/en-US/docs/Web/API/Element/classList )

::: browser/themes/shared/customizableui/panelUI.inc.css:211
(Diff revision 4)
> +  margin-top: -6px;
> +}
> +
> +.cui-widget-panel.bookmarks-toolbar,
> +#downloadsPanel.bookmarks-toolbar,
> +#widget-overflow.bookmarks-toolbar,

The widget-overflow node can't move, so you can omit this. :-)

::: browser/themes/shared/customizableui/panelUI.inc.css:212
(Diff revision 4)
> +}
> +
> +.cui-widget-panel.bookmarks-toolbar,
> +#downloadsPanel.bookmarks-toolbar,
> +#widget-overflow.bookmarks-toolbar,
> +#editBookmarkPanel.bookmarks-toolbar,

Neither can the edit bookmark panel, I think.

::: browser/themes/shared/toolbarbuttons.inc.css:119
(Diff revision 4)
>  
>  toolbar .toolbarbutton-1 > menupopup.cui-widget-panel {
>    margin-top: -8px;
>  }
>  
> +#main-window[uidensity=compact] toolbar .toolbarbutton-1 > menupopup.cui-widget-panel {

The 'toolbar' bit of this selector is probably not useful.

It also seems that all the `[uidensity]` selectors in the tree right now use the `:root` pseudoselector rather than `#main-window`, so for all these selectors we should probably do the same thing.

Finally, do these selectors match anything beside the bookmarks toolbar menu button? If not, maybe we should make that explicit by selecting for that button rather than `.toolbarbutton-1`.

::: browser/themes/windows/customizableui/panelUI.css:101
(Diff revision 4)
> +  margin-top: -9px;
> +}
> +
> +#main-window[uidensity=touch] #pageActionPanel,
> +#main-window[uidensity=touch] #editBookmarkPanel,
> +#main-window[uidensity=touch] #pageActionActivatedActionPanel,

Do we not need to change the pageActionActivatedActionPanel in the shared panelUI styles, too?

Comment 33

[Erica Wright [:ewright]]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

https://reviewboard.mozilla.org/r/179468/#review191926

> The 'toolbar' bit of this selector is probably not useful.
> 
> It also seems that all the `[uidensity]` selectors in the tree right now use the `:root` pseudoselector rather than `#main-window`, so for all these selectors we should probably do the same thing.
> 
> Finally, do these selectors match anything beside the bookmarks toolbar menu button? If not, maybe we should make that explicit by selecting for that button rather than `.toolbarbutton-1`.

No, this only matches the bookmarks toolbar menu - I was following the style used for selecting it in the line above, I figured, just in case someone follows the same pattern in the future... However I have seen in other files #BMB_bookmarksPopup instead.

Comment 34

[:Gijs (he/him)]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

https://reviewboard.mozilla.org/r/179468/#review192784

OK, I'm done going through this. I don't think the CSS variable gets us much here after all, except maybe for the bookmarks menu button. I think we can split out the behaviour of the bookmarks menu button on the bookmarks toolbar, see reasoning below. Hopefully that helps make this patch a bit smaller.

For further reviews while I'm away, you could probably ping :johannh ? :-)

::: browser/themes/shared/toolbarbuttons.inc.css:131
(Diff revision 4)
> +#PersonalToolbar .toolbarbutton-1 > menupopup.cui-widget-panel,
> +#main-window[uidensity=compact] #PersonalToolbar .toolbarbutton-1 > menupopup.cui-widget-panel {
> +  margin-top: -1px;
> +}

So, this set of selectors is a bit confusing, because the first always matches when the second matches. I guess the second is here to override the :not(touch) ones, or something ?

I tried to work out what all the rules for the bookmarks menu button in the `#PersonalToolbar` case were doing. It's a bit confusing, because the actual vertical padding on buttons in the personaltoolbar doesn't change based on the ui density. However, the padding on bookmark items changes, and the other buttons stretch accordingly. As a result, the changing offsets here match the values of `toolbarbutton-inner-padding`, but only if the bookmarks toolbar items are in their default position.

I think we should make the size of the padding of those buttons explicit, but this bug isn't the right place to do that in. Can we move the adjustments for the bookmarks menu button in the `#PersonalToolbar` case (but not the general, navbar or menubar styling) into a followup bug? That should simplify this patch a little bit.

::: browser/themes/windows/customizableui/panelUI.css:87
(Diff revision 4)
>      border-radius: 0;
>    }
>  }
>  
> +#toolbar-menubar .toolbarbutton-1 > menupopup.cui-widget-panel,
> +#main-window .browser-extension-panel {

This is 1px less than in the 'shared' case, do you know why? Does this really need the `#main-window` part of the selector?

If we move the bookmarks menu button on the `#PersonalToolbar` bits to a separate patch, I guess this CSS can mirror the CSS you're using on Linux (:not([compact])) and then we can remove the shared browser-extension-panel/bookmarks-menu-button rule in this block you're adding. Still not sure why Windows/Linux need a different value here than OSX, though.

Comment 39

[Erica Wright [:ewright]]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179468/diff/4-5/

Comment 40

[Erica Wright [:ewright]]

To explain some of the changes in there, the bug has been simplified by bringing the styles into the same file, for example, previously I chose to keep some of the bookmarks menu button styles in the toolbar icon file, since that's where they were before - but I think it is easier to see similarities and differences this way. 
Also, as Gijs suggested, I have removed the lines referencing the bookmarks menu when it is in the #PersonalToolbar - after the simplifications this consisted of 3 rules that can be added back in another bug - so keep in mind when testing, that that one case will not line up with the others.

Comment 41

[Johann Hofmann [:johannh]]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

https://reviewboard.mozilla.org/r/179468/#review194032

Heh, this is the sort of review I would have forwarded to Gijs. At least he had a good look at this already, so I think we can get it through the home-stretch.

I have to say that I'm generally a bit unhappy with the way that this uses magic numbers to adjust the different panels to compact and touch mode (I think Gijs mentioned this as well). I'm under the impression that the only relevant change when switching modes in our case is the --toolbarbutton-inner-padding variable. Would you think it's possible to adjust the compact and touch specific pieces to be relative to --toolbarbutton-inner-padding? I'm going to take another stab at it as well, just wanted to leave the remaining comments for now.

Thank you for working on this!

::: browser/themes/shared/customizableui/panelUI.inc.css:201
(Diff revision 5)
>  
>  panelview {
>    -moz-box-orient: vertical;
>    -moz-box-flex: 1;
>  }
>  

Please add a top-level comment before this section to outline what this does, and feel free to leave whatever crossed your mind when you made these changes in comments below.

CSS rules that move things up and down with arbitrary numbers, IMO, can't get enough comments ;)

::: browser/themes/shared/customizableui/panelUI.inc.css:206
(Diff revision 5)
>  
> +#BMB_bookmarksPopup {
> +  margin-top: -8px;
> +}
> +
> +.cui-widget-panel,

This manipulates the #sidebarMenu-popup panel in a way that looks a little off, at least to me. Maybe it is better to list the panels explicitly by ID here, to avoid such side effects in the future?

::: browser/themes/shared/customizableui/panelUI.inc.css:238
(Diff revision 5)
> +:root[uidensity=touch] #pageActionActivatedActionPanel,
> +:root[uidensity=touch] .browser-extension-panel {
> +  margin-top: -4px;
> +}
> +
> +:root:not([uidensity=touch]) #nav-bar #BMB_bookmarksPopup {

This rule overrides the rule in line 244 in the nav-bar, which is not what we want, at least for me on OSX.

Could you simply change this rule to use :root:not([uidensity])?

Again, this would vastly benefit from a short comment.

Comment 42

[Johann Hofmann [:johannh]]

(In reply to Johann Hofmann [:johannh] from comment #41)
> Could you simply change this rule to use :root:not([uidensity])?

Or just remove the :root part entirely, that sounds better :D

Comment 43

[Erica Wright [:ewright]]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179468/diff/5-6/

Comment 44

[Erica Wright [:ewright]]

(In reply to Johann Hofmann [:johannh] from comment #42)
> (In reply to Johann Hofmann [:johannh] from comment #41)
> > Could you simply change this rule to use :root:not([uidensity])?
> 
> Or just remove the :root part entirely, that sounds better :D

I can't remove the :root part since I need the `:root[uidensity=compact] #BMB_bookmarksPopup` rule below it it "win" even when it is in the nav-bar.

Comment 45

[Johann Hofmann [:johannh]]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

https://reviewboard.mozilla.org/r/179468/#review194394

Thank you, I think with the new variable and comments this is much easier to understand!

There are just a few things to clean up, then this should be ready to land!

Great work!

::: browser/themes/shared/customizableui/panelUI.inc.css:202
(Diff revision 6)
>  panelview {
>    -moz-box-orient: vertical;
>    -moz-box-flex: 1;
>  }
>  
> +/* This section is to keep all the drop down panels the same, shift the panel`s top

"This section is to keep all the drop down panels the same"

would maybe be clearer as

"This section is to anchor all the drop down panels at the same height"?

::: browser/themes/shared/customizableui/panelUI.inc.css:227
(Diff revision 6)
> +#identity-popup {
> +  margin-top: calc(var(--toolbarbutton-inner-padding) - 6px);
> +}
> +
> +/* The bookmarks toolbar is too thin to have the panels overlap 6px. */
> +.cui-widget-panel.bookmarks-toolbar,

Unfortunately this is now getting overriden by the IDs in line 204-207, so I'd suggest also explicitly listing elements here :)

::: browser/themes/shared/toolbarbuttons.inc.css:9
(Diff revision 6)
>  
>  :root {
>    --toolbarbutton-hover-transition-duration: 150ms;
>  
>    --toolbarbutton-inner-padding: 6px;
> +  --urlbar-icon-padding: 6px;

I feel like these variables would be better placed in urlbar-searchbar.inc.css.

::: browser/themes/shared/urlbar-searchbar.inc.css:172
(Diff revision 6)
>  }
>  
>  .urlbar-icon {
>    width: 28px;
>    height: 28px;
>    /* 28x28 box - 16x16 image = 12x12 padding, 6 on each side */

Please move these comments to the declaration of --urlbar-icon-padding.

::: browser/themes/shared/urlbar-searchbar.inc.css:184
(Diff revision 6)
>  
>  :root[uidensity=compact] .urlbar-icon {
>    width: 24px;
>    height: 24px;
>    /* 24x24 box - 16x16 image = 8x8 padding, 4 on each side */
> -  padding: 4px;
> +  padding: var(--urlbar-icon-padding);

Nit: You can remove this.

::: browser/themes/shared/urlbar-searchbar.inc.css:191
(Diff revision 6)
>  
>  :root[uidensity=touch] .urlbar-icon {
>    width: 30px;
>    height: 30px;
>    /* 30x30 box - 16x16 image = 14x14 padding, 7 on each side */
> -  padding: 7px;
> +  padding: var(--urlbar-icon-padding);

Nit: You can remove this.

::: browser/themes/windows/customizableui/panelUI.css:86
(Diff revision 6)
>    #zoom-controls@inAnyPanel@ > toolbarbutton {
>      border-radius: 0;
>    }
>  }
>  
> +#toolbar-menubar #BMB_bookmarksPopup {

Can you add a short comment on what this does?

Comment 46

[Erica Wright [:ewright]]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179468/diff/6-7/

Comment 47

[Johann Hofmann [:johannh]]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

https://reviewboard.mozilla.org/r/179468/#review194846

Looks great, thank you!

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b9d2ebcf44e
Panels Should Be Anchored from the Same Height on the Toolbar. r=johannh

Comment 49

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/7068ece575da for https://treeherder.mozilla.org/logviewer.html#?job_id=137400509&repo=autoland, so far looking like it's Mac-only.

Comment 50

[Erica Wright [:ewright]]

:johann can you look at this again? I had to change a test to make it pass, it was verifying the old anchors for the downloads panel, which was changed in this patch.

Comment 51

[Erica Wright [:ewright]]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179468/diff/7-8/

Comment 52

[Johann Hofmann [:johannh]]

That seems reasonable to me, you're clear for landing :)

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2129917e3d5
Panels Should Be Anchored from the Same Height on the Toolbar. r=johannh

Comment 54

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e2129917e3d5

Comment 55

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Works for me in Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0 ID:20171018100140. I am only not sure about the position of the popup - the top "corner" begins right below the bottom edge of the icon (especially in the compact view). I would expect it to match the bottom edge of the button (highlighted background area).

Comment 56

[Erica Wright [:ewright]]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

Approval Request Comment
[Feature/Bug causing the regression]: the panels' dropdowns are not vertically aligned
[User impact if declined]: does not look polished
[Is this code covered by automated tests?]: no, entirely visual
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: not sure, but steps below.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: not risky, mostly only visual effects. small changes to the panels' anchor nodes.
[String changes made/needed]: none

How I've tested it:
1. Open browser inspector, click "disable popup auto hide" button
2. Click some toolbar icons that have dropdowns (menu, bookmarks menu, library, overflow etc.) and observe that the drop down panels are the same height.
3. Click on things in the url bar (identity icon, pocket icon, star) and observe that the drop down panels are the same height.
4. Move icons to tabs toolbar, bookmarks bar, menu bar and repeat.
5. Try in compact, touch and default modes.
6. Try with an extension that adds a toolbar icon with a dropdown, and an extension that adds an icon to the urlbar with a dropdown.
7. Repeat all of this, in Windows, Mac and Linux.

Note: The bookmarks menu when in the bookmarks toolbar looks incorrect and is not included in this patch.

Comment 57

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8907781 [details]
Bug 1033395 - Panels Should Be Anchored from the Same Height on the Toolbar.

This is not a new regression and the patch is not a small change. I'd prefer to let this change ride the 58 train.