Bug 255478 – Attribute-dependent state is not updated for changes of certain attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Tyler Wilcock]

Several methods are called in Element::attributeChanged with the expectation that this method will actually be called for every attribute change:

https://github.com/WebKit/WebKit/blob/c5b51dbe820352229236745ca1208d3eb0bc75df/Source/WebCore/dom/Element.cpp#L2131#L2139

However, Element::attributeChanged is a virtual method, and many subclasses return early without calling up to Element::changed. For example, HTMLOptionElement::attributeChanged swallows disabled attribute changes:

https://github.com/WebKit/WebKit/blob/c5b51dbe820352229236745ca1208d3eb0bc75df/Source/WebCore/html/HTMLOptionElement.cpp#L180#L188

This means that the attribute-dependent state updates in Element::attributeChanged will not happen.

[Tyler Wilcock]

This was fixed for accessibility updates in https://bugs.webkit.org/show_bug.cgi?id=255433.

[Radar WebKit Bug Importer]

<rdar://problem/108078305>

[Tyler Wilcock]

Created attachment 465923 [details]
Patch

[Tyler Wilcock]

Queued Speedometer runs since this is a hot path.

[Tyler Wilcock]

Created attachment 465924 [details]
Patch

[Tyler Wilcock]

Actually, contrary to the bug description, it seems that all of Element::attributeChanged is coded under the assumption that it will be called for every attribute change. Uploading a new patch.

[Tyler Wilcock]

Created attachment 465925 [details]
Patch

[Chris Dumez]

(In reply to Tyler Wilcock from comment #6)
> Actually, contrary to the bug description, it seems that all of
> Element::attributeChanged is coded under the assumption that it will be
> called for every attribute change. Uploading a new patch.

No, I don’t think this is true. Subclasses will make sure to call into the base class for attributes the base class may care about (like id or class). And the subclasses won’t call into the base class for attributes it wouldn’t care about, as an optimization. This latest iteration kills this optimization and likely is way too aggressive for what you want to fix.

[Chris Dumez]

The earlier iteration looked more correct to me.

[Tyler Wilcock]

Got it, makes sense. Will revert to the previous patch.

[Tyler Wilcock]

Earlier patch un-obsoleted and Speedometer queued, later patch obsoleted.

[Tyler Wilcock]

This change is neutral on Speedometer.

[Chris Dumez]

Comment on attachment 465924 [details]
Patch

r=me

[EWS]

Committed 263005@main (0e0b997d2763): <https://commits.webkit.org/263005@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 465924 [details].