1273303 - style sheet additions (etc.) should check that rules might apply to the current document

Status: RESOLVED WONTFIX

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sites that dynamically add style sheets while pages are already loaded often do this in a way that the added style sheets cannot affect elements in the current page.

We should optimize for this by improving the aStyleSheet->HasRules() checks in methods like PresShell::StyleSheetAdded, etc. (all of the ones that call RecordStyleSheetChange) to check that there is at least one rule in the sheet that doesn't require the presence of an id or class that's known not to be in the document.

This requires maintaining a data structure for all of the ids/classes that are in the document -- or perhaps even better -- all the ones we've resolved style for.

We'd still need to rebuild the rule cascade -- which might make the dynamic media query change handling a little bit tricky (see, e.g., bug 1089417).

We also might want to first measure to try to figure out, for important cases, how much performance improvement we'd get from this versus bug 1273302.

Comment 1

[Cameron McCormack (:heycam)]

We have nsDocument::mIdentifierMap, which stores elements by id="" (and by name="").  Not sure if it's better to extend that to store elements by class="" too or to do as you suggest and have a separate table of IDs/classes that were used during previous selector matchings.

With bug 1213122 we will take all of the selectors in the added style sheet and only resolve style for elements in the document that do match them.  If that lands, we could skip adding selectors for those that fail this ID/class test.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, I believe this approach will be good enough to suppress the restyles for Facebook, and likely other sites that take a similar approach.  And it should be much cheaper than scanning the document, per-selector.

It's also probably worth issuing a console warning when we do have to restyle, and saying which selector was responsible.

Comment 3

[Jet Villegas (inactive)]

I'm inclined to implement this in Stylo rather than in the existing Gecko style system. 

+cc: Xidorn who's now working on Stylo's StyleSheet handling to put this on his radar.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I wonder how it makes sense to add a style sheet that the page doesn't use at the moment. Is that just for preloading the stylesheet? For that, I think the website should either add a style sheet which is disabled initially, and enable it later, or just use the resouce hint to trigger preload.

I'm not sure whether we've optimized for adding disabled style sheet, but that seems to be a much easier optimization than trying to figure out whether the style sheet could potentially affect the current rendering.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is apparently very common in large (and widely used!) sites that use large module systems for their content.  They ensure that they styles they add don't apply to the current page, and then add the content to the page that depends on those styles.

Comment 6

[Astley Chen (inactive)]

(In reply to Jet Villegas (:jet) from comment #3)
> I'm inclined to implement this in Stylo rather than in the existing Gecko
> style system. 

Is this bug in the scope of stylo implementation ?

Comment 7

[Cameron McCormack (:heycam)]

It's not one of the optimizations we discussed on Friday, but it's certainly something we could do in Stylo.  (And I agree that putting effort into doing it in Stylo rather than Gecko is the right thing to do.)  Given that this is on the Quantum Flow radar -- Bobby, do you think we should add this to our backlog?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

I would really want to see this done as part of stylo (I thought about doing that as a followup, given there's so much to do already, but we could also try to do it now).

Se an analysis of the blink approach at https://bugzilla.mozilla.org/show_bug.cgi?id=1213122#c34. FWIW they don't have any class to element map, they just do a full DOM traversal. Given we need to potentially do that traversal to do styling too, it seems like a good compromise to me, but we could also have that kind of map.

I think this shouldn't be hard to do, specially for the easy cases like bug 1213122 and facebook (which uses mostly class scopes). I can take this once I'm done with bug 1355343 and dependencies if you want.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> I would really want to see this done as part of stylo (I thought about doing
> that as a followup, given there's so much to do already, but we could also
> try to do it now).
> 
> Se an analysis of the blink approach at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1213122#c34. FWIW they don't
> have any class to element map, they just do a full DOM traversal. Given we
> need to potentially do that traversal to do styling too, it seems like a
> good compromise to me, but we could also have that kind of map.
> 
> I think this shouldn't be hard to do, specially for the easy cases like bug
> 1213122 and facebook (which uses mostly class scopes). I can take this once
> I'm done with bug 1355343 and dependencies if you want.

Yes, I think it makes sense to do this. We can use our parallel traversal machinery to make the traversal fast. It should be easy to add a perf-reftest for this as well.

Comment 10

[Bobby Holley (:bholley)]

Filed bug 1357583 for doing this in stylo.

Comment 11

[Daniel Holbert [:dholbert]]

Since this is a non-trivial style system change, we should aim to fix this in Stylo first (bug 1357583), and then we can "backport" the fix to Gecko (fixing this bug here) as needed.

--> adding dependency on bug 1357583

Comment 12

[Daniel Holbert [:dholbert]]

Given comment 11, I'm going to call this [qf:p2] as a Gecko bug.

Comment 13

[Bobby Holley (:bholley)]

Just to be clear, we shouldn't have somebody work on this for the Gecko style system unless it becomes clear that stylo won't make 57.

Comment 14

[Bobby Holley (:bholley)]

dbaron, what was the original context where we saw this showing up on Facebook? If there's a workflow we can profile, it would be interesting to compare stylo vs gecko given bug 1357583.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The original context was that we were told about it verbally by folks who work at Facebook.  I think the thing to profile would probably be initial load of https://www.facebook.com/, perhaps with a preference for a slower network connection.  But I'm not entirely sure.

Comment 16

[Andrew Overholt [:overholt]]

Given bug 1357583 is fixed (in Stylo) and I don't think we want to do work on the old style system, I'm going to close this.