Closed Bug 1316922 Opened 3 years ago Closed 3 years ago

Panels on macOS clip their contents to the wrong corner radius

Categories

(Add-on SDK Graveyard :: General, defect)

defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mstange, Assigned: mstange)

Details

Attachments

(3 files)

Attached image screenshot with patch
Comment on attachment 8809906 [details]
Bug 1316922 - Fix border radius of add-on sdk panels on macOS.

https://reviewboard.mozilla.org/r/92392/#review92426

::: addon-sdk/source/lib/sdk/panel/utils.js:258
(Diff revision 1)
>    frame.setAttribute("flex", 1);
>    frame.setAttribute("transparent", "transparent");
>    frame.setAttribute("autocompleteenabled", true);
>    frame.setAttribute("tooltip", "aHTMLTooltip");
>    if (platform === "darwin") {
> -    frame.style.borderRadius = "6px";
> +    frame.style.borderRadius = "var(--arrowpanel-border-radius)";

Please add a default value here, just so this doesn't blow up if the variable is removed and this use goes unnoticed.
Attachment #8809906 - Flags: review?(kmaglione+bmo) → review+
Is there a way to do that? I'm not aware of any.
Although, thinking about it again, it should probably be something like `calc(var(--arrowpanel-border-radius) + 1px)` to account for the padding.
(In reply to Markus Stange [:mstange] from comment #4)
> Is there a way to do that? I'm not aware of any.

Yes. Everything after the first comma in the var() function is used as the fallback value if the variable doesn't exist.
(In reply to Kris Maglione [:kmag] from comment #6)
> (In reply to Markus Stange [:mstange] from comment #4)
> > Is there a way to do that? I'm not aware of any.
> 
> Yes. Everything after the first comma in the var() function is used as the
> fallback value if the variable doesn't exist.

That's awesome! I didn't know about that.

(In reply to Kris Maglione [:kmag] from comment #5)
> Although, thinking about it again, it should probably be something like
> `calc(var(--arrowpanel-border-radius) + 1px)` to account for the padding.

This happens automatically. "border-radius" specifies the border radius outside the border, and the "inside border" and "inside padding" radii get automatically adjusted by the right amount by the browser.

We'd only have to do this if the iframe used margin instead of padding. But it would need to be "- 1px" instead of "+ 1px".
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/e01242f2d839
Fix border radius of add-on sdk panels on macOS. r=kmag
(In reply to Markus Stange [:mstange] from comment #7)
> (In reply to Kris Maglione [:kmag] from comment #5)
> > Although, thinking about it again, it should probably be something like
> > `calc(var(--arrowpanel-border-radius) + 1px)` to account for the padding.
> 
> This happens automatically. "border-radius" specifies the border radius
> outside the border, and the "inside border" and "inside padding" radii get
> automatically adjusted by the right amount by the browser.
> 
> We'd only have to do this if the iframe used margin instead of padding. But
> it would need to be "- 1px" instead of "+ 1px".

Sorry, you're right, I was thinking about this the wrong way. I still think it looks like the inner radius should be adjusted down slightly to match the outer, but given the crazy things the SDK does to the styling of arrow panels there, I'm not exactly sure what inner padding applies...

Anyway, I'm OK with it as is. It's a clear improvement over the previous behavior.
(In reply to Kris Maglione [:kmag] from comment #10)
> I'm not exactly sure what inner padding applies...

I think it's just an iframe element whose border box covers the whole panel, and which has no border but 1px of padding.
Iframe contents are clipped to the content-box of the iframe.

http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/layout/generic/nsSubDocumentFrame.cpp#425
http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/layout/base/DisplayListClipState.cpp#121
http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/layout/generic/nsFrame.cpp#1396
(In reply to Markus Stange [:mstange] from comment #11)
> (In reply to Kris Maglione [:kmag] from comment #10)
> > I'm not exactly sure what inner padding applies...
> 
> I think it's just an iframe element whose border box covers the whole panel,
> and which has no border but 1px of padding.
> Iframe contents are clipped to the content-box of the iframe.

Normally arrow panel contents have margins and/or padding:

http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/toolkit/themes/osx/global/popup.css#42-51

But the SDK tries to change that:

http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/addon-sdk/source/lib/sdk/panel/utils.js#219-222

And I'm not sure what the actual end result is, and I wouldn't trust myself to try to guess without looking at the final DOM.
https://hg.mozilla.org/mozilla-central/rev/e01242f2d839
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.