Closed
Bug 1303229
Opened 9 years ago
Closed 8 years ago
stylo: Supply correct viewport size when restyling (affects viewport units)
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: birtles, Assigned: emilio)
References
Details
Attachments
(1 file, 1 obsolete file)
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
Updated•9 years ago
|
Priority: -- → P2
Updated•8 years ago
|
Priority: P2 → P1
Updated•8 years ago
|
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Comment 4•8 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
Manish said that me taking this bug was fine.
Assignee: manishearth → emilio+bugs
Comment 7•8 years ago
|
||
mozreview-review |
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 8•8 years ago
|
||
mozreview-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 9•8 years ago
|
||
mozreview-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+
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8846189 -
Attachment is obsolete: true
Comment 13•8 years ago
|
||
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/578321c8644b
Get the proper viewport size for animations. r=heycam,hiro
Comment 14•8 years ago
|
||
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b2eb2db87c0f
followup: fixup stylo test expectations. r=me
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/578321c8644b
https://hg.mozilla.org/mozilla-central/rev/b2eb2db87c0f
Status: ASSIGNED → RESOLVED
Closed: 8 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.
Description
•