1396045 - stylo: Round down when dealing with relative units

Status: RESOLVED FIXED

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

Description

[Manish Goregaokar [:manishearth]]

./layout/reftests/pixel-rounding/viewport-units-rounding-1.html

Gecko rounds viewport units downwards in case someone does something like five adjacent elements of width 20vw and it overflows. We should do the same.

Comment 1

[Manish Goregaokar [:manishearth]]

Needs https://github.com/servo/app_units/pull/34

Comment 2

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1396045 - stylo: Update app_units ;

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

Comment 3

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1396045 - stylo: Round down when computing viewport units;

See bug 989802

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

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8903811 [details]
Bug 1396045 - stylo: Update app_units ;

https://reviewboard.mozilla.org/r/175566/#review180666

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8903812 [details]
Bug 1396045 - stylo: Round down when computing viewport units;

https://reviewboard.mozilla.org/r/175568/#review180668

r=me, assuming it's tested.

::: servo/components/style/values/specified/length.rs:216
(Diff revision 1)
>              ViewportPercentageLength::Vmin(length) =>
> -                length * to_unit!(cmp::min(viewport_size.width, viewport_size.height)),
> +                (length, cmp::min(viewport_size.width, viewport_size.height)),
>              ViewportPercentageLength::Vmax(length) =>
> -                length * to_unit!(cmp::max(viewport_size.width, viewport_size.height)),
> +                (length, cmp::max(viewport_size.width, viewport_size.height)),
>          };
> -        Au::from_f32_px(value)
> +        

trailing whitespace.

Comment 6

[Manish Goregaokar [:manishearth]]

This makes viewport-units-rounding-2.html fail, but only by a tiny bit :|

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8903811 [details]
Bug 1396045 - stylo: Update app_units ;

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

Comment 8

[Manish Goregaokar [:manishearth]]

Comment on attachment 8903812 [details]
Bug 1396045 - stylo: Round down when computing viewport units;

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

Comment 9

[Manish Goregaokar [:manishearth]]

The failure was due to rounding instead of truncating. Fixed.

Comment 10

[Manish Goregaokar [:manishearth]]

The test is still fuzzy because of differing rounding between stylo and gecko, but this isn't the important rounding (it used to fail because the divs would overflow the container)

Comment 11

[Manish Goregaokar [:manishearth]]

https://github.com/servo/servo/pull/18448

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4247bc5eed01
stylo: Round down when computing viewport units; r=emilio

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/4247bc5eed01