Closed
Bug 1342229
Opened 7 years ago
Closed 7 years ago
Webcompat issues with perspective: 0
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: mikokm, Assigned: mikokm)
References
Details
(Keywords: regression)
Attachments
(3 files)
293 bytes,
text/html
|
Details | |
59 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
xidorn
:
review+
lizzard
:
approval-mozilla-beta+
ritu
:
approval-mozilla-esr52-
|
Details |
885 bytes,
text/html
|
Details |
The behavior with perspective: 0 was changed with bug 1274158, and again in bug 1316236. These changes introduced regressions that cause 3D transforms with perspective: 0 to render incorrectly. The attached testcase.html shows red square in Safari and Chrome. On FF >= 51 there is a blank page.
Updated•7 years ago
|
status-firefox51:
--- → wontfix
status-firefox52:
--- → fix-optional
status-firefox53:
--- → affected
status-firefox54:
--- → affected
Keywords: regression
Version: unspecified → 51 Branch
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Another site that is broken by this is http://www.kyp.my/ (hover over the "Read more" buttons)
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8840610 [details] Bug 1342229 - Handle perspective: 0 and perspective(0) similarly https://reviewboard.mozilla.org/r/115074/#review116580
Attachment #8840610 -
Flags: review?(matt.woodrow) → review+
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8840610 [details] Bug 1342229 - Handle perspective: 0 and perspective(0) similarly https://reviewboard.mozilla.org/r/115074/#review116584 For the issues below, r- for now. But I guess it's mostly fine. ::: layout/painting/nsDisplayList.cpp:6469 (Diff revision 1) > - Float perspectivePx = std::max(NSAppUnitsToFloatPixels(perspective, > - aAppUnitsPerPixel), > - std::numeric_limits<Float>::epsilon()); > + Float perspectivePx = NSAppUnitsToFloatPixels(perspective, aAppUnitsPerPixel); > + nsStyleTransformMatrix::ApplyPerspectiveToMatrix(aOutMatrix, perspectivePx); > + > - aOutMatrix._34 = -1.0 / perspectivePx; > aOutMatrix.ChangeBasis(perspectiveOrigin); This makes `perspective: 0` identical to `perspective: none`. I don't know if that is desired. But if other browsers do so, and there are webcompat issues... ok... ::: layout/painting/nsDisplayList.cpp:6895 (Diff revision 1) > + // Checking HasPerspective() is needed to handle perspective value 0 when > + // the transform is 2D. > if (!GetTransform().Is2D() || mFrame->Combines3DTransformWithAncestors() || > - mIsTransformSeparator) { > + mIsTransformSeparator || mFrame->HasPerspective()) { I don't understand why this is needed for handling perspective value 0. Your patch makes zero perspective an identity transform, so the transform stays being 2D with that. Why do we need to force an active layer in that case? ::: layout/reftests/transform-3d/perspective-zero-2-ref.html:3 (Diff revision 1) > +<!DOCTYPE html> > +<html> > + I think you can remove all blank lines in this file... It doesn't seem to me this file is complicated enough that needs to be divided to be sections... But it's more a personal style preference so it's up to you. I guess I can live with either way. ::: layout/reftests/transform-3d/perspective-zero-2-ref.html:8 (Diff revision 1) > + > +<head> > +<style type="text/css"> > + > +.parent { > + perspective: none; Please use whitespaces for indentation like elsewhere in this file. And preferrably two whitespaces rather than four. ::: layout/reftests/transform-3d/perspective-zero-2-ref.html:11 (Diff revision 1) > + > +.parent { > + perspective: none; > +} > + > +.child-2d { Another suggestion for this test is that, we usually have a test showing red for wrong behavior and green for correct behavior. This test shows both red and green for correct behavior, which is confusing. I suggest you add `position: absolute` to both children, so that in the correct case, there is no red on the screen. And it is probably better to move this test to layout/reftests/w3c-css/submitted/transforms/ (like my test in bug 1316236) so that it shares between browsers. ::: layout/reftests/transform-3d/perspective-zero-2.html:8 (Diff revision 1) > + > +<head> > +<style type="text/css"> > + > +.parent { > + perspective: 0px; Whitespaces.
Attachment #8840610 -
Flags: review?(xidorn+moz) → review-
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4) > This makes `perspective: 0` identical to `perspective: none`. I don't know > if that is desired. But if other browsers do so, and there are webcompat > issues... ok... It doesn't appear to match https://drafts.csswg.org/css-transforms-2/#perspective-property , so if it is desired, then a spec issue should be raised (or updated) if there isn't one already. And there's also https://drafts.csswg.org/css-transforms-2/#funcdef-perspective ... So https://github.com/w3c/csswg-drafts/issues/413 should be updated with information (precise enough for a spec) about what's needed for web compat.
Comment 6•7 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #5) > So https://github.com/w3c/csswg-drafts/issues/413 should be updated with > information (precise enough for a spec) about what's needed for web compat. I've posted a reply in that issue about this. Probably better also adjust its title?
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Summary: Regression with perspective: 0 handling → Webcompat issues with perspective: 0
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8840610 [details] Bug 1342229 - Handle perspective: 0 and perspective(0) similarly https://reviewboard.mozilla.org/r/115074/#review116584 Thank you for the review! > I don't understand why this is needed for handling perspective value 0. > > Your patch makes zero perspective an identity transform, so the transform stays being 2D with that. Why do we need to force an active layer in that case? FrameLayerBuilder seems to rely on perspective/3D transformed layers being active. If this is not forced, the visible area calculations for PaintedLayers will be incorrect. An alternative to forcing an active layer would be to change the visible area calculation logic, but since this case is rare, and the spec might still change, it is probably not worth doing. What are your thoughts?
Comment 9•7 years ago
|
||
Can we just not create the perspective item at all in this case? For opacity we have nsIFrame::HasOpacity() and nsIFrame::HasVisualOpacity. The former is used for checks that need to return true for all values (like deciding if we need to force a stacking context), and the latter is used for checks that want to know if the opacity is actually visible (like deciding if we need to create nsDisplayOpacity). We could do something similar here as I believe perspective:0 still need a stacking context, but we could treat it the same as perspective:none for drawing purposes.
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8840610 [details] Bug 1342229 - Handle perspective: 0 and perspective(0) similarly https://reviewboard.mozilla.org/r/115074/#review117606 ::: layout/painting/nsDisplayList.cpp:6495 (Diff revision 2) > if (perspective < 0) { > return true; > } I think the easiest way to fix is probably returning false here when `perspective < std::numeric_limits<Float>::epsilon()`. With this added, I suspect that you no longer need to create an active layer, and you can simplify the `perspectivePx` calculation below. ::: layout/reftests/w3c-css/submitted/transforms/perspective-zero-2.html:34 (Diff revision 2) > + transform: translateZ(1px); > +} > +</style> > +</head> > +<body> > +<p>Test passes if there is a green and a blue box below.</p> This test case becomes even worse than before. You are showing a green box in the failing case, which is completely counter-intuisive.
Attachment #8840610 -
Flags: review?(xidorn+moz) → review-
Comment 11•7 years ago
|
||
Please use this as a testcase. It shows red box when fail, and green box when pass.
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8840610 [details] Bug 1342229 - Handle perspective: 0 and perspective(0) similarly https://reviewboard.mozilla.org/r/115074/#review117606 > I think the easiest way to fix is probably returning false here when `perspective < std::numeric_limits<Float>::epsilon()`. With this added, I suspect that you no longer need to create an active layer, and you can simplify the `perspectivePx` calculation below. If you still need to create an active layer even if you do change here, I guess I can live with that. What Matt said in comment 9 can be a good optimization, but that can be done separately. If you want to do that here, you can probably investigate callers of `nsIFrame::HasPerspective()` and replace some of them with a new `HasVisualPerspective()`, otherwise you can file a new bug documenting this potential optimization.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10) > You are showing a green box > in the failing case, which is completely counter-intuisive. Initially I had meant to test both 2D and 3D transforms with perspective: 0 to catch regressions. However, now that these tests are aimed for w3c suite, it is better to limit the scope of the test, and I have used the testcase you added. (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #12) > > I think the easiest way to fix is probably returning false here when `perspective < std::numeric_limits<Float>::epsilon()`. With this added, I suspect that you no longer need to create an active layer, and you can simplify the `perspectivePx` calculation below. > > If you still need to create an active layer even if you do change here, I > guess I can live with that. I think this is a better solution anyways, since it avoids the (now) unnecessary operations for perspective-origin. I have made this change. Forcing an active layer is still required though, since the resulting transformation matrix does not change. > What Matt said in comment 9 can be a good optimization, but that can be done > separately. If you want to do that here, you can probably investigate > callers of `nsIFrame::HasPerspective()` and replace some of them with a new > `HasVisualPerspective()`, otherwise you can file a new bug documenting this > potential optimization. I will file a new bug for this. It would probably make sense to wait for the spec to change (regarding perspective property) before spending time on optimizing this?
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8840610 [details] Bug 1342229 - Handle perspective: 0 and perspective(0) similarly https://reviewboard.mozilla.org/r/115074/#review118082 Ok, r=me with the ref page fixed. ::: layout/reftests/w3c-css/submitted/transforms/perspective-zero-2-ref.html:8 (Diff revision 3) > +<style type="text/css"> > +.parent { > + transform: perspective(0px); > +} > +.parent > div { > + width: 200px; > + height: 200px; > + position: absolute; > +} > +.child-2d { > + background: red; > + transform: translateZ(0px); > +} > +.child-3d { > + background: green; > + transform: translateZ(1px); > +} > +</style> > +</head> > +<body> > +<p>Test passes if there is only green below.</p> > +<div class="parent"> > + <div class="child-2d"></div> > + <div class="child-3d"></div> > +</div> You can just leave a green box here rather than one green box with a invisible red box.
Attachment #8840610 -
Flags: review?(xidorn+moz) → review+
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #15) > You can just leave a green box here rather than one green box with a > invisible red box. Done. I also had to remove transform: translateZ(0px) from the red box on test page to get rid of (invisible) 1,16 reftest difference at least on OSX. I have verified that the behavior of the test did not change though.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cdc833d0028865e19966a92e827ecfccf078b94
Keywords: checkin-needed
Comment 19•7 years ago
|
||
(In reply to Miko Mynttinen [:miko] from comment #18) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=5cdc833d0028865e19966a92e827ecfccf078b94 You should probably avoid triggering all tests on all platforms. In general, a T-push, which means build on all platforms but run tests on one platform, should be enough. You can use the following try syntax next time: > try: -b do -p all -u all[x64] -t none[x64]
Comment 20•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e68a7bc9b7de Handle perspective: 0 and perspective(0) similarly r=mattwoodrow,xidorn
Keywords: checkin-needed
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e68a7bc9b7de
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 22•7 years ago
|
||
Too late for Fx52 at this point. Please request Beta approval on this when you get a chance.
Importing into the CSS test suite, I get: remote: vendor-imports/mozilla/mozilla-central-reftests/transforms/perspective-zero-2.html status changed to 'Needs Work' due to error: remote: Not linked to a tracked specification anchor.
Assignee | ||
Comment 24•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #22) > Too late for Fx52 at this point. Please request Beta approval on this when > you get a chance. To prevent having to roll back this change, we should probably wait with the uplift at least until the w3c draft covering the perspective property is updated. There has not yet been a definite answer to Xidorns question here: https://github.com/w3c/csswg-drafts/issues/413.
Flags: needinfo?(mikokm)
Comment 25•7 years ago
|
||
Should we wontfix this for 53 at this point? I'm honestly getting a bit worried about 54 at this point given the lack of reply to the thread in comment 24.
Flags: needinfo?(mikokm)
Assignee | ||
Comment 26•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #25) > Should we wontfix this for 53 at this point? I'm honestly getting a bit > worried about 54 at this point given the lack of reply to the thread in > comment 24. I think we can just go ahead with the uplift since this is pretty trivial code change and because this new behavior matches the other browsers.
Flags: needinfo?(mikokm)
Assignee | ||
Comment 27•7 years ago
|
||
Comment on attachment 8840610 [details] Bug 1342229 - Handle perspective: 0 and perspective(0) similarly Approval Request Comment [Feature/Bug causing the regression]: Behavior of perspective: 0px differs from the other browsers, causing webcompat issues. This behavior is currently subject to change in the spec draft. [User impact if declined]: Websites using perspective property with value 0 might have missing or corrupted elements. [Is this code covered by automated tests?]: Yes, this patch adds a reftest to verify that the behavior is consistent with the other browsers. [Has the fix been verified in Nightly?]: Yes, I have verified that the issue is fixed in Nightly. [Needs manual test from QE? If yes, steps to reproduce]: Optional. The fix can be verified by, for example, hovering over the orange "read more" buttons at http://www.kyp.my/. This should rotate the button. [List of other uplifts needed for the feature/fix]: N/A [Is the change risky?]: Low risk. [Why is the change risky/not risky?]: The code change is trivial, there is a reftest, and this feature is probably somewhat rare. [String changes made/needed]: N/A
Attachment #8840610 -
Flags: approval-mozilla-beta?
Comment 28•7 years ago
|
||
Comment on attachment 8840610 [details] Bug 1342229 - Handle perspective: 0 and perspective(0) similarly Getting us in line with other browsers seems sensible. Thanks for the new tests.
Attachment #8840610 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 29•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/8bb9ee1f4d82
Comment 30•7 years ago
|
||
Setting qe-verify- based on the fact that this fix implies a trivial code change (see Comment 27) that has automated coverage.
Flags: qe-verify-
Comment 31•7 years ago
|
||
Comment on attachment 8840610 [details] Bug 1342229 - Handle perspective: 0 and perspective(0) similarly This is a webcompat fix we're shipping in Fx53. Seems worth taking in ESR52 as well so we're not shipping a supported browser with the broken behavior for the next year.
Attachment #8840610 -
Flags: approval-mozilla-esr52?
Comment on attachment 8840610 [details] Bug 1342229 - Handle perspective: 0 and perspective(0) similarly Based on the comments in the bug, it seems this isn't a commonly used feature. ESR releases usually contain only security driven fixes and severe regressions. I don't believe this is a severe enough problem on ESR. I am willing to reconsider if the severity assessment changes.
Attachment #8840610 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
You need to log in
before you can comment on or make changes to this bug.
Description
•