1199050 - default_popups for browserActions should have rounded corners.

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Blake Winton (:bwinton) (:☕️)]

Currently they look like this: https://dl.dropboxusercontent.com/u/2301433/Firefox/WebExtensions/SquareCorner.png but we want them to look like the bookmark menu dropdown, or the Menu Panel.

Comment 1

[Blake Winton (:bwinton) (:☕️)]

I'll note that the second time I click the button, the corners appear to be rounded, but there is no content…
(I'll file another bug for that second bit.  ;)

Comment 2

[Tim Nguyen :ntim]

overflow: hidden; should fix the round corner.

Comment 3

[Markus Jaritz [:designakt] (UX)]

Applies on Mac. On Windows all have sharp corners.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Attachment: A WebExtension and the Menu panel.

Just a preview of a patch I have in progress…
How does it look to you, Markus?  :)

(I also lined it up with the Menu Panel, cause I was in that code anyways… ;)

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Attachment: MozReview Request: Bug 1199050 - Round off the corners of browser-extension-panel's content. ui-r=maritz, r=gijs

Bug 1199050 - Round off the corners of browser-extension-panel's content. ui-r=maritz, r=gijs

Comment 6

[Blake Winton (:bwinton) (:☕️)]

https://reviewboard.mozilla.org/r/22969/#review20495

(I haven't tested it on Windows or Linux yet, so I'm really looking for more of an f? than an r?, but if there are changes I can make to make it more likely to get the r+, I'ld love to hear them.  ;)

::: browser/themes/linux/browser.css:1941
(Diff revision 1)
>  #context-navigation > .menuitem-iconic > .menu-iconic-left {

Comment 7

[:Gijs (he/him)]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #1)
> I'll note that the second time I click the button, the corners appear to be
> rounded, but there is no content…
> (I'll file another bug for that second bit.  ;)

If this is an iframe, I'm fairly sure there's a bug on file...

Comment 8

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/22969/#review20505

::: browser/themes/linux/browser.css:1949
(Diff revision 1)
> +  margin: 1px;

Can you explain in more detail what this is fixing? I mean, as a code change this seems fine... just not entirely sure what the before/after is here and why we're changing it specifically for these panels.

::: browser/themes/windows/browser.css:2817
(Diff revision 1)
> +  border-radius: 3.5px;

Seems like mjaritz said they should be sharp corners on Windows?

Comment 9

[Tim Nguyen :ntim]

https://reviewboard.mozilla.org/r/22969/#review20513

I disagree with this approach, it's bringing unecessary code duplication. It would be better if we could only set the border-radius once, so it can easily be changed in the future. In fact, this patch is already creating inconsistencies, on Windows, we use sharp corners for Windows 8+ and a 4px radius for Windows 7 or lower.
You can try one of these 2 approaches http://jsfiddle.net/ntim/ypq3cqaq/ to avoid code duplication, or you could as well convert the border-radius to a CSS variable. Gijs might have a better idea though.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

(In reply to :Gijs Kruitbosch from comment #8)
> https://reviewboard.mozilla.org/r/22969/#review20505
> 
> ::: browser/themes/linux/browser.css:1949
> (Diff revision 1)
> > +  margin: 1px;
> Can you explain in more detail what this is fixing? I mean, as a code change
> this seems fine... just not entirely sure what the before/after is here and
> why we're changing it specifically for these panels.

It was to line it up with the Menu Panel.

> ::: browser/themes/windows/browser.css:2817
> (Diff revision 1)
> > +  border-radius: 3.5px;
> Seems like mjaritz said they should be sharp corners on Windows?

I'll be looking into Windows next, but I suspect it's currently like ntim said.  ;)

Actually, I might try the overflow:hidden, since that would also fix the Hello and Pocket panels…

Comment 11

[Markus Jaritz [:designakt] (UX)]

Comment on attachment 8677593 [details]
A WebExtension and the Menu panel.

Wow, looks perfectly aligned. Thanks.

Comment 12

[:Gijs (he/him)]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #10)
> (In reply to :Gijs Kruitbosch from comment #8)
> > https://reviewboard.mozilla.org/r/22969/#review20505
> > 
> > ::: browser/themes/linux/browser.css:1949
> > (Diff revision 1)
> > > +  margin: 1px;
> > Can you explain in more detail what this is fixing? I mean, as a code change
> > this seems fine... just not entirely sure what the before/after is here and
> > why we're changing it specifically for these panels.
> 
> It was to line it up with the Menu Panel.

None of the other panels line up with the menu panel, and it doesn't seem related to this bug particularly. Can we make this decision in bug 1206100 and friends, and fix all the panels to be aligned the same way? I don't know that styling them is what will do the right thing there, I think it might be fixing the anchor to be the icon instead of the button (or the other way around).

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8677598 [details]
MozReview Request: Bug 1199050 - Round off the corners of browser-extension-panel's content. ui-r=maritz, r=gijs

Bug 1199050 - Round off the corners of browser-extension-panel's content. ui-r=maritz, r=gijs

Comment 14

[Blake Winton (:bwinton) (:☕️)]

(In reply to :Gijs Kruitbosch (at OSCON, limited availability) from comment #12)
> > It was to line it up with the Menu Panel.
> None of the other panels line up with the menu panel, and it doesn't seem
> related to this bug particularly. Can we make this decision in bug 1206100
> and friends, and fix all the panels to be aligned the same way? I don't know
> that styling them is what will do the right thing there, I think it might be
> fixing the anchor to be the icon instead of the button (or the other way
> around).

Removed!  Can I get an r+ when you get some time, even though you're not accepting r?s, Gijs?  ;)
Also, Markus, I assume you're happy with pushing off the panel-positioning to the other bug?

Comment 15

[Markus Jaritz [:designakt] (UX)]

Yes, I can understand why it is not part of this bug.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

https://hg.mozilla.org/integration/fx-team/rev/c66c3620de8426242471640ad30e8b7a9f56ccd7
Bug 1199050 - Round off the corners of browser-extension-panel's content. ui-r=maritz, r=gijs

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 43.0a1 (2015-08-26) under Mac OS X 10.11.
Verified fixed on Firefox 48.0a1 (2016-03-21) using Mac OS X 10.11.