stylo: Restyle should not be triggered when viewport size changes while there is no media query rules

RESOLVED FIXED in Firefox 56

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

2 years ago
layout/style/test/test_media_queries_dynamic.html checks that, the restyle generation is not updated when viewport size changes but there is no media query rules, which currently fails in Stylo.

It is marked with bug 1357461 but that bug has been closed, thus file a new bug for that.
Yup, that's known[1]. I've been meant to fix it for a while already, so probably will take it in a bit, once I've gotten rid of all the other bugs.

[1]: http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/layout/base/nsPresContext.cpp#2128
(All the other bugs I have assigned, I mean)
Assignee

Comment 3

2 years ago
I guess I can try fixing this.
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Blocks: 1383992

Comment 9

2 years ago
mozreview-review
Comment on attachment 8889675 [details]
Bug 1382077 part 1 - Have StyleSet::MediumFeaturesChanged return nsRestyleHint rather than a bool.

https://reviewboard.mozilla.org/r/160708/#review166096
Attachment #8889675 - Flags: review?(cam) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8889676 [details]
Bug 1382077 part 2 - Move mUsesViewportUnits into nsStyleSet.

https://reviewboard.mozilla.org/r/160710/#review166090

::: layout/style/nsRuleNode.cpp:477
(Diff revision 1)
> -  aPresContext->SetUsesViewportUnits(true);
> +  // This is possible to be called on a Servo-styled document, from media
> +  // query evaluation outside stylesheets.

Nit: "It is possible for this to be called".
Attachment #8889676 - Flags: review?(cam) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8889677 [details]
Bug 1382077 part 3 - Record viewport unit usage and generate proper restyle hint.

https://reviewboard.mozilla.org/r/160712/#review166094

I notice viewport_rule.rs can call this function, and cause the bool to be set, but we don't use ViewportRule in stylo.  Maybe add a comment to ViewportConstraints::maybe_new near the au_viewport_size call to say if we ever start supporting ViewportRule in Gecko, we probably want to avoid that bool being set?

::: servo/ports/geckolib/glue.rs:878
(Diff revision 1)
>      //
>      // FIXME(emilio, bug 1369984): do the computation conditionally, to do it
>      // less often.
>      let mut data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
>  
> +    let was_viewport_units_used = data.stylist.device().used_viewport_size();

Nit: "were_viewport_units_used" (or just "viewport_units_used")

::: servo/ports/geckolib/glue.rs:880
(Diff revision 1)
>      // less often.
>      let mut data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
>  
> +    let was_viewport_units_used = data.stylist.device().used_viewport_size();
>      data.stylist.device_mut().reset_computed_values();
> -    data.stylist.media_features_change_changed_style(
> +    let rule_changed = data.stylist.media_features_change_changed_style(

Nit: maybe "rules_changed"?
Attachment #8889677 - Flags: review?(cam) → review+

Comment 12

2 years ago
mozreview-review
Comment on attachment 8889678 [details]
Bug 1382077 part 4 - Update test expectations.

https://reviewboard.mozilla.org/r/160714/#review166102
Attachment #8889678 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23c6326dd926
part 1 - Have StyleSet::MediumFeaturesChanged return nsRestyleHint rather than a bool. r=heycam
https://hg.mozilla.org/integration/autoland/rev/972821835d48
part 2 - Move mUsesViewportUnits into nsStyleSet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/d3f7744b4ec3
part 3 - Record viewport unit usage and generate proper restyle hint. r=heycam
https://hg.mozilla.org/integration/autoland/rev/333e1d30100d
part 4 - Update test expectations. r=heycam

Comment 19

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d41432f808de
Backed out changeset 333e1d30100d 
https://hg.mozilla.org/integration/autoland/rev/a95f9f156785
Backed out changeset d3f7744b4ec3 
https://hg.mozilla.org/integration/autoland/rev/d173f12089ac
Backed out changeset 972821835d48 
https://hg.mozilla.org/integration/autoland/rev/a34cb0f468bc
Backed out changeset 23c6326dd926 on request from xidorn

Comment 20

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5cf8525a1c18
part 1 - Have StyleSet::MediumFeaturesChanged return nsRestyleHint rather than a bool. r=heycam
https://hg.mozilla.org/integration/autoland/rev/fe2124146276
part 2 - Move mUsesViewportUnits into nsStyleSet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/2d7df7d3a30f
part 3 - Record viewport unit usage and generate proper restyle hint. r=heycam
https://hg.mozilla.org/integration/autoland/rev/0203a69a9e6d
part 4 - Update test expectations. r=heycam
Duplicate of this bug: 1347250
You need to log in before you can comment on or make changes to this bug.