Bug 253337

<rdar://problem/106206220>

Created attachment 465283 [details] Patch

Created attachment 465284 [details] Patch

Comment on attachment 465284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=465284&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:2722 > + if (axObject) { if !axObject it looks like we're returning true. that might be different than in the past either way, can we do an early return here, so we don't have to indent the rest of this

(In reply to Tyler Wilcock from comment #3) > Created attachment 465284 [details] > Patch --- a/Source/WebCore/accessibility/AccessibilityObject.cpp +++ b/Source/WebCore/accessibility/AccessibilityObject.cpp // [Bug: 136247] Heuristic: element handlers that have more than one accessible descendant should not be exposed as supporting press. I would get rid of this heuristics altogether and eliminate the need for this block of code. Unless there is a real example where this is an actual problem, to me this is trying to implement an obscure rule that has little or none user experience impact.

(In reply to chris fleizach from comment #4) > Comment on attachment 465284 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=465284&action=review > > > Source/WebCore/accessibility/AccessibilityObject.cpp:2722 > > + if (axObject) { > > if !axObject it looks like we're returning true. that might be different > than in the past Interestingly, that was the behavior before: if (actionElement != element()) { if (AccessibilityObject* axObj = axObjectCache()->getOrCreate(actionElement)) { ... } } return !nodeHasPresentationRole(actionElement); If actionElement wasn't element(), or if we couldn't getOrCreate(actionElement), then we would return true as long as actionElement didn't have a presentation role. It sort of makes sense -- the idea seems to be that the AX object is a root from which inaccessible-but-clickable descendants can be exposed, so it doesn't matter if we can getOrCreate() something for said clickable descendants. > either way, can we do an early return here, so we don't have to indent the > rest of this Will do.

(In reply to Andres Gonzalez from comment #5) > (In reply to Tyler Wilcock from comment #3) > > Created attachment 465284 [details] > > Patch > > --- a/Source/WebCore/accessibility/AccessibilityObject.cpp > +++ b/Source/WebCore/accessibility/AccessibilityObject.cpp > > // [Bug: 136247] Heuristic: element handlers that have more than one > accessible descendant should not be exposed as supporting press. > > I would get rid of this heuristics altogether and eliminate the need for > this block of code. Unless there is a real example where this is an actual > problem, to me this is trying to implement an obscure rule that has little > or none user experience impact. The goal of this code and related code (i.e. AccessibilityObject::actionElement(), AccessibilityObject::press()) is to expose mouse-event handlers that are not made properly accessible. For example: <span onclick="handleClick(this)" style="...styled to look like a button...">Open Shopping Cart (improper usage because no button role)</span> I think this heuristic is not perfect, and probably less needed than when it was introduced years ago, but still provides value. I think if it causes us more trouble beyond this patch, we should more deeply re-litigate whether it's worth the complexity.

Created attachment 465320 [details] Patch

(In reply to Tyler Wilcock from comment #7) > (In reply to Andres Gonzalez from comment #5) > > (In reply to Tyler Wilcock from comment #3) > > > Created attachment 465284 [details] > > > Patch > > > > --- a/Source/WebCore/accessibility/AccessibilityObject.cpp > > +++ b/Source/WebCore/accessibility/AccessibilityObject.cpp > > > > // [Bug: 136247] Heuristic: element handlers that have more than one > > accessible descendant should not be exposed as supporting press. > > > > I would get rid of this heuristics altogether and eliminate the need for > > this block of code. Unless there is a real example where this is an actual > > problem, to me this is trying to implement an obscure rule that has little > > or none user experience impact. > The goal of this code and related code (i.e. > AccessibilityObject::actionElement(), AccessibilityObject::press()) is to > expose mouse-event handlers that are not made properly accessible. For > example: > > <span onclick="handleClick(this)" style="...styled to look like a > button...">Open Shopping Cart (improper usage because no button role)</span> > > I think this heuristic is not perfect, and probably less needed than when it > was introduced years ago, but still provides value. I think if it causes us > more trouble beyond this patch, we should more deeply re-litigate whether > it's worth the complexity. This patch is definitely a big improvement over what we had before, so I'm totally fine going ahead with it. Trying to get to the bottom of the issue though, in your example above, wouldn't it be more straightforward to expose any span with an onclick handler? To avoid exposing elements like the body or a div that has many descendants as clickable AX objects, we may simplify the traversal to look for a descendant that cannot be clickable instead of the current search for two clickable ones.

> Trying to get to the bottom of the issue > though, in your example above, wouldn't it be more straightforward to expose > any span with an onclick handler? To avoid exposing elements like the body > or a div that has many descendants as clickable AX objects, we may simplify > the traversal to look for a descendant that cannot be clickable instead of > the current search for two clickable ones. Yeah, I do think that would work and ultimately be more simple. My vote would be to consider that for a future change rather than change this patch to do that, since it would require reworking ::supportsPressAction, ::press, ::actionElement, and probably others, all for roughly the same effect. What do you think?

(In reply to Tyler Wilcock from comment #10) > > Trying to get to the bottom of the issue > > though, in your example above, wouldn't it be more straightforward to expose > > any span with an onclick handler? To avoid exposing elements like the body > > or a div that has many descendants as clickable AX objects, we may simplify > > the traversal to look for a descendant that cannot be clickable instead of > > the current search for two clickable ones. > Yeah, I do think that would work and ultimately be more simple. > > My vote would be to consider that for a future change rather than change > this patch to do that, since it would require reworking > ::supportsPressAction, ::press, ::actionElement, and probably others, all > for roughly the same effect. What do you think? Agree, let's go ahead with this change which is critical for ITM, and filing a bug for the rework. Thanks!

> Agree, let's go ahead with this change which is critical for ITM, and filing > a bug for the rework. Thanks! Filed: https://bugs.webkit.org/show_bug.cgi?id=253464

Created attachment 465329 [details] Patch

Created attachment 465345 [details] Patch

Created attachment 465364 [details] Patch

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