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?

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?

(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.

CJ, Jet said you could help us out on Stylo. Would you mind looking at this? Let me know if you have any questions. :-)

Sure

I suspect bug 1347381 may also fix this. Let's wait for that to land first and see.

(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

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.

(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

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!

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.

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?

Yeah, I missed that one method, seems like the right place to call NoteStylesheetsChanged, actually. Will fix that up, and remove the current call.

(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!

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 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?

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 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...

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...

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 on attachment 8861618 [details] Bug 1336863: Update expectations. https://reviewboard.mozilla.org/r/133592/#review137282

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

https://hg.mozilla.org/mozilla-central/rev/bad07d55c339 https://hg.mozilla.org/mozilla-central/rev/7a7dd1e981cd