| Summary: | AX: Several classes never reset m_subtreeDirty to false | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tyler Wilcock <tyler_w> | ||||||
| Component: | Accessibility | Assignee: | Tyler Wilcock <tyler_w> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | Other | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Tyler Wilcock
2023-07-07 15:11:11 PDT
Created attachment 466985 [details]
Patch
Comment on attachment 466985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=466985&action=review > Source/WebCore/accessibility/AccessibilityTable.cpp:683 > + m_subtreeDirty = false; do we need the clearDirtySubtree scopeExit and this one here? > Source/WebCore/accessibility/AccessibilityTableRow.cpp:147 > + m_subtreeDirty = false; should we have a scope exit here? (In reply to chris fleizach from comment #3) > Comment on attachment 466985 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=466985&action=review > > > Source/WebCore/accessibility/AccessibilityTable.cpp:683 > > + m_subtreeDirty = false; > > do we need the clearDirtySubtree scopeExit and this one here? This function does have a clearDirtySubtree-scopeExit, but I intentionally want to make sure to set `m_subtreeDirty = false` before updateChildrenRoles() to avoid unnecessarily passing a dirty subtree flag around if child roles do get updated (since that kicks off a lot of other operations). > > Source/WebCore/accessibility/AccessibilityTableRow.cpp:147 > > + m_subtreeDirty = false; > > should we have a scope exit here? In my opinion, it's unnecessary for this one since both branches in this if-statement now cover modifying the flag (with the `else` modifying it through AccessibilityRenderObject::addChildren()). I don't feel overly strong on either of these points if you have a different preference, though. Created attachment 466987 [details]
Patch
Committed 265878@main (2517a540e6f5): <https://commits.webkit.org/265878@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 466987 [details]. |