Bug 255478

Summary:

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

Product:

WebKit

Reporter:

Tyler Wilcock <tyler_w>

Component:

DOM

Assignee:

Tyler Wilcock <tyler_w>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, cmarcelo, esprehn+autocc, ews-watchlist, kangil.han, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Description Tyler Wilcock 2023-04-14 21:28:22 PDT

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.

Comment 1 Tyler Wilcock 2023-04-14 21:28:57 PDT

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

Comment 2 Radar WebKit Bug Importer 2023-04-14 21:29:28 PDT

<rdar://problem/108078305>

Comment 3 Tyler Wilcock 2023-04-14 21:33:07 PDT

Created attachment 465923 [details]
Patch

Comment 4 Tyler Wilcock 2023-04-14 21:38:24 PDT

Queued Speedometer runs since this is a hot path.

Comment 5 Tyler Wilcock 2023-04-14 21:45:20 PDT

Created attachment 465924 [details]
Patch

Comment 6 Tyler Wilcock 2023-04-14 22:16:15 PDT

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.

Comment 7 Tyler Wilcock 2023-04-14 22:16:27 PDT

Created attachment 465925 [details]
Patch

Comment 8 Chris Dumez 2023-04-14 22:20:38 PDT

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

Comment 9 Chris Dumez 2023-04-14 22:21:30 PDT

The earlier iteration looked more correct to me.

Comment 10 Tyler Wilcock 2023-04-14 22:23:17 PDT

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

Comment 11 Tyler Wilcock 2023-04-14 22:30:20 PDT

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

Comment 12 Tyler Wilcock 2023-04-15 09:35:45 PDT

This change is neutral on Speedometer.

Comment 13 Chris Dumez 2023-04-15 19:44:53 PDT

Comment on attachment 465924 [details]
Patch

r=me

Comment 14 EWS 2023-04-15 20:33:20 PDT

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