[Photon] overflow panel should have rounded corners (not square ones) on OS X

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Depends on 2 bugs, Blocks 3 bugs)

53 Branch
Firefox 57
Unspecified
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-structure])

Attachments

(2 attachments)

Assignee

Description

2 years ago
No description provided.
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-structure][triage] → [photon-structure]
Posted patch patch?Splinter Review
Something like this maybe?

The way this is fixed will have some impact on what I'm doing in bug 1348294.
Assignee

Comment 2

2 years ago
Why can't we just remove the border-radius rule on OS X? Is there some reason we need all the other bits of this patch (and especially the DOM structure changes...) for this particular change that I'm not seeing?
Flags: needinfo?(mstange)
Assignee

Updated

2 years ago
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
(In reply to :Gijs from comment #2)
> Why can't we just remove the border-radius rule on OS X?

I think the <panelview> element would then cover the rounded corners of the panel itself. The panelview has a background color and no border-radius at the moment.
If we additionally add a border-radius to the panelview elements, things will probably look ok.

And if we go ahead with bug 1374749, we don't even need to set a background on the panelviews any more, so then the transparent background of the arrow panel would be completely visible in all its translucent glory.

If we want to have nice transparency without bug 1374749, then things are a bit more complicated. My attempt to solve that problem was what led me to suggesting the approach in this patch, and I realize now that it's impossible to understand why I did what I did unless I explain what I had in mind for the transparency.
My plan was to give the panelview elements a -moz-appearance: -moz-mac-arrowpanel-inner; which would clear everything behind its rectangle and render a translucent panel background. That way, subviews will "hide" what's inside the panel behind them (by erasing it), and at the same time, the window behind the panel would be slightly visible through the panel's transparent background.
The problem with this approach is that we can only clear rectangles. If the panelview elements now render a slightly transparent background on top of the cleared rectangle, and this background has rounded corners, then the corners of the subview will look odd during the slide-in animation because the panel will have holes around the corner curve. Unfortunately I don't have a screenshot of this phenomenon at the moment. I'm not sure if my explanation makes sense without one, though.
The idea of my patch was then to find a way to make -moz-appearance: -moz-mac-arrowpanel-inner only apply on a rectangle that does not cover the very top and very bottom of the panel, so that it doesn't erase the corners, and so that it doesn't need to render rounded corners itself.

However, bug 1374749 would make this complexity unnecessary.
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #3)
> However, bug 1374749 would make this complexity unnecessary.

Bug 1374749 will not be appearing on our roadmap very soon, I suspect, because it requires considerable changes to the markup & dimensions precalc. I'm not sure we'll be able to pull that animation off without a sync reflow. It'll also be animating two elements at the same time, depending on the strategy taken; if so, the chance of frame drop is higher. If we can make sure to animate only the container, than it'll be fine probably.
All that needs some time to experiment and fiddle with.
Assignee

Comment 5

2 years ago
From talking to Aaron, it sounds like we do want to fix bug 1374749, so my understanding is we could take a simpler patch here. That said, we should probably get to 1374749 sooner rather than later.
Depends on: 1374749

Updated

2 years ago
Blocks: 1387512
Assignee

Comment 6

2 years ago
I fixed this in passing in https://hg.mozilla.org/integration/autoland/rev/862e0401146f because otherwise webextension tests complained, for everything except the overflow panel, because I got a bit lost trying to figure out why it was broken there, and in any case I had other fish to fry in that bug. I'm not sure why the corners are still square there off-hand, but either way I'll update the summary to be slightly more precise about what's left here.
Summary: [Photon] panels should have rounded corners (not square ones) on OS X. → [Photon] overflow panel should have rounded corners (not square ones) on OS X
Blocks: 925284
Assignee

Comment 8

2 years ago
Funny thing, fixing the overflow stuff here seems to also improve things for bug 1374509 for me.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Priority: P2 → P1

Comment 9

2 years ago
mozreview-review
Comment on attachment 8897390 [details]
Bug 1374315 - fix CSS overflow:hidden in overflow panel and page action panel to fix rounded corners,

https://reviewboard.mozilla.org/r/168706/#review174070
Attachment #8897390 - Flags: review?(mdeboer) → review+

Comment 10

2 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1e7bcce37cfc
fix CSS overflow:hidden in overflow panel and page action panel to fix rounded corners, r=mikedeboer

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1e7bcce37cfc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Comment 12

2 years ago
Verified on OSX.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
OS: Unspecified → Mac OS X
Backed out for frequently failing browser-chrome's browser_ext_browserAction_popup_resize.js (bug 1373507):

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

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=123306724&repo=autoland

17:43:58     INFO -  307 INFO TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Browser height should increase (1425 > 175) -
17:43:58     INFO -  308 INFO TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Window width should not change -
17:43:58     INFO -  309 INFO TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Window height should increase (1425 > 560) -
17:43:58     INFO -  Buffered messages finished
17:43:58    ERROR -  310 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Window height be less than the screen height (1425 < 1024) -
17:43:58     INFO -  Stack trace:
17:43:58     INFO -  chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:testPopupSize:255
Status: VERIFIED → REOPENED
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: FIXED → ---
Iteration: 57.2 - Aug 29 → ---
Flags: qe-verify+
Assignee

Comment 14

2 years ago
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #13)
> Backed out for frequently failing browser-chrome's
> browser_ext_browserAction_popup_resize.js (bug 1373507):
> 
> https://hg.mozilla.org/mozilla-central/rev/
> e365137fa61bfd729617ba1ebf9f1ed79facd1f2
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=123306724&repo=autoland
> 
> 17:43:58     INFO -  307 INFO TEST-PASS |
> browser/components/extensions/test/browser/
> browser_ext_browserAction_popup_resize.js | Browser height should increase
> (1425 > 175) -
> 17:43:58     INFO -  308 INFO TEST-PASS |
> browser/components/extensions/test/browser/
> browser_ext_browserAction_popup_resize.js | Window width should not change -
> 17:43:58     INFO -  309 INFO TEST-PASS |
> browser/components/extensions/test/browser/
> browser_ext_browserAction_popup_resize.js | Window height should increase
> (1425 > 560) -
> 17:43:58     INFO -  Buffered messages finished
> 17:43:58    ERROR -  310 INFO TEST-UNEXPECTED-FAIL |
> browser/components/extensions/test/browser/
> browser_ext_browserAction_popup_resize.js | Window height be less than the
> screen height (1425 < 1024) -
> 17:43:58     INFO -  Stack trace:
> 17:43:58     INFO - 
> chrome://mochitests/content/browser/browser/components/extensions/test/
> browser/browser_ext_browserAction_popup_resize.js:testPopupSize:255

Kris, any idea *why* this change would break that test often-but-not-always? :-\
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(kmaglione+bmo)
I can't say for sure without looking at it in detail, but it looks from the screenshot like the popup isn't immediately resizing to fit the view.

The way that we currently calculate the max height of the popup contents is we take the initial height of the view (since that was always the minimum height in the menu panel) and add the initial distance from the bottom of the panel to the bottom of the screen to it. If the panel hasn't resized to fit the view at the point where we take that measurement, then we'll think we have much more vertical screen space than we actually do.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1373507
Assignee

Comment 16

2 years ago
I can't reproduce the failures locally (on my win10 machine) at all, neither with debug or opt builds, so I am at a loss as to how to address the issue. Instead, I get timeouts because https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/head.js#165-170 gets stuck in an infinite loop because the width of the window vs. browser is off by one on my (hidpi) machine. Fixing that, the test just always passes, even when run as part of the directory, and even when run with --run-until-failure, despite failing on 40% of pushes when this patch was in the tree (so presumably once every 5-10 test runs given the different incarnations of win7/win8 opt/pgo).

Can we just disable this test? Do you think the failure actually indicates something is wrong with the patch, or just wrong with the test?
Flags: needinfo?(kmaglione+bmo)
Status: REOPENED → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
Assignee

Comment 17

2 years ago
Pinged Kris about this on IRC. I'll disable the height vs. screen check and file a followup to investigate re-enabling it. Self-ni so I go do this on Monday.
Flags: needinfo?(kmaglione+bmo) → needinfo?(gijskruitbosch+bugs)
Assignee

Updated

2 years ago
Depends on: 1396843
Comment hidden (mozreview-request)

Comment 19

2 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1559cabac7b6
fix CSS overflow:hidden in overflow panel and page action panel to fix rounded corners, r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/1559cabac7b6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Assignee

Updated

2 years ago
Flags: needinfo?(gijskruitbosch+bugs)
Tested this with today's nightly and the overflow panel has rounded corners on OSX 10.12.
Updating the bug as verified-fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.