Closed Bug 1342229 Opened 3 years ago Closed 3 years ago

Webcompat issues with perspective: 0

Categories

(Core :: Layout, defect)

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: miko, Assigned: miko)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files)

Attached file testcase.html
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.
Keywords: regression
Version: unspecified → 51 Branch
Another site that is broken by this is http://www.kyp.my/ (hover over the "Read more" buttons)
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 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.
(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?
Summary: Regression with perspective: 0 handling → Webcompat issues with perspective: 0
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?
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 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-
Attached file updated testcase
Please use this as a testcase. It shows red box when fail, and green box when pass.
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.
(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 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+
Blocks: 1343844
(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.
(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]
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
https://hg.mozilla.org/mozilla-central/rev/e68a7bc9b7de
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Too late for Fx52 at this point. Please request Beta approval on this when you get a chance.
Flags: needinfo?(mikokm)
Flags: in-testsuite+
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.
Depends on: 1345603
(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)
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)
(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)
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 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+
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 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.