968595 - Update OS X Arrow Panel Styling

Status: VERIFIED FIXED

(Toolkit :: Themes, enhancement)

Description

[Stephen Horlander [:shorlander]]

Attachment: Update OS X Arrow Panels - 01

This patch updates the styles of the menu panels and fixes some inconsistencies.

- Reduces the size of the arrow
- Lightens them up
- Tightens the border-radius
- Aligns the styles of the various footers
- Adjusts the vertical alignment of the panels
- Fixes the missing shadow on the Menu Panel (bug 930064)

Comment 1

[Stephen Horlander [:shorlander]]

Attachment: Panels Screenshot - Before

Comment 2

[Stephen Horlander [:shorlander]]

Attachment: Panels Screenshot - After

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8371186 [details] [diff] [review]
Update OS X Arrow Panels - 01

>--- a/browser/themes/osx/browser.css
>+++ b/browser/themes/osx/browser.css

>+#identity-popup {
>+  margin-top: -1px;
>+}

>--- a/browser/themes/osx/downloads/downloads.css
>+++ b/browser/themes/osx/downloads/downloads.css

>+#downloadsPanel {
>+  margin-top: -3px;
>+}

>--- a/toolkit/themes/osx/global/popup.css
>+++ b/toolkit/themes/osx/global/popup.css

> panel[type="arrow"] {
>+  margin-top: -2px;
>   -moz-appearance: none;
>   background: transparent;
>   transition: opacity 300ms;
> }

This will hinder users from closing the panel by re-clicking the same spot at the bottom of the anchor that responds to clicks to open the panel. We should probably avoid that. (This issue aside, the margin would also be wrong when the panel opened in a different direction.)

Comment 4

[Mike de Boer [:mikedeboer]]

Stephen, thanks for working on this!

Apart from Dão's concern mentioned above, I think we should look at this together to
1) split up the patch in reasonable chunks, for example to split off the toolkit part and the browser part.
2) think about possible regressions when making the panels opaque, like "Will it negatively affect text rendering?"

Let me know what you think!

Comment 5

[Markus Stange [:mstange]]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> "Will it negatively affect text rendering?"

Since bug 941095 has been backed out completely, making the panel transparent won't affect text rendering on OS X.

Comment 6

[Stephen Horlander [:shorlander]]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> Apart from Dão's concern mentioned above, I think we should look at this
> together to
> 1) split up the patch in reasonable chunks, for example to split off the
> toolkit part and the browser part.

Sure thing. How granular do you want it?

> 2) think about possible regressions when making the panels opaque, like
> "Will it negatively affect text rendering?"

As Markus mentions text rendering, on OS X at least, seems to be fine. Could affect legibility if it is too translucent. Could just defer transparency until bug 940797 is fixed.

Comment 7

[Mike de Boer [:mikedeboer]]

Stephen, I'd like to suggest the following chunks:

1) toolkit popup.css changes. (In fear of stating the obvious, but here goes!) You need to be aware that any change needs to be valid for each panel position. See https://developer.mozilla.org/en-US/docs/XUL/PopupGuide/Positioning for a complete reference and screenshots.
2) one patch for each panel that you touch.

Looking forward to it! :)

Comment 8

[Stephen Horlander [:shorlander]]

Attachment: change-arrow-size-panel-styling-p01.patch

Comment 9

[Stephen Horlander [:shorlander]]

Attachment: change-identityPanel-styling-p02.patch

Comment 10

[Stephen Horlander [:shorlander]]

Attachment: change-sharePanel-alignment-p03.patch

Comment 11

[Stephen Horlander [:shorlander]]

Attachment: change-downloadPanel-styling-and-alignment-p04.patch

Comment 12

[Stephen Horlander [:shorlander]]

Attachment: change-clickToPlayPanel-styling-p05.patch

Comment 13

[Stephen Horlander [:shorlander]]

Attachment: change-menuPanelSubview-styling-p06.patch

Split-up patches

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8372713 [details] [diff] [review]
change-arrow-size-panel-styling-p01.patch

I'm not a toolkit peer, so not eligeble to review this patch.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8372714 [details] [diff] [review]
change-identityPanel-styling-p02.patch

>+#identity-popup {
>+  margin-top: 1px;
>+}

Why this change?

The identity popup can also open above the anchor, by the way.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8372716 [details] [diff] [review]
change-downloadPanel-styling-and-alignment-p04.patch

>+#downloadsPanel {
>+  margin-top: -1px;
>+}

Seems like comment 3 will still be an issue here, even if to a lesser extent.

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8372716 [details] [diff] [review]
change-downloadPanel-styling-and-alignment-p04.patch

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

I agree with adjusting the borders, but not with the button styling. This needs more work and I think we need to tackle that in a separate bug that makes these buttons consistent across all platforms.

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8372719 [details] [diff] [review]
change-menuPanelSubview-styling-p06.patch

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

These styles defined in this patch also show a lot of overlap with the ones already defined in shared/panelUIOverlay.inc.css. To me they do not look OSX specific per sé... can you take a look which ones you can put in the shared section and which ones are truly OSX-specific?

Comment 19

[Stephen Horlander [:shorlander]]

(In reply to Mike de Boer [:mikedeboer] from comment #18)
> Comment on attachment 8372719 [details] [diff] [review]
> change-menuPanelSubview-styling-p06.patch
> 
> Review of attachment 8372719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These styles defined in this patch also show a lot of overlap with the ones
> already defined in shared/panelUIOverlay.inc.css. To me they do not look OSX
> specific per sé... can you take a look which ones you can put in the shared
> section and which ones are truly OSX-specific?

That is modifying the shared/panelUIOverlay.inc.css

Comment 20

[Mike de Boer [:mikedeboer]]

(In reply to Stephen Horlander [:shorlander] from comment #19)
> That is modifying the shared/panelUIOverlay.inc.css

Oh, bugger. I'll take another look...

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8372719 [details] [diff] [review]
change-menuPanelSubview-styling-p06.patch

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

This patch has bitrotted, unfortunately. Can you put up a new one?

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +557,5 @@
> +  background-color: hsla(210,4%,10%,.1);
> +  border-top: 1px solid hsla(210,4%,10%,.12);
> +}
> +
> +.subviewbutton.panel-subview-footer@buttonStateActive@ {

Can you leave the footer styles at it's current location and put the hover/ active states there too?

Comment 22

[Stephen Horlander [:shorlander]]

Attachment: change-menuPanelSubview-styling-p06.patch

Un-bitrotted.

(In reply to Mike de Boer [:mikedeboer] from comment #21)
> Can you leave the footer styles at it's current location and put the hover/
> active states there too?

I put it at the end of the other .subviewbutton rules because it was picking up those styles.

Comment 23

[Stephen Horlander [:shorlander]]

Attachment: change-downloadPanel-styling-and-alignment-p04.patch

Removed styling but left in border-radius and alignment.

Comment 24

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #15)
Sorry, totally missed these comments. Thanks for the reviews! :)

> Comment on attachment 8372714 [details] [diff] [review]
> Why this change?

So all the panels open at the same place.

> The identity popup can also open above the anchor, by the way.

True, I could make it side specific but the other orientation is going to be pretty rare.

Comment 25

[Dão Gottwald [:dao]]

(In reply to Stephen Horlander [:shorlander] from comment #24)
> > The identity popup can also open above the anchor, by the way.
> 
> True, I could make it side specific but the other orientation is going to be
> pretty rare.

We should still take care of it.

Comment 26

[Dão Gottwald [:dao]]

Also, see comment 16.

Comment 27

[Mike de Boer [:mikedeboer]]

Comment on attachment 8375270 [details] [diff] [review]
change-menuPanelSubview-styling-p06.patch

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

LGTM, thanks!

Comment 28

[Mike de Boer [:mikedeboer]]

Comment on attachment 8375271 [details] [diff] [review]
change-downloadPanel-styling-and-alignment-p04.patch

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

r=me, IF you can make peace with Dão about this negative margin ;-)

Comment 29

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #25)
> (In reply to Stephen Horlander [:shorlander] from comment #24)
> > > The identity popup can also open above the anchor, by the way.
> > 
> > True, I could make it side specific but the other orientation is going to be
> > pretty rare.
> 
> We should still take care of it.

So, after looking into this a bit, it appears that when the identity panel flips it is also flipping the margin-top. For example this is a setting with margin-top: 30px — http://people.mozilla.org/~shorlander/bugs/Bug-968595-panel-flip.png. I wasn't aware that it was supposed to work that way, so I am not sure if it is a bug.

(In reply to Dão Gottwald [:dao] from comment #3)
> This will hinder users from closing the panel by re-clicking the same spot
> at the bottom of the anchor that responds to clicks to open the panel. We
> should probably avoid that. (This issue aside, the margin would also be
> wrong when the panel opened in a different direction.)

We actually currently have this problem on all of the panels. There are about 3–4 pixels at the bottom of the button where you can activate a panel but not close it by clicking again. I don't think this will make it worse (although we could certainly fix this on all of the panels :)). Also I don't think it is too much of an issue given that you have to click low on the button and not jitter your mouse at all in between a rapidly opening and closing a panel.

Comment 30

[Dão Gottwald [:dao]]

(In reply to Stephen Horlander [:shorlander] from comment #29)
> So, after looking into this a bit, it appears that when the identity panel
> flips it is also flipping the margin-top. For example this is a setting with
> margin-top: 30px —
> http://people.mozilla.org/~shorlander/bugs/Bug-968595-panel-flip.png. I
> wasn't aware that it was supposed to work that way, so I am not sure if it
> is a bug.

Neil, can you explain this?

Comment 31

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #30)
> (In reply to Stephen Horlander [:shorlander] from comment #29)
> > So, after looking into this a bit, it appears that when the identity panel
> > flips it is also flipping the margin-top. For example this is a setting with
> > margin-top: 30px —
> > http://people.mozilla.org/~shorlander/bugs/Bug-968595-panel-flip.png. I
> > wasn't aware that it was supposed to work that way, so I am not sure if it
> > is a bug.
> 
> Neil, can you explain this?

FWIW, if/when there is an explanation for this, it also came by in 940379, so we'd need to adjust both if there is a separate bug here.

Comment 32

[Neil Deakin]

When the popup is opening with 'after_start', the top margin specifies how far away the popup is from the bottom edge of the anchor. When the popup gets flipped, the top margin is still used to specify how far away the popup will be from the top edge of the anchor.

The bottom margin is used when opening with 'before_start'.

I'm assuming you're expecting the top margin to apply when the popup is not flipped and the bottom margin to apply when the popup is flipped. To change this, a few margin references within would need to be adjusted in nsMenuPopupFrame::FlipOrResize, then lots of testing would occur.

Comment 33

[Stephen Horlander [:shorlander]]

Comment on attachment 8372713 [details] [diff] [review]
change-arrow-size-panel-styling-p01.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Existing panels
User impact if declined: Uglier panels
Risk to taking this patch (and alternatives if risky): Low risk style/image changes
String or IDL/UUID changes made by this patch: None

Comment 34

[Stephen Horlander [:shorlander]]

Comment on attachment 8372714 [details] [diff] [review]
change-identityPanel-styling-p02.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Existing panels
User impact if declined: Uglier panels
Risk to taking this patch (and alternatives if risky): Low risk style/image changes
String or IDL/UUID changes made by this patch: None

Comment 35

[Stephen Horlander [:shorlander]]

Comment on attachment 8372715 [details] [diff] [review]
change-sharePanel-alignment-p03.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Existing panels
User impact if declined: Uglier panels
Risk to taking this patch (and alternatives if risky): Low risk style/image changes
String or IDL/UUID changes made by this patch: None

Comment 36

[Stephen Horlander [:shorlander]]

Comment on attachment 8372718 [details] [diff] [review]
change-clickToPlayPanel-styling-p05.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Existing panels
User impact if declined: Uglier panels
Risk to taking this patch (and alternatives if risky): Low risk style/image changes
String or IDL/UUID changes made by this patch: None

Comment 37

[Stephen Horlander [:shorlander]]

Comment on attachment 8375270 [details] [diff] [review]
change-menuPanelSubview-styling-p06.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Existing panels
User impact if declined: Uglier panels
Risk to taking this patch (and alternatives if risky): Low risk style/image changes
String or IDL/UUID changes made by this patch: None

Comment 38

[Stephen Horlander [:shorlander]]

Comment on attachment 8375271 [details] [diff] [review]
change-downloadPanel-styling-and-alignment-p04.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Existing panels
User impact if declined: Uglier panels
Risk to taking this patch (and alternatives if risky): Low risk style/image changes
String or IDL/UUID changes made by this patch: None

Comment 39

[Mike de Boer [:mikedeboer]]

Stephen, your patches need to land on fx-team first, before you can get approval for aurora. Would you like me to do that?

Comment 40

[Mike de Boer [:mikedeboer]]

Missed checkin-needed flag, will do that now.

Comment 41

[Mike de Boer [:mikedeboer]]

Checked-in as:

remote:   https://hg.mozilla.org/integration/fx-team/rev/a040e19c30c1
remote:   https://hg.mozilla.org/integration/fx-team/rev/1cf01a7bd18b
remote:   https://hg.mozilla.org/integration/fx-team/rev/4ebb361218a1
remote:   https://hg.mozilla.org/integration/fx-team/rev/fa8c62aeff33
remote:   https://hg.mozilla.org/integration/fx-team/rev/87364536ec02
remote:   https://hg.mozilla.org/integration/fx-team/rev/5d90aed97e13

Comment 42

[Sylvestre Ledru [:Sylvestre]]

Stefen, I am going to uplift your patches once they have reached m-c.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a040e19c30c1
https://hg.mozilla.org/mozilla-central/rev/1cf01a7bd18b
https://hg.mozilla.org/mozilla-central/rev/4ebb361218a1
https://hg.mozilla.org/mozilla-central/rev/fa8c62aeff33
https://hg.mozilla.org/mozilla-central/rev/87364536ec02
https://hg.mozilla.org/mozilla-central/rev/5d90aed97e13

Comment 44

[Jose Fandos]

Attachment: "Bookmark this page" panel

Comment 45

[Jose Fandos]

Attachment: "Show your bookmarks" panel

Comment 46

[Jose Fandos]

There is a discrepancy in the latest nightly—18th Feb, and please let me know if this nightly is not meant to show the changes—with the "Bookmark this page" panel and the "Show your bookmarks" panel. See attached screenshots for reference.

Also, please let me know if a new bug would need to be filled and I'll follow up on that.

Comment 47

[:Gijs (he/him)]

(In reply to Jose Fandos from comment #46)
> There is a discrepancy in the latest nightly—18th Feb, and please let me
> know if this nightly is not meant to show the changes—with the "Bookmark
> this page" panel and the "Show your bookmarks" panel. See attached
> screenshots for reference.

I don't see anything amiss.

> Also, please let me know if a new bug would need to be filled

Yes. Please add more details there.

Comment 48

[Jose Fandos]

Attachment: Identity panel popping up

Comment 49

[Jose Fandos]

Attachment: Identity panel popping down

Comment 50

[:Gijs (he/him)]

(In reply to Jose Fandos from comment #49)
> Created attachment 8377570 [details]
> Identity panel popping down

Again, please file a followup bug. Adding stuff to this bug isn't helpful.

Comment 51

[Jose Fandos]

Sorry about that. I reopen bug 940379, since that is exactly what I reported here first and it is not fixed in latest nightly.

Comment 52

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/11690721732f
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/a1ddd968e1d3
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/bb9e1bcad0b7
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/e362f24fe667
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/e8eecab71e67
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/01c5b49eeaed

(In reply to Jose Fandos from comment #51)
> Sorry about that. I reopen bug 940379, since that is exactly what I reported
> here first and it is not fixed in latest nightly.

OK. Did you file a separate bug for comment 48 / comment 49 yet?

(You might as well note in it that this is also an issue for just opening a subview in the main menu panel)

Comment 53

[Jose Fandos]

(In reply to :Gijs Kruitbosch from comment #52)
> OK. Did you file a separate bug for comment 48 / comment 49 yet?
> 
> (You might as well note in it that this is also an issue for just opening a
> subview in the main menu panel)

Yes, added bug 974234.

Comment 54

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0

The OS X Arrow Panel Styling is updated according to the design on latest Aurora (build ID: 20140406004002) and on Firefox 29 beta 5 (build ID: 20140403132807).