Closed Bug 1199050 Opened 5 years ago Closed 5 years ago

default_popups for browserActions should have rounded corners.

Categories

(WebExtensions :: Untriaged, defect, P2)

defect

Tracking

(firefox43 affected, firefox44 fixed, firefox48 verified)

VERIFIED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed
firefox48 --- verified

People

(Reporter: bwinton, Assigned: bwinton, Mentored)

References

Details

(Whiteboard: [webextension-polish][browserAction])

Attachments

(2 files)

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.
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.  ;)
overflow: hidden; should fix the round corner.
Applies on Mac. On Windows all have sharp corners.
Whiteboard: [webextension-polish] → [webextension-polish][browserAction]
Blocks: webext
Priority: -- → P2
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
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… ;)
Attachment #8677593 - Flags: feedback?(mjaritz)
Bug 1199050 - Round off the corners of browser-extension-panel's content. ui-r=maritz, r=gijs
Attachment #8677598 - Flags: review?(mjaritz)
Attachment #8677598 - Flags: review?(gijskruitbosch+bugs)
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 {
Attachment #8677598 - Flags: ui-review?(mjaritz)
Attachment #8677598 - Flags: review?(mjaritz)
Attachment #8677598 - Flags: review?(gijskruitbosch+bugs)
Attachment #8677598 - Flags: feedback?(gijskruitbosch+bugs)
(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...
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?
Attachment #8677598 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
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.
(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…
Flags: blocking-webextensions+
Comment on attachment 8677593 [details]
A WebExtension and the Menu panel.

Wow, looks perfectly aligned. Thanks.
Attachment #8677593 - Flags: feedback?(mjaritz) → feedback+
(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 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
Attachment #8677598 - Flags: feedback+
(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?
Flags: needinfo?(gijskruitbosch+bugs)
Yes, I can understand why it is not part of this bug.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8677598 - Flags: review+
Attachment #8677598 - Flags: ui-review?(mjaritz) → ui-review+
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
https://hg.mozilla.org/mozilla-central/rev/c66c3620de84
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.