Closed Bug 1303229 Opened 7 years ago Closed 6 years ago

stylo: Supply correct viewport size when restyling (affects viewport units)

Categories

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

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

Tracking

()

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

People

(Reporter: birtles, Assigned: emilio)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1303229: Get the proper viewport size for animations.
59 bytes, text/x-review-board-request
heycam
: review+
hiro
: review+
Details 
It seems that in restyle_subtree we pass a viewport size of 0x0 in the SharedStyleContext.[1]

From what I understand this is used for resolving viewport units (vw/vh/etc.).

It looks like PerDocumentStyleData contains a Stylist which contains ViewportConstraints which has a 'size' member, however I believe that size is based on viewport pixels and would need to be converted to application units. That's as far as I've looked.

In bug 1302949 I'm adding Servo_RestyleWithAddedDeclaration which will, likewise use a 0x0 viewport for now until we fix this.

[1] https://dxr.mozilla.org/servo/rev/e0e6a7be4fa00da2968acdb1d310379194a43390/ports/geckolib/glue.rs#94
Priority: -- → P2
Priority: P2 → P1
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Sorry for kinda-stealing this. I used this to test bug 1328652, and I thought it's worth to land. I tried to ping Manish on IRC, but given there was no response, I guess it was worth to leave it posted. I hope we haven't done too much duplicate work here.

That's the correct size at least until we properly support scrollbars.
Also, that patch leaves the animation place unchanged. We have the prescontext there, but instead of duplicating that I think we should manage to get the styleset to there. If it isn't we can always duplicate that logic there.
Manish said that me taking this bug was fine.
Assignee: manishearth → emilio+bugs
Comment on attachment 8846189 [details]
Bug 1303229: Get the proper viewport size for stylo.

https://reviewboard.mozilla.org/r/119292/#review121242

r=me with the print stuff.

::: servo/components/style/gecko/media_queries.rs:98
(Diff revision 1)
>      pub fn au_viewport_size(&self) -> Size2D<Au> {
>          self.viewport_override.as_ref().map(|v| {
>              Size2D::new(Au::from_f32_px(v.size.width),
>                          Au::from_f32_px(v.size.height))
> -        }).unwrap_or_else(|| {
> -            // TODO(emilio): Grab from pres context.
> +        }).unwrap_or_else(|| unsafe {
> +            // TODO(emilio): Need to take into account scrollbars.

How do we need to take into scrollbars?  Looking at http://searchfox.org/mozilla-central/rev/b035220d0f939559f4f8cf7b9079bc12f6adfc35/layout/style/nsMediaFeatures.cpp#80-91 it doesn't seem to do anything with scrollbars.

Also, we should probably duplicate GetSize's behaviour in returning the page size, when in print mode.
Attachment #8846189 - Flags: review?(cam) → review+
Comment on attachment 8846214 [details]
Bug 1303229: Get the proper viewport size for animations.

https://reviewboard.mozilla.org/r/119314/#review121244
Attachment #8846214 - Flags: review?(cam) → review+
Comment on attachment 8846214 [details]
Bug 1303229: Get the proper viewport size for animations.

https://reviewboard.mozilla.org/r/119314/#review121256

Wow! Thanks for doing this!
Attachment #8846214 - Flags: review?(hikezoe) → review+
Comment on attachment 8846189 [details]
Bug 1303229: Get the proper viewport size for stylo.

https://reviewboard.mozilla.org/r/119292/#review121242

> How do we need to take into scrollbars?  Looking at http://searchfox.org/mozilla-central/rev/b035220d0f939559f4f8cf7b9079bc12f6adfc35/layout/style/nsMediaFeatures.cpp#80-91 it doesn't seem to do anything with scrollbars.
> 
> Also, we should probably duplicate GetSize's behaviour in returning the page size, when in print mode.

Note that we handle _that_ size correctly already from nsMediaFeatures. This is about viewport units, which are here[1], and does query de frame tree to get scrollbar sizes.

[1]: http://searchfox.org/mozilla-central/rev/b035220d0f939559f4f8cf7b9079bc12f6adfc35/layout/style/nsRuleNode.cpp#398
Comment on attachment 8846189 [details]
Bug 1303229: Get the proper viewport size for stylo.

https://reviewboard.mozilla.org/r/119292/#review121242

> Note that we handle _that_ size correctly already from nsMediaFeatures. This is about viewport units, which are here[1], and does query de frame tree to get scrollbar sizes.
> 
> [1]: http://searchfox.org/mozilla-central/rev/b035220d0f939559f4f8cf7b9079bc12f6adfc35/layout/style/nsRuleNode.cpp#398

Aha, thanks!
Attachment #8846189 - Attachment is obsolete: true
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/578321c8644b
Get the proper viewport size for animations. r=heycam,hiro
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b2eb2db87c0f
followup: fixup stylo test expectations. r=me
https://hg.mozilla.org/mozilla-central/rev/578321c8644b
https://hg.mozilla.org/mozilla-central/rev/b2eb2db87c0f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.