1381196 - stylo: Yelp's thumbnail images shake and rotate when mousing over then instead of

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Load a Yelp page, such as https://www.yelp.com/biz/golden-gate-bridge-san-francisco
2. Mouse over the photo gallery's thumbnail images.

EXPECTED RESULT:
When Stylo is not enabled, the thumbnail images grow larger.

ACTUAL RESULT:
When Stylo is enabled, the thumbnail images shake and rotate.

Comment 1

[Jeremy Chen ([e-mail])]

Inspector shows that the shake and rotate comes from these 3 elements:
  <div class="js-photo photo photo-1" data-ga-label="left_photo" data-media-id="FhUCjr71X_XB9sGQc-xEcA" data-media-index="1">
  <div class="js-photo photo photo-2" data-ga-label="middle_photo" data-media-id="PbumJneeHsIs1FImU6yQzg" data-media-index="0">
  <div class="js-photo photo photo-3 photo-grid" data-ga-label="right_photo" data-media-id="en99IZJdTCA-BsbHzyHoAA" data-media-index="2">

... all these 3 have following styles applied:
==========
.photo {
  z-index:1000;
  display:block;
  position:absolute;
  margin:-15px;
  width:250px;
  height:250px;
  -webkit-box-shadow:none;
  box-shadow:none;
  -webkit-transition:all 0.2s ease-out;
  transition:all 0.2s ease-out;
  -webkit-transition-property:-webkit-transform, box-shadow;
  transition-property:transform, box-shadow;
  -webkit-transform:scale(.88);
  -moz-transform:scale(.88);
  -ms-transform:scale(.88);
  transform:scale(.88);
  -webkit-transform:scale(.88) translateZ(0);
  -moz-transform:scale(.88) translateZ(0);
  -ms-transform:scale(.88) translateZ(0);
  transform:scale(.88) translateZ(0)
}
[...]
.photo:hover {
  z-index:1001;
  -webkit-box-shadow:0 0 25px -3px rgba(0,0,0,0.5);
  box-shadow:0 0 25px -3px rgba(0,0,0,0.5);
  -webkit-transform:none;
  -moz-transform:none;
  -ms-transform:none;
  transform:none;
  -webkit-transform:translateZ(1px);
  -moz-transform:translateZ(1px);
  -ms-transform:translateZ(1px);
  transform:translateZ(1px)
}
==========

I try to remove the setting of "transform:translateZ(1px)" to make the transition only from scale(0.88) to scale(1), then the issue is gone. So, I suspect the shake and rotate effect comes from a transition effect from translateZ(0) to translateZ(1px).

Comment 2

[Jeremy Chen ([e-mail])]

Attachment: reduced testcase

STR:
1. Try to move mouse near the boundary of the blue rectangle
2. Move mouse over the blue rectangle
3. Move mouse out of the blue rectangle
4. Repeat 2 & 3 back and forth, then an unexpected rotate is shown

It seems that a z-index change is also essential to reproduce the issue.

Comment 3

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #2)
> 
> It seems that a z-index change is also essential to reproduce the issue.

No, it is NOT essential to set z-index.
Extend the transition duration to 2s to make it even more obvious, the issue still remains even if the z-index setting is removed.

Comment 4

[Jeremy Chen ([e-mail])]

Ok, I know what's going on here.

We read the transform into matrices correctly for both gecko and stylo, until we get to [1]. In stylo, we delegate matrix decomposition/interpolation/recomposition to Servo. And in decompose_3d_matrix function [2], we accidentally use the wrong value to normalize the 2nd row of the decomposed matrix, which then would cause the unexpected interpolation result in this bug. I have a working patch that can fix this issue on my local machine.


[1] https://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/layout/style/nsStyleTransformMatrix.cpp#517-525
[2] https://searchfox.org/mozilla-central/source/servo/components/style/properties/helpers/animated_properties.mako.rs#2113

Comment 5

[Jeremy Chen ([e-mail])]

Attachment: Bug 1381196 - stylo: fix Y scale computation while decomposing a 3D matrix.

While decomposing a 3D matrix, we should normalize the 2nd row right after the
Y scale computation. However, we accidentally use the length of the 1st row to
do the normalization. This causes the wrong Scale3D function while decomposing,
and then leads to the wrong decomposed 3D matrix.

Here, we correct it by using the right value (the length of the 2nd row).

Review commit: https://reviewboard.mozilla.org/r/158778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158778/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8887857 [details]
Bug 1381196 - stylo: fix Y scale computation while decomposing a 3D matrix.

https://reviewboard.mozilla.org/r/158778/#review164064

Good catch! We might need length method for Matrix's row in future somehow.

Anyway this needs an automation test.

Comment 7

[Jeremy Chen ([e-mail])]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> 
> Anyway this needs an automation test.
>

Per discussion w/ Hiroyuki on IRC, it might be better to land the fix sooner, so we can provide better experience for stylo nightly users. Filed Bug 1382517 as a followup to add the tests.

servo PR: https://github.com/servo/servo/pull/17789

Comment 8

[Jeremy Chen ([e-mail])]

PR merged to autoland: https://hg.mozilla.org/integration/autoland/rev/eb19a789a455

Comment 9

[Jeremy Chen ([e-mail])]

merge to m-c: https://hg.mozilla.org/mozilla-central/rev/eb19a789a455

Comment 10

[Darkspirit]

Verified fixed in Nightly 56 x64 20170721100159 @ Debian Testing (Linux 4.11.0-1-amd64, Radeon RX480)