Bug 253247

<rdar://problem/106148404>

Created attachment 465263 [details] Patch

Comment on attachment 465263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=465263&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2147 > if (!node) I don’t know if weak ptr helps here. If it goes away, we might still hit a null ptr. Do we instead want to ref this so we hold onto to it until end of scope?

Comment on attachment 465263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=465263&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-2173 > + // For <select> elements, title should be empty if they are not rendered or have role PopUpButton. > + if (node && node->hasTagName(selectTag) > + && (!isAccessibilityRenderObject() || roleValue() == AccessibilityRole::PopUpButton)) > + return { }; > > switch (roleValue()) { > - case AccessibilityRole::PopUpButton: > - // Native popup buttons should not use their button children's text as a title. That value is retrieved through stringValue(). > - if (node->hasTagName(selectTag)) > - return String(); > - FALLTHROUGH; Prior to your change, AccessibilityRole::PopUpButton without node->hasTagName(selectTag) would FALLTHROUGH to useTextUnderElement = true. And now we won't do that (because AccessibilityRole::PopUpButton is no longer handled in the switch). Am I reading that right, and if so is this behavior change intentional? > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:114 > + AXLOG(makeString("AXIsolatedObject::attributedStringForTextMarkerRange", " role = ", accessibilityRoleToString(roleValue()))); Definitely understand why these logs would be useful for your work on this bug, but are they generally useful? I think this gets requested a lot, so they might make our logs more noisy then they already are. (Same comment for the AXTRACE in this method)

(In reply to chris fleizach from comment #3) > Comment on attachment 465263 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=465263&action=review > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2147 > > if (!node) > > I don’t know if weak ptr helps here. If it goes away, we might still hit a > null ptr. Do we instead want to ref this so we hold onto to it until end of > scope? All depends on what we are doing with the pointer in the given scope. In this case I think WeakPtr is fine because we only use it for dynamicDowncast or we check for nil before using. That's what makes this a bit tricky, one has to understand the context. In general we don't always want to do RefPtr, especially for Nodes or other objects that we don't create.

Created attachment 465266 [details] Patch

(In reply to Tyler Wilcock from comment #4) > Comment on attachment 465263 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=465263&action=review > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-2173 > > + // For <select> elements, title should be empty if they are not rendered or have role PopUpButton. > > + if (node && node->hasTagName(selectTag) > > + && (!isAccessibilityRenderObject() || roleValue() == AccessibilityRole::PopUpButton)) > > + return { }; > > > > switch (roleValue()) { > > - case AccessibilityRole::PopUpButton: > > - // Native popup buttons should not use their button children's text as a title. That value is retrieved through stringValue(). > > - if (node->hasTagName(selectTag)) > > - return String(); > > - FALLTHROUGH; > > Prior to your change, AccessibilityRole::PopUpButton without > node->hasTagName(selectTag) would FALLTHROUGH to useTextUnderElement = true. > And now we won't do that (because AccessibilityRole::PopUpButton is no > longer handled in the switch). Am I reading that right, and if so is this > behavior change intentional? Good catch! meant to put it down below and forgot! > > > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:114 > > + AXLOG(makeString("AXIsolatedObject::attributedStringForTextMarkerRange", " role = ", accessibilityRoleToString(roleValue()))); > > Definitely understand why these logs would be useful for your work on this > bug, but are they generally useful? I think this gets requested a lot, so > they might make our logs more noisy then they already are. (Same comment for > the AXTRACE in this method) Removed all the new AX logging. We definitely have to improve our logging so that we can leave debugging stuff like this, so that you don't have to re-create it, and still doesn't show in the logs unless you want to.

Committed 261140@main (1b2f162302f9): <https://commits.webkit.org/261140@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 465266 [details].