...
<rdar://problem/121157922>
Created attachment 469434 [details] Patch
Created attachment 469435 [details] Patch
Created attachment 469436 [details] Patch
Comment on attachment 469436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=469436&action=review > LayoutTests/accessibility/ax-thread-text-apis/text-marker-for-index.html:71 > +function forward(count, previousMarker, currentMarker, obj) { > + for (var i = 0; i < count; i++) { > + previousMarker = currentMarker; > + currentMarker = obj.nextTextMarker(currentMarker); > + } > + return { > + previous: previousMarker, > + current: currentMarker > + }; > +} > +function replaceLinebreakInString(str) { > + var newline = '\n'; > + str = str.replace(newline, "'line break'"); > + return str; > +} > + > +function verifyMarkerIndex(previousMarker, textMarker, obj) { > + var markerRange = obj.textMarkerRangeForMarkers(previousMarker, textMarker); > + var originalString = replaceLinebreakInString(obj.stringForTextMarkerRange(markerRange)); > + output += `Original marker string: ${originalString}\n`; > + > + var index = obj.indexForTextMarker(textMarker); > + var newMarker = obj.textMarkerForIndex(index); > + markerRange = obj.textMarkerRangeForMarkers(previousMarker, newMarker); > + var newString = replaceLinebreakInString(obj.stringForTextMarkerRange(markerRange)); > + output += `Index is: ${index}\nNew marker string: ${newString}\n`; > +} Do you anticipate we'd want these three methods in any other tests? Maybe, like we do for tables, we could have a JS helper file with these. > LayoutTests/accessibility/ax-thread-text-apis/text-marker-range-with-unordered-markers.html:30 > + for (let i = 0; i < 10; ++i) > + start = text.nextTextMarker(start); Would it be possible to abstract these loops into methods, either in a helper JS file or in AccessibilityUIElement itself? Perhaps something like `text.nextTextMarker(start, x)`, where x is 10 in this case. Unless I am missing why iterating here is important?
(In reply to Tyler Wilcock from comment #4) > Created attachment 469436 [details] > Patch diff --git a/Source/WebCore/accessibility/AXTextMarker.cpp b/Source/WebCore/accessibility/AXTextMarker.cpp index 80d325eb4be2..8b31c3b88794 100644 --- a/Source/WebCore/accessibility/AXTextMarker.cpp +++ b/Source/WebCore/accessibility/AXTextMarker.cpp @@ -167,6 +167,11 @@ AXTextMarker::operator CharacterOffset() const return result; } +bool AXTextMarker::hasSameObjectAndOffset(const AXTextMarker& other) const +{ + return offset() == other.offset() && objectID() == other.objectID() && treeID() == other.treeID(); +} + static Node* nodeAndOffsetForReplacedNode(Node& replacedNode, int& offset, int characterCount) { // Use this function to include the replaced node itself in the range we are creating. @@ -424,6 +429,49 @@ static RefPtr<AXIsolatedObject> findObjectWithRuns(AXIsolatedObject& start, AXDi return nullptr; } +size_t AXTextMarker::offsetFromRoot() const +{ + RELEASE_ASSERT(!isMainThread()); + + if (!isValid()) + return { }; + RefPtr object = isolatedObject(); + auto* tree = object ? object->tree() : nullptr; + if (RefPtr root = tree ? tree->rootNode() : nullptr) { + AXTextMarker rootMarker = { root->treeID(), root->objectID(), 0 }; AG: I dont thing you need = if you are creating the object with an initializer list, i.e., I think you can write AXTextMarker rootMarker { ... }; + size_t offset = 0; + auto current = rootMarker.toTextLeafMarker(); + while (current.isValid() && !hasSameObjectAndOffset(current)) { + current = current.findMarker(AXDirection::Next); + offset++; + } + return hasSameObjectAndOffset(current) ? offset : notFound; + } + return notFound; +} + +AXTextMarker AXTextMarker::nextMarkerFromOffset(size_t offset) const +{ + RELEASE_ASSERT(!isMainThread()); + + if (!isValid()) + return { }; + if (!isInTextLeaf()) + return toTextLeafMarker().nextMarkerFromOffset(offset); + RELEASE_ASSERT(isInTextLeaf()); AG: this assert is redundant since it is returning above if !isInTextLeaf. + + auto marker = *this; + while (offset) { + if (auto newMarker = marker.findMarker(AXDirection::Next)) + marker = WTFMove(newMarker); + else + break; + + offset -= 1; AG: --offset; + } + return marker; +} + String AXTextMarkerRange::toString() const { RELEASE_ASSERT(!isMainThread());
(In reply to Tyler Wilcock from comment #4) > Created attachment 469436 [details] > Patch diff --git a/Source/WebCore/accessibility/AXTextMarker.h b/Source/WebCore/accessibility/AXTextMarker.h index 966405d3eb9f..5df01a9ded98 100644 --- a/Source/WebCore/accessibility/AXTextMarker.h +++ b/Source/WebCore/accessibility/AXTextMarker.h @@ -136,6 +136,7 @@ public: operator VisiblePosition() const; operator CharacterOffset() const; std::optional<BoundaryPoint> boundaryPoint() const; + bool hasSameObjectAndOffset(const AXTextMarker&) const; #if PLATFORM(COCOA) RetainPtr<PlatformTextMarkerData> platformData() const; @@ -170,6 +171,8 @@ public: AXTextMarkerRange lineRange(LineRangeType) const; // Given a character offset relative to this marker, find the next marker the offset points to. AXTextMarker nextMarkerFromOffset(size_t) const; + // Returns the number of intermediate text markers between this and the root. + size_t offsetFromRoot() const; AG: we were using unsigned for offsets, and it seems that we are changing to size_t. Should we be consistent and use one or the other? #endif // ENABLE(AX_THREAD_TEXT_APIS) private:
Created attachment 469449 [details] Patch
(In reply to Andres Gonzalez from comment #7) > + size_t offsetFromRoot() const; > > AG: we were using unsigned for offsets, and it seems that we are changing to > size_t. Should we be consistent and use one or the other? Fixed, I now use unsigned. Also addressed all other comments.
(In reply to Joshua Hoffman from comment #5) > > +function verifyMarkerIndex(previousMarker, textMarker, obj) { > > + var markerRange = obj.textMarkerRangeForMarkers(previousMarker, textMarker); > > + var originalString = replaceLinebreakInString(obj.stringForTextMarkerRange(markerRange)); > > + output += `Original marker string: ${originalString}\n`; > > + > > + var index = obj.indexForTextMarker(textMarker); > > + var newMarker = obj.textMarkerForIndex(index); > > + markerRange = obj.textMarkerRangeForMarkers(previousMarker, newMarker); > > + var newString = replaceLinebreakInString(obj.stringForTextMarkerRange(markerRange)); > > + output += `Index is: ${index}\nNew marker string: ${newString}\n`; > > +} > > Do you anticipate we'd want these three methods in any other tests? Maybe, > like we do for tables, we could have a JS helper file with these. Possibly! This test is basically just a 1 to 1 copy of an existing test, so didn't put much thought into abstracting these out. I'll keep it in mind as an option as I port more tests and write new ones. > > LayoutTests/accessibility/ax-thread-text-apis/text-marker-range-with-unordered-markers.html:30 > > + for (let i = 0; i < 10; ++i) > > + start = text.nextTextMarker(start); > > Would it be possible to abstract these loops into methods, either in a > helper JS file or in AccessibilityUIElement itself? Perhaps something like > `text.nextTextMarker(start, x)`, where x is 10 in this case. Unless I am > missing why iterating here is important? I think it's important to test this step-by-step next text marker movement to make sure we end up at a consistent place, but it's a good point that this could be a helper JS function. This test is a 1 to 1 copy of an existing one, so not looking to change much if possible, but if I see this pattern in other tests I'll abstract it into a helper.
diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm index b9950193f94f..974f7cfb5162 100644 --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm +++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm @@ -31,6 +31,7 @@ #if PLATFORM(MAC) +#import "AXIsolatedObject.h" #import "AXLogger.h" #import "AXObjectCache.h" #import "AXTextMarker.h" @@ -3271,11 +3272,32 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END if ([attribute isEqualToString:AXTextMarkerIsNullAttribute]) return [NSNumber numberWithBool:AXTextMarker(textMarker).isNull()]; - if ([attribute isEqualToString:AXIndexForTextMarkerAttribute]) + if ([attribute isEqualToString:AXIndexForTextMarkerAttribute]) { +#if ENABLE(AX_THREAD_TEXT_APIS) + if (AXObjectCache::useAccessibilityThreadTextApis()) AG: can we shorten the name to useAXThreadTextApis ? Didn't realize you kept the accessibility in the previous patch. + return [NSNumber numberWithUnsignedLong:AXTextMarker { textMarker }.offsetFromRoot()]; +#endif return [NSNumber numberWithInteger:[self _indexForTextMarker:textMarker]]; + } + + if ([attribute isEqualToString:AXTextMarkerForIndexAttribute]) { +#if ENABLE(AX_THREAD_TEXT_APIS) + if (AXObjectCache::useAccessibilityThreadTextApis()) { + long index = [number longValue]; + if (index < 0) + return nil; - if ([attribute isEqualToString:AXTextMarkerForIndexAttribute]) + RefPtr isolatedObject = dynamicDowncast<AXIsolatedObject>(backingObject.get()); + auto* tree = isolatedObject ? isolatedObject->tree() : nullptr; AG: please don't access iso objects directly here' that is contrary to all we have done to encapsulate the core object functionality in AXCoreObject. Until we have a common interface for AXObjectCache and AXIsolatedTree, maybe the way to do this is to add a treeID() method to AXCoreObject, then get the tree using that ID from the AXTreeStore. + if (RefPtr root = tree ? tree->rootNode() : nullptr) { + AXTextMarker rootMarker = { root->treeID(), root->objectID(), 0 }; + return rootMarker.nextMarkerFromOffset(static_cast<size_t>(index)).platformData().bridgingAutorelease(); + } + return nil; + } +#endif // ENABLE(AX_THREAD_TEXT_APIS) return (id)[self _textMarkerForIndex:[number integerValue]]; + } if ([attribute isEqualToString:AXUIElementForTextMarkerAttribute]) { AXTextMarker marker { textMarker }; @@ -3470,7 +3492,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END #if ENABLE(AX_THREAD_TEXT_APIS) if (AXObjectCache::useAccessibilityThreadTextApis()) { AXTextMarker inputMarker { textMarker }; - return inputMarker.findMarker(AXDirection::Next).platformData().bridgingAutorelease(); + return inputMarker.findMarker(AXDirection::Previous).platformData().bridgingAutorelease(); } #endif return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([textMarker = retainPtr(textMarker), protectedSelf = retainPtr(self)] () -> RetainPtr<id> {
Created attachment 469450 [details] Patch
Created attachment 469451 [details] Patch
(In reply to Andres Gonzalez from comment #11) > + if ([attribute isEqualToString:AXIndexForTextMarkerAttribute]) { > +#if ENABLE(AX_THREAD_TEXT_APIS) > + if (AXObjectCache::useAccessibilityThreadTextApis()) > > AG: can we shorten the name to useAXThreadTextApis ? Didn't realize you kept > the accessibility in the previous patch. TW: Fixed. > - if ([attribute isEqualToString:AXTextMarkerForIndexAttribute]) > + RefPtr isolatedObject = > dynamicDowncast<AXIsolatedObject>(backingObject.get()); > + auto* tree = isolatedObject ? isolatedObject->tree() : nullptr; > > AG: please don't access iso objects directly here' that is contrary to all > we have done to encapsulate the core object functionality in AXCoreObject. > Until we have a common interface for AXObjectCache and AXIsolatedTree, maybe > the way to do this is to add a treeID() method to AXCoreObject, then get the > tree using that ID from the AXTreeStore. TW: Great suggestion, thanks! Fixed.
(In reply to Tyler Wilcock from comment #13) > Created attachment 469451 [details] > Patch --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm +++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm @@ -31,6 +31,7 @@ #if PLATFORM(MAC) +#import "AXIsolatedObject.h" AG: do you still need to include this here?
(In reply to Andres Gonzalez from comment #15) > (In reply to Tyler Wilcock from comment #13) > > Created attachment 469451 [details] > > Patch > > --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm > +++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm > @@ -31,6 +31,7 @@ > > #if PLATFORM(MAC) > > +#import "AXIsolatedObject.h" > > AG: do you still need to include this here? TW: Unfortunately so. I tried removing it, but because AXIsolatedTree::rootNode returns an AXIsolatedObject*, we need AXIsolatedObject.h to call any methods on it. /Users/twilco/projects/web/OpenSource/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h:39:7: note: forward declaration of 'WebCore::AXIsolatedObject' class AXIsolatedObject; ^ /Users/twilco/projects/web/OpenSource/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3291:63: error: member access into incomplete type 'WebCore::AXIsolatedObject' AXTextMarker rootMarker { root->treeID(), root->objectID(), 0 }; I tried some other things to make this work (e.g. making the pointer type explicitly AXCoreObject*), but all bottomed out with similar errors. We can fix this when AXObjectCache and AXIsolatedTree share an interface, so that a `rootNode` method on that interface can return an AXCoreObject.
Committed 273234@main (8f9276392840): <https://commits.webkit.org/273234@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 469451 [details].