stylo: something is weirdly wrong with viewport units

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: bz, Assigned: manishearth)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
I am trying to add a stylo reftest like so:

== text-indent-1a.html text-indent-1a.html

where the file is:

  <!DOCTYPE html>
  <iframe sandbox
          style="width: 400px"
          srcdoc="<canvas style='text-indent: 50vw; display: block'>Text</canvas>"></iframe>

This test fails in an opt build (in the sense that the stylo and gecko rendering do not match) but passes in a debug build.  The failing case looks like "50vw" computed to "0" when loading with stylo.

Loading this testcase outside the reftest harness shows the right thing.  So it's possible we're getting the correct viewport size to the subframe in an async fashion or something and don't actually have the right style/layout onload....
Flags: needinfo?(emilio+bugs)
(Reporter)

Updated

4 months ago
Blocks: 1243581
See Also: → bug 1346829
Assignee: nobody → emilio+bugs
Priority: -- → P1
Ryan, you've been poking at viewport units. Can you look at this?
Assignee: emilio+bugs → jryans
Flags: needinfo?(emilio+bugs)
Emilio says he still wants this one. ;-)
Assignee: jryans → emilio+bugs
(Assignee)

Comment 3

2 months ago
Seems to also be making layout/reftests/w3c-css/received/css-values-3/vh-support-atviewport.html fail. That test fails even when the @viewport is removed, so it's basically the 100vw / 100vh computing to zero.
Blocks: 1324348
I can't repro Comment 3 now (I could last week though), and the test case in comment 1 seems to be working AFAICT (it may be the case that the test-case was modified before landing).

Manish, can you confirm that layout/reftests/w3c-css/received/css-values-3/vh-support-atviewport.html shows the right thing without the @vieport rule?
Flags: needinfo?(manishearth)
(Assignee)

Comment 5

2 months ago
Nope, it shows a small green square on a red background, even when I remove the @viewport. I'm on yesterday's tip.
Flags: needinfo?(manishearth)
(Assignee)

Comment 6

2 months ago
This can no longer be reproduced when I remove @viewport.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Assignee: emilio+bugs → manishearth
Status: NEW → ASSIGNED

Comment 9

2 months ago
mozreview-review
Comment on attachment 8874183 [details]
Bug 1347410: stylo: disable @viewport ;

https://reviewboard.mozilla.org/r/145606/#review149524

::: servo/components/style/gecko/media_queries.rs:71
(Diff revision 2)
>  
>      /// Tells the device that a new viewport rule has been found, and stores the
>      /// relevant viewport constraints.
>      pub fn account_for_viewport_rule(&mut self,
>                                       constraints: &ViewportConstraints) {
> +        println!("ACCOUNTING");

Hmm... remove the printlns?

::: servo/components/style/values/computed/length.rs:32
(Diff revision 2)
>              specified::NoCalcLength::FontRelative(length) =>
>                  length.to_computed_value(context, FontBaseSize::CurrentStyle),
> -            specified::NoCalcLength::ViewportPercentage(length) =>
> -                length.to_computed_value(context.viewport_size()),
> +            specified::NoCalcLength::ViewportPercentage(length) => {
> +                let a = length.to_computed_value(context.viewport_size());
> +
> +                println!("VP {:?} {:?} {:?}", length, context.viewport_size(), a);

Ditto, revert all this.
Attachment #8874183 - Flags: review?(emilio+bugs) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 months ago
https://github.com/servo/servo/pull/17153

Comment 12

2 months ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b9bb7e5e804f
stylo: disable @viewport ; r=emilio
https://hg.mozilla.org/mozilla-central/rev/b9bb7e5e804f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.