Bug 255478 - Attribute-dependent state is not updated for changes of certain attributes
Summary: Attribute-dependent state is not updated for changes of certain attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tyler Wilcock
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-04-14 21:28 PDT by Tyler Wilcock
Modified: 2023-04-15 20:33 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.63 KB, patch)
2023-04-14 21:33 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (9.35 KB, patch)
2023-04-14 21:45 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (9.31 KB, patch)
2023-04-14 22:16 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].