1488414 - Broken slider box on cdu-niedersachsen.de

Status: RESOLVED FIXED

(Core :: CSS Transitions and Animations, defect)

Description

[Darkspirit]

Attachment: 2018-09-04_15-04-46.mp4

Debian Testing, KDE, Xorg, GTX 1060

mozregression --good 2018-04-01 --bad 2018-09-03 -a https://cdu-niedersachsen.de/
> 12:13.56 INFO: Last good revision: 2d910b41bff477568a84ab3c65656cb744b2ae84
> 12:13.56 INFO: First bad revision: 5d477ed8d62de3ff678519f79fc3ac9f23bb91cc
> 12:13.56 INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2d910b41bff477568a84ab3c65656cb744b2ae84&tochange=5d477ed8d62de3ff678519f79fc3ac9f23bb91cc

> 5d477ed8d62d	Emilio Cobos Álvarez — Bug 1458715: Fix perspective interpolation. r=hiro

This issue looks different with WebRender.

Screencast:
last good (left): mozregression --repo autoland --launch 2d910b41bff477568a84ab3c65656cb744b2ae84 -a https://cdu-niedersachsen.de/
first bad (right): mozregression --repo autoland --launch 5d477ed8d62de3ff678519f79fc3ac9f23bb91cc -a https://cdu-niedersachsen.de/

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase

Play with the different #target rules for fun.

Blink's code for interpolation of perspective is here:

  https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/transforms/perspective_transform_operation.cc?l=51&rcl=5b4f4438ae7900d9f4deb532f130b7e59fa864ba

When interpolating from / to zero perspective they just return the old perspective. Which I guess makes sense, since zero is not really a sensible value for perspective.

Brian, do you know what's the spec situation here?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)
> When interpolating from / to zero perspective they just return the old
> perspective. Which I guess makes sense, since zero is not really a sensible
> value for perspective.

Actually they don'd do that, their algorithm makes sense... I think :)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Use decomposition to interpolate matched perspective transform operations.

Looks like this produces sensible results for interpolation with 0, though I'm
not really convinced about the results from, let's say, 1px to 2000px in the
attached test-case, I would've expected a linear interpolation from that to go
through normal length interpolation.

css-transforms-1 says:

  > Two transform functions with the same name and the same number of arguments
  > are interpolated numerically without a former conversion. The calculated
  > value will be of the same transform function type with the same number of
  > arguments.
  >
  > Special rules apply to <matrix()>.

Which is what we do... I was going to file a spec issue but turns out that it's
already addressed in css-transforms-2:

  https://drafts.csswg.org/css-transforms-2/#interpolation-of-transform-functions

Which says:

  > The transform functions <matrix()>, matrix3d() and perspective() get
  > converted into 4x4 matrices first and interpolated as defined in section
  > Interpolation of Matrices afterwards.

In case there's no test for this in WPT, what's the best way to test this?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

ni? -> r? :)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Thanks a lot for the report as always Darkspirit :)

Comment 6

[Brian Birtles (:birtles)]

Comment on attachment 9006276 [details]
Use decomposition to interpolate matched perspective transform operations.

Brian Birtles (:birtles) has approved the revision.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Add a test for perspective function interpolation.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28aa0be49808
Use decomposition to interpolate matched perspective transform operations. r=birtles

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/28aa0be49808

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9006551 [details]
Add a test for perspective function interpolation.

Hiroyuki Ikezoe (:hiro) has approved the revision.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9fd0a6da9c1
Add a test for perspective function interpolation. r=hiro

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12862 for changes under testing/web-platform/tests

Comment 13

[Darkspirit]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)
> Thanks a lot for the report as always Darkspirit :)

Thanks a lot for the fix as always Emilio! ;)


The second line of black text on white background jumps up some pixels at the end:
"Digitalisierung" (Masterplan Digitalisierung) and "Niedersachsen" (Start-Up Niedersachsen)
It does not happen with Chrome Dev 70. Is that a minor leftover, a different bug or expected?

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 15

[Andrei Ciure[:aciure]]

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

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 17

[Pascal Chevrel:pascalc]

Emilio, is that safe to uplift in 63 Beta? Thanks

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9006276 [details]
Use decomposition to interpolate matched perspective transform operations.



Approval Request Comment
[Feature/Bug causing the regression]: bug 1458715, kind of 
[User impact if declined]: See comment 0
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: The change reuses all our transform interpolation code and matches other browsers.
[String changes made/needed]: none

Comment 19

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #13)
> The second line of black text on white background jumps up some pixels at
> the end:
> "Digitalisierung" (Masterplan Digitalisierung) and "Niedersachsen" (Start-Up
> Niedersachsen)
> It does not happen with Chrome Dev 70. Is that a minor leftover, a different
> bug or expected?

I think that should be a different bug.

Comment 20

[Pascal Chevrel:pascalc]

Comment on attachment 9006276 [details]
Use decomposition to interpolate matched perspective transform operations.

Uplift approved for 63 beta 5

Comment 21

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aa1a35103016

Comment 22

[Emilio Cobos Álvarez (:emilio)]

The wrong patch was uplifted: https://hg.mozilla.org/mozilla-central/rev/28aa0be49808 is the one that should get uplifted (or both).

The new test will fail without that.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7f9b2799bcb3

Comment 24

[Eliza Balazs [:ebalazs_]]

Clearing the ni. Thank you Aryx for uplifting the second patch.