Bug 255478

Summary: Attribute-dependent state is not updated for changes of certain attributes
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: DOMAssignee: Tyler Wilcock <tyler_w>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cmarcelo, esprehn+autocc, ews-watchlist, kangil.han, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Tyler Wilcock
Reported 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.
Attachments
Patch (3.63 KB, patch)
2023-04-14 21:33 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Patch (9.35 KB, patch)
2023-04-14 21:45 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Patch (9.31 KB, patch)
2023-04-14 22:16 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Tyler Wilcock
Comment 1 2023-04-14 21:28:57 PDT
This was fixed for accessibility updates in https://bugs.webkit.org/show_bug.cgi?id=255433.
Radar WebKit Bug Importer
Comment 2 2023-04-14 21:29:28 PDT
<rdar://problem/108078305>
Tyler Wilcock
Comment 3 2023-04-14 21:33:07 PDT
Created attachment 465923 [details] Patch
Tyler Wilcock
Comment 4 2023-04-14 21:38:24 PDT
Queued Speedometer runs since this is a hot path.
Tyler Wilcock
Comment 5 2023-04-14 21:45:20 PDT
Created attachment 465924 [details] Patch
Tyler Wilcock
Comment 6 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.
Tyler Wilcock
Comment 7 2023-04-14 22:16:27 PDT
Created attachment 465925 [details] Patch
Chris Dumez
Comment 8 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.
Chris Dumez
Comment 9 2023-04-14 22:21:30 PDT
The earlier iteration looked more correct to me.
Tyler Wilcock
Comment 10 2023-04-14 22:23:17 PDT
Got it, makes sense. Will revert to the previous patch.
Tyler Wilcock
Comment 11 2023-04-14 22:30:20 PDT
Earlier patch un-obsoleted and Speedometer queued, later patch obsoleted.
Tyler Wilcock
Comment 12 2023-04-15 09:35:45 PDT
This change is neutral on Speedometer.
Chris Dumez
Comment 13 2023-04-15 19:44:53 PDT
Comment on attachment 465924 [details] Patch r=me
EWS
Comment 14 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].
Note You need to log in before you can comment on or make changes to this bug.