Bug 268672 – AX: Implement off-main-thread AXTextMarker::partialOrder and AXCoreObject::textMarkerRange

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

...

[Radar WebKit Bug Importer]

<rdar://problem/122212250>

[Tyler Wilcock]

Created attachment 469682 [details]
Patch

[Tyler Wilcock]

Created attachment 469716 [details]
Patch

[Joshua Hoffman]

Comment on attachment 469716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=469716&action=review

> Source/WebCore/accessibility/AXTextMarker.cpp:610
> +    RefPtr start = isolatedObject();
> +    RefPtr current = start->hasTextRuns() ? WTFMove(start) : findObjectWithRuns(*start, AXDirection::Next);

Is `start` guaranteed to be non-null here? AXTextMarker::object() looks like it can return a nullptr, which would make start null as well.

> Source/WebCore/accessibility/AXTextMarker.h:183
> +    AXTextMarker findLast(std::optional<AXID> = std::nullopt) const;

Nit: do you think we should name this something like `findLastBefore`? This comment helps explain that it navigates "to the last marker before the given AXID", but I can see confusion without that context.

> Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:144
> +        // This object doesn't have text content of its own. Create a range pointing to the first and last
> +        // text positions of our descendants. We can do this by stopping text marker traversal when we try
> +        // to move to our sibling. For example, getting textMarkerRange() for {ID 1, Role Group}:

These comments & diagrams have been super helpful, thanks for including them!

[Tyler Wilcock]

Created attachment 469752 [details]
Patch

[Tyler Wilcock]

(In reply to Joshua Hoffman from comment #4)
> Comment on attachment 469716 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=469716&action=review
> 
> > Source/WebCore/accessibility/AXTextMarker.cpp:610
> > +    RefPtr start = isolatedObject();
> > +    RefPtr current = start->hasTextRuns() ? WTFMove(start) : findObjectWithRuns(*start, AXDirection::Next);
> 
> Is `start` guaranteed to be non-null here? AXTextMarker::object() looks like
> it can return a nullptr, which would make start null as well.
It is technically guaranteed to be non-null, as we return early for !isValid() above. But if the object dies between that check and when we use isolatedObject() here, then we will nullptr crash. I'm not sure if this is possible, but if it is, we have this problem elsewhere and we can address it holistically later.

> > Source/WebCore/accessibility/AXTextMarker.h:183
> > +    AXTextMarker findLast(std::optional<AXID> = std::nullopt) const;
> 
> Nit: do you think we should name this something like `findLastBefore`? This
> comment helps explain that it navigates "to the last marker before the given
> AXID", but I can see confusion without that context.
Good idea, fixed!

[Tyler Wilcock]

Created attachment 469753 [details]
Patch

[EWS]

Committed 274240@main (6429c99998c8): <https://commits.webkit.org/274240@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 469753 [details].