stylo: style change via insertRule and deleteRule may not work

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: xidorn, Assigned: emilio)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

5 months ago
Created attachment 8833827 [details]
testcase

See the attached testcase. In theory, the newly inserted style should be applied to the element, so the color should be green and the computed style would change to green as well. But it doesn't work.

Solely delete the given rule via deleteRule doesn't work either, but if you use both insertRule and deleteRule, things magically start to work.

CSSOM code should have noticed the document that there is rule added / removed. Probably the corresponding flag in document is not checked?
Is this basically the same issue as bug 1331874, where we don't implement the RestyleManager callbacks correctly?
(Reporter)

Comment 2

5 months ago
I'm unsure about that. Bug 1331874 sounds more similar to bug 1331301 where style is added as part of the document (via DOM API), so maybe restyle manager isn't aware of the new stylesheets. In this case, the style info is added via CSSOM, and the CSSOM API correctly informs the document that there are changes to existing stylesheets.

I would suggest they are different issues.
Xidorn, are you going to take these kinds of bugs?
Flags: needinfo?(xidorn+moz)
(In reply to Andrew Overholt [:overholt] from comment #3)
> Xidorn, are you going to take these kinds of bugs?

You can avoid triaging any bugs with |stylo:| in the title. We'll take care of them.
Flags: needinfo?(xidorn+moz)
Component: DOM: CSS Object Model → CSS Parsing and Computation
Priority: -- → P1
CJ, Jet said you could help us out on Stylo. Would you mind looking at this?

Let me know if you have any questions. :-)
Assignee: nobody → cku
Flags: needinfo?(cku)

Comment 6

3 months ago
Sure
Flags: needinfo?(cku)
(Reporter)

Comment 7

3 months ago
I suspect bug 1347381 may also fix this. Let's wait for that to land first and see.
Depends on: 1347381

Comment 8

3 months ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #7)
> I suspect bug 1347381 may also fix this. Let's wait for that to land first
> and see.

noted
(Assignee)

Comment 9

3 months ago
bug 1347381 fixed at least one test mentioning this bug. I think there are a few failures left, but I haven't dug in whether they're legit or not.
From comment 2, I suspect this is related to bug 1334732. Unassigning from CJ for now.
Assignee: cku → nobody
Depends on: 1334732
Priority: P1 → P2
Blocks: 1243581
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #0)
> Created attachment 8833827 [details]
> testcase
> 
> See the attached testcase. In theory, the newly inserted style should be
> applied to the element, so the color should be green and the computed style
> would change to green as well. But it doesn't work.
> 
> Solely delete the given rule via deleteRule doesn't work either, but if you
> use both insertRule and deleteRule, things magically start to work.

While writing test cases for bug 1356779, I noticed similar thing.  A single insertRule call does not work but consecutive insertRule calls works fine.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

> While writing test cases for bug 1356779, I noticed similar thing.  A single
> insertRule call does not work but consecutive insertRule calls works fine.

I found the reason why single insertRule call does not work and why multiple calls works.

On current stylo in PresShell::EndUpdate() [1], we call ServoStyleSet::EndUpdate() and then RestyleForCSSRuleChange(). ServoStyleSet::EndUpdate() calls Servo_StyleSet_FlushStyleSheets(), RestyleForCSSRuleChanges() ends up calling Servo_StyleSet_NoteStyleSheetsChanged (calls force_dirty()).

So, when we call a insertRule, we call Servo_StyleSet_FlushStyleSheets() first then set the dirty flag. Whereas when we call two insertRule, we call Servo_StyleSet_FlushStyleSheets() and set the dirty flag and call the second Servo_StyleSet_FlushStyleSheets(), it lead correctly to update stylesheets.

[1] https://hg.mozilla.org/mozilla-central/file/a30dc237c3a6/layout/base/PresShell.cpp#l2525
(Assignee)

Comment 13

2 months ago
Hmmm... So I thought we were flushing stylesheets as appropriate each restyle, but apparently that changed, and we only flush explicitly now.

Given we don't, we should assert when restyling that stylesheets are up-to-date. I'm doing a try run with such assertion to know if it's tested, then will send a fix.

Great catch hiro!
(Assignee)

Updated

2 months ago
Assignee: nobody → emilio+bugs
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41029bfbe11c5cf35d5f11b23251b6804f055028

This exposes a few more media query tests passing (yay!), and a few -moz-linear-gradient tests failing that were passing accidentally before.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I did call NoteStyleSheetsChanged() in PresShell::RecordStyleSheetChange() locally and it worked fine at least for my test case.  Considering the name, RecordStyleSheetChange, it's a reasonable place to call NoteStyleSheetsChanged(). No?
(Assignee)

Comment 19

2 months ago
Yeah, I missed that one method, seems like the right place to call NoteStylesheetsChanged, actually. Will fix that up, and remove the current call.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=41029bfbe11c5cf35d5f11b23251b6804f055028
> 
> This exposes a few more media query tests passing (yay!), and a few
> -moz-linear-gradient tests failing that were passing accidentally before.

Though we have not run tests in dom/animations/test on automation, but there are a bunch of test cases that will be passed with these patches! yay!
(Assignee)

Comment 23

2 months ago
Oh, btw, the try run is even better, only new passes (other local patches were affecting the previous one):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=962ef3d4057202c19659c13922720ac443eb4b88

Comment 24

2 months ago
mozreview-review
Comment on attachment 8861623 [details]
Bug 1336863: Flush stylesheets in RestyleForCSSRuleChanges if not under an update.

https://reviewboard.mozilla.org/r/133604/#review136576

::: commit-message-3f0c8:1
(Diff revision 2)
> +Bug 1336863: Flush stylesheets in RestyleForCSSRuleChanges if not under an update. r?heycam

This commit message describes the ServoStyleSet::NoteStyleSheetsChanged change, which seems reasonable on its face.  But please also explain why we are making the other changes related to author style.

::: layout/style/ServoStyleSet.cpp:135
(Diff revision 2)
> -  if (mAuthorStyleDisabled == aStyleDisabled) {
> +  MOZ_ASSERT(mAuthorStyleDisabled != aStyleDisabled);
> -    return NS_OK;
> -  }
> -

Making this assert if we try to set the same value feels a little hostile to me. :-)  Would prefer to leave the check and early return.

::: layout/style/ServoStyleSet.cpp:138
(Diff revision 2)
>    // If we've just enabled, then PresShell will trigger the notification and
>    // later flush when the stylesheet objects are enabled in JS.

Is this comment still accurate?

When we toggle author style disabling, where do we actually end up calling NoteStyleSheetsChanged?
(Assignee)

Comment 25

2 months ago
mozreview-review
Comment on attachment 8861623 [details]
Bug 1336863: Flush stylesheets in RestyleForCSSRuleChanges if not under an update.

https://reviewboard.mozilla.org/r/133604/#review136742

::: commit-message-3f0c8:1
(Diff revision 2)
> +Bug 1336863: Flush stylesheets in RestyleForCSSRuleChanges if not under an update. r?heycam

That's fair, I adapted it in light of hiro's comments :)

::: layout/style/ServoStyleSet.cpp:138
(Diff revision 2)
>    // If we've just enabled, then PresShell will trigger the notification and
>    // later flush when the stylesheet objects are enabled in JS.

I guess it is, yeah. With my first patch we called RestyleForCSSRuleChanges, which would call NoteStyleSheetsChanged.

But then I moved it, oh well... I guess I'll restore the NoteStyleSheetsChanged, because I don't want to dive in on it right now.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

2 months ago
mozreview-review
Comment on attachment 8861618 [details]
Bug 1336863: Update expectations.

https://reviewboard.mozilla.org/r/133592/#review137148

::: servo/ports/geckolib/glue.rs:182
(Diff revision 4)
> +    debug_assert!(!per_doc_data.stylesheets.has_changed());
> +

Could this fail if we're doing the "resolve styles in a subtree for a reconstruction, but use old ancestor styles"?  Though if you're getting rid of that in that other bug...
Attachment #8861618 - Flags: review?(cam) → review+

Comment 29

2 months ago
mozreview-review
Comment on attachment 8861623 [details]
Bug 1336863: Flush stylesheets in RestyleForCSSRuleChanges if not under an update.

https://reviewboard.mozilla.org/r/133604/#review137152

::: layout/style/ServoStyleSet.cpp:146
(Diff revision 3)
>    // If we've just enabled, then PresShell will trigger the notification and
>    // later flush when the stylesheet objects are enabled in JS.
> +  //
> +  // TODO(emilio): Users can have JS disabled, can't they? Will that affect that
> +  // notification on content documents?

I still don't really know what JS is being referred to here.  It seems unlikely that we have to rely on content JS doing something to respond to author styles having just been enabled...
Attachment #8861623 - Flags: review?(cam) → review+
(Assignee)

Comment 30

2 months ago
mozreview-review
Comment on attachment 8861618 [details]
Bug 1336863: Update expectations.

https://reviewboard.mozilla.org/r/133592/#review137250

::: servo/ports/geckolib/glue.rs:182
(Diff revision 4)
> +    debug_assert!(!per_doc_data.stylesheets.has_changed());
> +

It shouldn't, given we don't update the styles asynchronously (we do it when the styleset update ends, and we don't process restyles during a styleset update).

I'd want to eventually make that async, though. But right now this can't fail.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 33

2 months ago
mozreview-review
Comment on attachment 8861618 [details]
Bug 1336863: Update expectations.

https://reviewboard.mozilla.org/r/133592/#review137282
Attachment #8861618 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 34

2 months ago
Mozreview/Autoland are quite drunk. Both commits have autolanded with r=heycam, apparently, but pulsebot seems to have missed them too? shrug.
Taskcluster was having problems earlier this afternoon, might be why.

https://hg.mozilla.org/integration/autoland/rev/7a7dd1e981cd9214eef68681a9b2b932f62ee3f5
https://hg.mozilla.org/integration/autoland/rev/bad07d55c3391061415e89405626f19c3f49ba68

Comment 36

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bad07d55c339
https://hg.mozilla.org/mozilla-central/rev/7a7dd1e981cd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

2 months ago
Blocks: 1360403
You need to log in before you can comment on or make changes to this bug.