Closed Bug 1382077 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(4 files)

Bug 1382077 part 1 - Have StyleSet::MediumFeaturesChanged return nsRestyleHint rather than a bool.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1382077 part 2 - Move mUsesViewportUnits into nsStyleSet.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1382077 part 3 - Record viewport unit usage and generate proper restyle hint.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1382077 part 4 - Update test expectations.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
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)
I guess I can try fixing this.
Assignee: nobody → xidorn+moz
Blocks: 1383992
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 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 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 on attachment 8889678 [details]
Bug 1382077 part 4 - Update test expectations.

https://reviewboard.mozilla.org/r/160714/#review166102
Attachment #8889678 - Flags: review?(cam) → review+
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
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
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.