Closed
Bug 1374315
Opened 7 years ago
Closed 7 years ago
[Photon] overflow panel should have rounded corners (not square ones) on OS X
Categories
(Firefox :: Theme, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: Gijs, Assigned: Gijs)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [photon-structure])
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-review-board-request
|
mikedeboer
:
review+
|
Details |
No description provided.
Updated•7 years ago
|
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-structure][triage] → [photon-structure]
Comment 1•7 years ago
|
||
Something like this maybe?
The way this is fixed will have some impact on what I'm doing in bug 1348294.
Assignee | ||
Comment 2•7 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•7 years ago
|
Flags: qe-verify? → qe-verify+
Updated•7 years ago
|
QA Contact: gwimberly
Comment 3•7 years ago
|
||
(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)
Comment 4•7 years ago
|
||
(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•7 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
Assignee | ||
Comment 6•7 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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 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•7 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•7 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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 12•7 years ago
|
||
Verified on OSX.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
OS: Unspecified → Mac OS X
Comment 13•7 years ago
|
||
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 → ---
Updated•7 years ago
|
Iteration: 57.2 - Aug 29 → ---
Flags: qe-verify+
Assignee | ||
Comment 14•7 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)
Comment 15•7 years ago
|
||
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)
Updated•7 years ago
|
status-firefox57:
verified → ---
Assignee | ||
Comment 16•7 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)
Updated•7 years ago
|
Status: REOPENED → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Updated•7 years ago
|
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
Assignee | ||
Comment 17•7 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)
Comment hidden (mozreview-request) |
Comment 19•7 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
Comment 20•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 21•7 years ago
|
||
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
Updated•7 years ago
|
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•