stylo: something is weirdly wrong with viewport units

RESOLVED FIXED in Firefox 55



2 years ago
2 years ago


(Reporter: bzbarsky, Assigned: manishearth)


(Blocks: 2 bugs)

Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)



(1 attachment)

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)
Blocks: 1243581
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

Comment 3

2 years 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)

Comment 5

2 years 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)

Comment 6

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


2 years ago
Assignee: emilio+bugs → manishearth

Comment 9

2 years ago
Comment on attachment 8874183 [details]
Bug 1347410: stylo: disable @viewport ;

::: servo/components/style/gecko/
(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/
(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)
Last Resolved: 2 years 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.