Closed Bug 1380224 Opened 7 years ago Closed 7 years ago

stylo: Microsoft Edge "Custom Properties Pooch" demo does not switch background image when Stylo is enabled

Categories

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

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

Tracking

()

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

People

(Reporter: cpeterson, Assigned: heycam)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(3 files, 2 obsolete files)

reduced test
230 bytes, text/html
Details
Bug 1380224 - Part 2: Reftest.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1380224 - Part 1: Check custom properties for differences in Servo-backed style contexts.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
STR:
1. Load https://developer.microsoft.com/en-us/microsoft-edge/testdrive/demos/custom-props/
2. You can ignore the warning about calc() values and the missing images because they also affect Firefox without Stylo.
3. Click the nighttime and daytime environment pictures.

RESULT:
The picture's background switches between nighttime and daytime when Stylo is disabled but not when it is enabled.
Click on nighttime, then zoom in (Ctrl +). Only then the background does change.
Has STR: --- → yes
Version: unspecified → Trunk
Attached file reduced test — 
I'm pretty sure it's something to do with thinking we have no style changes and so we cut off cascading.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8886037 - Flags: review?(emilio+bugs)
Comment on attachment 8886037 [details]
style: Check custom properties for changes when computing damage.

https://reviewboard.mozilla.org/r/156824/#review161944

::: servo/components/style/matching.rs:774
(Diff revision 1)
>          new_values: &Arc<ComputedValues>,
>          pseudo: Option<&PseudoElement>
>      ) -> StyleDifference {
>          debug_assert!(pseudo.map_or(true, |p| p.is_eager()));
>          if let Some(source) = self.existing_style_for_restyle_damage(old_values, pseudo) {
> -            return RestyleDamage::compute_style_difference(source, new_values)
> +            let mut difference = RestyleDamage::compute_style_difference(source, new_values);

I think we should handle it there though... This is essentially a dupe of bug 1375536, isn't it?

::: servo/components/style/matching.rs:777
(Diff revision 1)
>          debug_assert!(pseudo.map_or(true, |p| p.is_eager()));
>          if let Some(source) = self.existing_style_for_restyle_damage(old_values, pseudo) {
> -            return RestyleDamage::compute_style_difference(source, new_values)
> +            let mut difference = RestyleDamage::compute_style_difference(source, new_values);
> +            // RestyleDamage::compute_style_difference doesn't take into account
> +            // custom properties, so do that here.
> +            if difference.change == StyleChange::Unchanged &&

This feels like a one-off, and it should really be handled there... I think fixing that shouldn't be that hard, what do you think?
Comment on attachment 8886038 [details]
Bug 1380224 - Part 2: Reftest.

https://reviewboard.mozilla.org/r/156826/#review161946
Attachment #8886038 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886037 [details]
style: Check custom properties for changes when computing damage.

https://reviewboard.mozilla.org/r/156824/#review161950

::: servo/components/style/matching.rs:774
(Diff revision 1)
>          new_values: &Arc<ComputedValues>,
>          pseudo: Option<&PseudoElement>
>      ) -> StyleDifference {
>          debug_assert!(pseudo.map_or(true, |p| p.is_eager()));
>          if let Some(source) = self.existing_style_for_restyle_damage(old_values, pseudo) {
> -            return RestyleDamage::compute_style_difference(source, new_values)
> +            let mut difference = RestyleDamage::compute_style_difference(source, new_values);

Well, RestyleDamage::compute_style_difference takes an nsStyleContext for its first argument.  Should we grab the ComputedValues out through there?
Comment on attachment 8886037 [details]
style: Check custom properties for changes when computing damage.

https://reviewboard.mozilla.org/r/156824/#review161950

> Well, RestyleDamage::compute_style_difference takes an nsStyleContext for its first argument.  Should we grab the ComputedValues out through there?

I meant handling it in nsStyleContext::ComputeStyleDifference. If IsServo(), call something like Servo_ComputedValues_VariablesAreDifferent(ComputedValues(), aNewContext.ComputedValues())
Comment on attachment 8886037 [details]
style: Check custom properties for changes when computing damage.

https://reviewboard.mozilla.org/r/156824/#review161948

::: servo/components/style/matching.rs:774
(Diff revision 1)
>          new_values: &Arc<ComputedValues>,
>          pseudo: Option<&PseudoElement>
>      ) -> StyleDifference {
>          debug_assert!(pseudo.map_or(true, |p| p.is_eager()));
>          if let Some(source) = self.existing_style_for_restyle_damage(old_values, pseudo) {
> -            return RestyleDamage::compute_style_difference(source, new_values)
> +            let mut difference = RestyleDamage::compute_style_difference(source, new_values);

(Oh, and servo does handle it there, afaict)
Comment on attachment 8886037 [details]
style: Check custom properties for changes when computing damage.

https://reviewboard.mozilla.org/r/156824/#review161948

> (Oh, and servo does handle it there, afaict)

Err, outdated review comment :)
Comment on attachment 8886037 [details]
style: Check custom properties for changes when computing damage.

https://reviewboard.mozilla.org/r/156824/#review161950

> I meant handling it in nsStyleContext::ComputeStyleDifference. If IsServo(), call something like Servo_ComputedValues_VariablesAreDifferent(ComputedValues(), aNewContext.ComputedValues())

OK, can do that.
Attachment #8886037 - Flags: review?(emilio+bugs)
Attachment #8886087 - Flags: review?(emilio+bugs)
Comment on attachment 8886087 [details]
style: Add FFI function to compare ComputedValues for custom property differences.

https://reviewboard.mozilla.org/r/156892/#review162008
Attachment #8886087 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886088 [details]
Bug 1380224 - Part 1: Check custom properties for differences in Servo-backed style contexts.

https://reviewboard.mozilla.org/r/156894/#review162010
Attachment #8886088 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886037 [details]
style: Check custom properties for changes when computing damage.

https://reviewboard.mozilla.org/r/156824/#review162012

r=me on leaving a comment about it, if you want :)

Thanks for doing this refactoring!

::: servo/components/style/servo/restyle_damage.rs:68
(Diff revision 2)
>      pub fn compute_style_difference(old: &ServoComputedValues,
>                                      new: &ServoComputedValues)
>                                      -> StyleDifference {
>          let damage = compute_damage(old, new);
> -        let change = if damage.is_empty() { StyleChange::Unchanged } else { StyleChange::Changed };
> +        let change =
> +            if damage.is_empty() && old.get_custom_properties() == new.get_custom_properties() {

Variables in servo cause damage due to paint worklets, so this is unnecessary.
Attachment #8886037 - Flags: review-
Attachment #8886037 - Attachment is obsolete: true
Attachment #8886087 - Attachment is obsolete: true
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/54ebf794e1d3
Part 1: Check custom properties for differences in Servo-backed style contexts. r=emilio
https://hg.mozilla.org/integration/autoland/rev/58a0310411f8
Part 2: Reftest. r=emilio
https://hg.mozilla.org/mozilla-central/rev/54ebf794e1d3
https://hg.mozilla.org/mozilla-central/rev/58a0310411f8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Oops, a week too late. This is fixed in 20170721100159 and surely the 7 days before.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: