350002 - Reposition HasAttributeDependentStyle() to follow AttributeChanged()

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[rbs]

At the moment, nsCSSFrameConstructor::AttributeChanged() calls the HasAttributeDependentStyle() optimization before calling the handler of the frame's AttributeChanged().

I would like to swap the positions so that the frame's AttributeChanged handlder is called first, allowing it to possibly insert a rule in a stylesheet so that a subsequent HasAttributeDependentStyle() can pick up the update.

The motivation is because of the way MathML maps its attributes to style lazily. I am doing some MathML's AttributeChanged() work (in bug 179619) and add style rules to the internal MathML's attribute stylesheet on the fly -- when their addition is deemed necessary in the various AttributeChanged() handlers. But I have to manually check the attributes that matter and make the re-resolve.

I could avoid that for some common attributes. I miss the automatic re-resolve (which even happens asynchronously these days via the nifty PostRestyleEvent() that bz added a little while ago) because the early call of HasAttributeDependentStyle() didn't find the attributes and so optimize nsCSSFrameConstructor::AttributeChanged.

Another option for me would be to pre-propulate the attribute stylesheet with some bogus rules that involve those attributes. But it seems quirky -- compared to just swapping the calls in nsCSSFrameConstructor::AttributeChanged...

Given the working of the HasAttributeDependentStyle() optimization (way back from bug 38378 per cvs archeology), I don't forsee any side-effects.

Any objection to the swapping?

Comment 1

[rbs]

Attachment: swapping patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 235228 [details] [diff] [review]
swapping patch

Ergh...  This is ok for now, maybe, but we want to remove nsIFrame::AttributeChanged if we can.  Why can't we just create mathml content objects and map attributes into style via the mapped attribute  stuff we already have (which would need minor modification to not be HTML-specific)?  That would be a much cleaner and more performant solution...

If we do make this change, please add comments explaining the ordering dependency so people don't switch this back.

I really think dbaron should ok this.

Comment 3

[rbs]

> Why can't we just create mathml content objects and map attributes into style
> via the mapped attribute  stuff we already have

We can and we will eventually get there, but I fear whether the extra footprint warrants the benefits at this stage (compared to the function that does it now). Also the mappable attributes work well for simple cases (which are what are handled with the public, on the fly, InsertRule I mentioned). If we were to rely solely on mappable attributes, complex attributes would involve making the Style System itself understand things that depend on the actual textual content, or the :nth-child (on the C++ side for MathML specific things -- c.f. bug 179619 comment 10). Hence, simply having MathML content objects don't resolve the matter. Having seen how those non-critical bugs hang around for so long, I am afraid issues won't have high priority, leading to a bog down, or the fear from you guys that the patches are invasive and break something else, or getting me overloaded with the implied general tasks.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> but I fear whether the extra footprint

I'd think there would be very little extra footprint -- some extra vtables and a very few method overrides.

I'm not sure I follow the rest of your comment, frankly; are you saying that there are MathML attributes that cannot be mapped into CSS?

Comment 5

[rbs]

They can. But not easily (or cheaply) as it seems -- e.g., bug 179619 comment 10.

Comment 6

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

Comment on attachment 235228 [details] [diff] [review]
swapping patch

sr=dbaron.  I don't have much of an opinion about how this should work since I don't think nsIFrame should have an AttributeChanged method; Attribute handling should live in content or style.

Comment 7

[rbs]

Checked in.

Re: comment #6
What about CharacterDataChanged() -- also considering removing that too on the same principle? Both methods are mainly for frames to sync some of their internal cached state that depends on such attributes (or a combination of them). For HTML content, AttributeChanged() might not be that much relevant, indeed, since its attributes are fully resolved in the style. But other content implementations might end up notify their frames anyway -- unless that state is added to the style context structs and updated via the style mechanism. It could end up being less flexible.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Content with special needs could test the frame type and cast to a known frame class, then poke that via a private interface. SVG does this. Text could do it. I think it probably is the better way to go.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It would encourage people to put more functionality in content, which in the end is a good thing