Try to use TextIterator class to refactor the next/previous text marker functions, so we might be able to avoid some VisiblePosition bugs.
<rdar://problem/24052432>
Created attachment 268269 [details] patch
Comment on attachment 268269 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=268269&action=review > Source/WebCore/ChangeLog:8 > + Implemented a way to navigate to previous/next text marker using Range and TextIterator. can you add a few sentences as to why you're doing this > Source/WebCore/accessibility/AXObjectCache.cpp:1431 > + Node* currNode = nullptr; currNode -> currentNode > Source/WebCore/accessibility/AXObjectCache.cpp:1448 > + if (preNode) if (Node* preNode = previousNode(currNode)) > Source/WebCore/accessibility/AXObjectCache.cpp:1457 > + int currLen = iterator.text().length(); currLen = currentLength > Source/WebCore/accessibility/AXObjectCache.cpp:1469 > + extra newline can be removed > Source/WebCore/accessibility/AXObjectCache.cpp:1509 > + RefPtr<Range> range = Range::create(*document); too many spaces "= Range" > Source/WebCore/accessibility/AXObjectCache.cpp:1512 > + if (ec) return ec ? nullptr : range; > Source/WebCore/accessibility/AXObjectCache.cpp:1626 > + if (node->parentNode() && node->parentNode()->renderer() && node->parentNode()->renderer()->isBody() && !node->previousSibling()) too many spaces between the && "node->parentNode()->renderer() && node->parentNode()->renderer()->isBody()" > Source/WebCore/accessibility/AXObjectCache.h:65 > + CharacterOffset(Node* n, int offset, int remaining) you might be able to put default values for these parameters and then remove the default constructor CharacterOffset() { } --> CharacterOffset(Node* n = nullptr, int offset = 0, int remaining = 0) > Source/WebCore/accessibility/AccessibilityObject.cpp:724 > + return AXObjectCache::rangeForNodeContents(node); seems like you could probably just do return AXObjectCache::rangeForNodeContents(node()); and then handle the nil case inside rangeForNodeContents > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:790 > +static AccessibilityObject* accessibilityObjectForTextMarker(AXObjectCache* cache, CFTypeRef textMarker) these methods might be better off inside WebAXObjectWrapperBase so that they can be used by iOS and OSX > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:830 > +- (id)textMarkerForNode:(Node&)node Offset:(int)offset Offset -> offset > LayoutTests/ChangeLog:7 > + we should add a test for navigation within a user-select:none area
Created attachment 268345 [details] patch review comments
Comment on attachment 268345 [details] patch Working on fixing the iOS build issue and adding support for iOS
Comment on attachment 268345 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=268345&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:1445 > + // Here when we don't have any character to move and we are going backwards, we traverse to previous node. traverse to "the" previous > Source/WebCore/accessibility/AXObjectCache.cpp:1462 > + int suboffset = iterator.range()->startOffset(); suboffset -> subOffset > Source/WebCore/accessibility/AXObjectCache.cpp:1470 > + if (currentLength == 1 && (iterator.text()[0] == ' ' || iterator.text()[0] == '\n' || iterator.text()[0] == '\t')) did this line come from existing code? My guess is that there's some whitespace detection mechanism that will account for other weird things that you don't have here like \r > Source/WebCore/accessibility/AXObjectCache.cpp:1484 > + offsetInCharacter = offset - (offsetSoFar - currentLength); bad spacing after = > Source/WebCore/accessibility/AXObjectCache.cpp:1513 > +void AXObjectCache::startOrEndTextMarkerDataforRange(TextMarkerData& textMarkerData, RefPtr<Range> range, bool isStart) Datafor -> DataFor > Source/WebCore/accessibility/AXObjectCache.cpp:1528 > + if (is<HTMLInputElement>(*domNode) && downcast<HTMLInputElement>(*domNode).isPasswordField()) we should add a layout test variation for the password field case > Source/WebCore/accessibility/AXObjectCache.cpp:1531 > + AXObjectCache* cache = domNode->document().axObjectCache(); do we think that this domNode will have a different AXObjectCache then this axObjectCache that we're in right now? > Source/WebCore/accessibility/AXObjectCache.cpp:1583 > + domNode = charOffset.node; this bit of code is pretty identical to the method above this one. Can we make a helper routine to consolidate the code? > Source/WebCore/accessibility/AXObjectCache.cpp:1669 > + AXObjectCache* cache = domNode->document().axObjectCache(); seems like we should be using this AXObjectCache instead of asking for the cache through the document > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:503 > + if (!textMarker) we should check for !cache case too just to be safe > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3866 > + return [self textMarkerForNode:*textMarkerData.node Offset:textMarkerData.characterOffset + 1]; Offset should be lowercase > LayoutTests/accessibility/mac/previous-next-text-marker.html:96 > + shouldBeTrue("text3.accessibilityElementForTextMarker(currentMarker).isEqual(text3)"); let's add a check here that does 1. iterate into the user-select: none text 2. we get two markers that represent positions within user-select:none (like from "s" -> "l" or something) 3. check the string that we get back from this range and verify text is correct 4. then let's iterate backwards from that 2nd node and get another range where we verify the string thanks
Created attachment 268903 [details] patch - Changed a lot of code to implement stringForTextMarkerRange call - Fixed an issue where text node's start offset is not always 0 Since the patch is already very large, I'll put the iOS implementation in another bug.
Created attachment 268922 [details] patch Fixed some issue in rangeForUnorderedCharacterOffsets.
Comment on attachment 268922 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=268922&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:1457 > + for (; !iterator.atEnd(); iterator.advance()) { seems like int count = 1 and count++ can be part of the for loop > Source/WebCore/accessibility/AXObjectCache.cpp:1466 > + if (AccessibilityObject::replacedNodeNeedsCharacter(node.traverseToChildAt(subOffset))) { you can probably cache node.traverseToChildAt(subOffset) in a local variable so you don't have to call it twice > Source/WebCore/accessibility/AXObjectCache.cpp:1479 > + if (!currentLength) this check could be put up as the else for the if block at 1466 right? > Source/WebCore/accessibility/AXObjectCache.cpp:1562 > + // endOffset can be out of bound sometimes if the node is a replaced node or has brTag. out of "bounds" > Source/WebCore/accessibility/AXObjectCache.cpp:1565 > + if (endOffset > TextIterator::rangeLength(nodeRange.get())) you can store TextIterator::rangeLength(nodeRange.get()) so you don't have to call it twice > Source/WebCore/accessibility/AXObjectCache.h:55 > + int characterOffset; what is startIndex and what is ignored used for? > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:805 > + return nil; nullptr since it's a C++ object > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:847 > + if (isTextMarkerIgnored(textMarker)) doesn't this need to be in a while loop > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:856 > + if (isTextMarkerIgnored(textMarker)) doesn't this need to be in a while loop > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:877 > + if (!textMarkerData.axID && !textMarkerData.ignored) can we use axID = 0 to mean ignored? > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:886 > + return nil; nullptr since its C++ > LayoutTests/accessibility/mac/previous-next-text-marker.html:117 > + shouldBeTrue("!psw.accessibilityElementForTextMarker(start)"); shouldBeFalse
Comment on attachment 268922 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=268922&action=review I'll address other comments. >> Source/WebCore/accessibility/AXObjectCache.h:55 >> + int characterOffset; > > what is startIndex and what is ignored used for? In the range of a text node's content, the start offset is not always 0. So I used (startIndex + character count) to set the correct start offset when creating a range from text marker range. Ignored is used here for password field, so we can skip that when calling next/previous text marker call. >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:847 >> + if (isTextMarkerIgnored(textMarker)) > > doesn't this need to be in a while loop I'm calling nextTextMarkerForNode:, not textMarkerForNode:. So it's recursive. >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:856 >> + if (isTextMarkerIgnored(textMarker)) > > doesn't this need to be in a while loop ditto. >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:877 >> + if (!textMarkerData.axID && !textMarkerData.ignored) > > can we use axID = 0 to mean ignored? We also return an empty TextMarkerData when node is nullptr. In that case we want to return nil for the text marker. For example we get to the end of document that we can't ignore it and keep going forward. But here for password filed, we want to treat it differently, so that we know the node exists and there might be nodes after it.
Comment on attachment 268922 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=268922&action=review >> LayoutTests/accessibility/mac/previous-next-text-marker.html:117 >> + shouldBeTrue("!psw.accessibilityElementForTextMarker(start)"); > > shouldBeFalse I tried to use shouldBeNull, but DumpRenderTree is returning a undefined object which will cause the check fail. So I kept it this way.
(In reply to comment #11) > Comment on attachment 268922 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268922&action=review > > >> LayoutTests/accessibility/mac/previous-next-text-marker.html:117 > >> + shouldBeTrue("!psw.accessibilityElementForTextMarker(start)"); > > > > shouldBeFalse > > I tried to use shouldBeNull, but DumpRenderTree is returning a undefined > object which will cause the check fail. So I kept it this way. ok no worries
Created attachment 269022 [details] patch Update from review comments.
Comment on attachment 269022 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=269022&action=review > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:877 > + if (!textMarkerData.axID && !textMarkerData.ignored) should this be if (!textMarkerData.axID || !textMarkerData.ignored)
i also wrote a test for this a million years ago in https://bugs.webkit.org/attachment.cgi?id=209142&action=review can you include that test in this patch as well for more coverage?
(In reply to comment #14) > Comment on attachment 269022 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269022&action=review > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:877 > > + if (!textMarkerData.axID && !textMarkerData.ignored) > > should this be if (!textMarkerData.axID || !textMarkerData.ignored) For the ignored element, I only set the ignored property, its axID is still 0. So || will return a nil which is not what we want. For a zeroed TextMarketData, the ignored property is false, so the empty data is not ignored by default.
(In reply to comment #15) > i also wrote a test for this a million years ago in > https://bugs.webkit.org/attachment.cgi?id=209142&action=review > > can you include that test in this patch as well for more coverage? Ok, will do.
Created attachment 269234 [details] patch Added text-marker-with-user-select-none.html test. Since the indexForTextMarker function is using VisiblePosition code and it's not working fine with user-select:none, I removed that portion from the test.
Comment on attachment 269234 [details] patch Clearing flags on attachment: 269234 Committed r195240: <http://trac.webkit.org/changeset/195240>
All reviewed patches have been landed. Closing bug.