Closed Bug 1347410 Opened 7 years ago Closed 6 years ago

stylo: something is weirdly wrong with viewport units

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: manishearth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1347410: stylo: disable @viewport ;
59 bytes, text/x-review-board-request
emilio
: review+
Details 
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: stylo
See Also: → 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
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: stylo-reftest
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)
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)
This can no longer be reproduced when I remove @viewport.
Assignee: emilio+bugs → manishearth
Status: NEW → ASSIGNED
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+
https://hg.mozilla.org/mozilla-central/rev/b9bb7e5e804f
Status: ASSIGNED → RESOLVED
Closed: 6 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.