1388216 - stylo: Rendering of afr.com differs between stylo enabled and disabled nightly

Status: RESOLVED FIXED

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

Description

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: AssertStackTrace.txt

http://www.afr.com/ displays differently in nightly based on stylo being toggled on or off. When toggled on the main.main element is styled with NaN dimensions. In a debug build this results in hitting an assert (see attached stack trace).

Comment 1

[Astley Chen (inactive)]

Rank #243 in Australia.

Comment 2

[Cameron McCormack (:heycam)]

Thanks for pinpointing the issue, Bryce.

The specified value of main.main's transform is rotate3d(0, 0, 0, 0), and the conversion to a computed value here:

  http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/servo/components/style/properties/longhand/box.mako.rs#1414-1421

is dividing by zero.  Boris I think you were looking at 3D transforms at one point, do you want to look at fixing this?

Comment 3

[Boris Chiou [:boris]]

(In reply to Cameron McCormack (:heycam) from comment #2)
> Thanks for pinpointing the issue, Bryce.
> 
> The specified value of main.main's transform is rotate3d(0, 0, 0, 0), and
> the conversion to a computed value here:
> 
>  
> http://searchfox.org/mozilla-central/rev/
> bd39b6170f04afeefc751a23bb04e18bbd10352b/servo/components/style/properties/
> longhand/box.mako.rs#1414-1421
> 
> is dividing by zero.  Boris I think you were looking at 3D transforms at one
> point, do you want to look at fixing this?

OK. Interesting, it uses (0,0,0) as the direction vector.

Comment 4

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #3)
> OK. Interesting, it uses (0,0,0) as the direction vector.
s/direction/rotate axis/

Comment 5

[Boris Chiou [:boris]]

Attachment: Bug 1388216 - Don't apply the rotation if it cannot be normalized.

In Gecko, we do nothing while computing values, and do normalization in
ProcessRotate3D(). If the direction cannot be normalized, we treat it as
an identity matrix.
However, In Servo, we do normalization in to_computed_value(), and looks
like we are trying to normalize any kind of direction vector, so according
to the spec, we don't apply the rotation whose direction vector cannot be
normalized.

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

Comment 6

[Boris Chiou [:boris]]

Attachment: Bug 1388216 - Add a rotate3d whose direction vector cannot be normalized into property_database.js.

Make sure it doesn't cause assertions.

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

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8894823 [details]
Bug 1388216 - Don't apply the rotation if it cannot be normalized.

https://reviewboard.mozilla.org/r/165950/#review171106

::: servo/components/style/properties/longhand/box.mako.rs:1420
(Diff revision 1)
> -                        result.push(computed_value::ComputedOperation::Rotate(ax / len, ay / len, az / len, theta));
> +                        // https://www.w3.org/TR/css-transforms-1/#funcdef-rotate3d
> +                        // Spec: A direction vector that cannot be normalized, such as [0,0,0],
> +                        // will cause the rotation to not be applied. Therefore, we don't push this
> +                        // transform operation into the result.
> +                        if len > 0. {

Yeah, I think it's wrong that Servo is doing the normalization here, since the spec says that the computed value of transform should just be the specified value with units made absolute.

(Although Gecko and other browsers just return a matrix() or matrix3d() from getComputedStyle, so I'm not sure how accurate the spec is.)

So do you think it could make sense to just append the value without normalizing here?  Then Gecko should still skip this item in ProcessRotate3D.

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8894824 [details]
Bug 1388216 - Add a rotate3d whose direction vector cannot be normalized into property_database.js.

https://reviewboard.mozilla.org/r/165980/#review171108

Comment 9

[Boris Chiou [:boris]]

Comment on attachment 8894823 [details]
Bug 1388216 - Don't apply the rotation if it cannot be normalized.

https://reviewboard.mozilla.org/r/165950/#review171106

> Yeah, I think it's wrong that Servo is doing the normalization here, since the spec says that the computed value of transform should just be the specified value with units made absolute.
> 
> (Although Gecko and other browsers just return a matrix() or matrix3d() from getComputedStyle, so I'm not sure how accurate the spec is.)
> 
> So do you think it could make sense to just append the value without normalizing here?  Then Gecko should still skip this item in ProcessRotate3D.

Yes. It makes sense and the behavior would be much closer to that in Gecko. I will update this. Thanks.

Comment 10

[Boris Chiou [:boris]]

Comment on attachment 8894823 [details]
Bug 1388216 - Don't apply the rotation if it cannot be normalized.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165950/diff/1-2/

Comment 11

[Boris Chiou [:boris]]

Comment on attachment 8894824 [details]
Bug 1388216 - Add a rotate3d whose direction vector cannot be normalized into property_database.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165980/diff/1-2/

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8894823 [details]
Bug 1388216 - Don't apply the rotation if it cannot be normalized.

https://reviewboard.mozilla.org/r/165950/#review171512

Looks good, thanks.

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8894824 [details]
Bug 1388216 - Add a rotate3d whose direction vector cannot be normalized into property_database.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165980/diff/2-3/

Comment 14

[Boris Chiou [:boris]]

Attachment: Servo PR, #18016

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b04b798e6b2
Add a rotate3d whose direction vector cannot be normalized into property_database.js. r=heycam

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b04b798e6b2

Comment 17

[Darkspirit]

(In reply to Boris Chiou [:boris] from comment #14)
> Created attachment 8895200 [details] [review]
> Servo PR, #18016

Seen fixed in Nightly 57 x64 20170810100255 @ Debian Testing.
It's no longer white where you would expect the site's main content (a list of articles).

Broken in Nightly 57 x64 20170808114032 @ Windows 7,
but fixed in Nightly 57 x64 20170809100326 @ Windows 7.

Comment 18

[Gerry Chang [:gchang]]

Too late for 56. Mass won't fix for 56.