Closed Bug 1488414 Opened Last year Closed Last year

Broken slider box on cdu-niedersachsen.de

Categories

(Core :: CSS Transitions and Animations, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: darkspirit, Assigned: emilio)

References

()

Details

(Keywords: nightly-community, regression)

Attachments

(4 files)

Attached video 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/
Flags: needinfo?(emilio)
Attached file 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?
Assignee: nobody → emilio
Flags: needinfo?(emilio) → needinfo?(bbirtles)
Attachment #9006258 - Attachment mime type: text/plain → text/html
(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 :)
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?
ni? -> r? :)
Flags: needinfo?(bbirtles)
Thanks a lot for the report as always Darkspirit :)
Comment on attachment 9006276 [details]
Use decomposition to interpolate matched perspective transform operations.

Brian Birtles (:birtles) has approved the revision.
Attachment #9006276 - Flags: review+
Component: CSS Parsing and Computation → CSS Transitions and Animations
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/28aa0be49808
Use decomposition to interpolate matched perspective transform operations. r=birtles
Keywords: leave-open
Comment on attachment 9006551 [details]
Add a test for perspective function interpolation.

Hiroyuki Ikezoe (:hiro) has approved the revision.
Attachment #9006551 - Flags: review+
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/a9fd0a6da9c1
Add a test for perspective function interpolation. r=hiro
Keywords: leave-open
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12862 for changes under testing/web-platform/tests
(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?
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/a9fd0a6da9c1
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: in-testsuite+
Emilio, is that safe to uplift in 63 Beta? Thanks
Flags: needinfo?(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
Flags: needinfo?(emilio)
Attachment #9006276 - Flags: approval-mozilla-beta?
(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 on attachment 9006276 [details]
Use decomposition to interpolate matched perspective transform operations.

Uplift approved for 63 beta 5
Attachment #9006276 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: needinfo?(sheriffs)
Clearing the ni. Thank you Aryx for uplifting the second patch.
Flags: needinfo?(sheriffs)
You need to log in before you can comment on or make changes to this bug.