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

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1303229: Get the proper viewport size for stylo.

At least until we support scrollbars properly, this size is going to be the
correct one. I've left a TODO to grab the proper one once we support it.

This allows to trivially test viewport units for now.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1303229: Get the proper viewport size for animations.

While we're here, don't re-recompute the default computed values, just grab them
from the device.

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Manish said that me taking this bug was fine.

Comment 7

[Cameron McCormack (:heycam)]

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.

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8846214 [details]
Bug 1303229: Get the proper viewport size for animations.

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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!

Comment 10

[Emilio Cobos Álvarez (:emilio)]

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 11

[Cameron McCormack (:heycam)]

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!

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8846214 [details]
Bug 1303229: Get the proper viewport size for animations.

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/578321c8644b
Get the proper viewport size for animations. r=heycam,hiro

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2eb2db87c0f
followup: fixup stylo test expectations. r=me

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/578321c8644b
https://hg.mozilla.org/mozilla-central/rev/b2eb2db87c0f