visiblePositionForTextMarkerData transforms TextMarkerData into a VisiblePosition. As a sanity check it compares the node and offset values of the position returned by createLegacyEditingPosition() with the deepEquivalent() of the resulting VisiblePosition. This doesn't account for canonicalization of the position via Position::upstream and Position::downstream causing the position to incorrectly be considered invalid.
<rdar://problem/24710269>
Created attachment 271607 [details] Patch
Created attachment 271611 [details] Patch
Comment on attachment 271611 [details] Patch I think we need a test for this. We're adding new behavior here
Comment on attachment 271611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271611&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:1416 > + Position deepEquivalent = visiblePosition.deepEquivalent(); > + bool equivalent = deepEquivalent.deprecatedNode() == textMarkerData.node && deepEquivalent.deprecatedEditingOffset() == textMarkerData.offset; > + if (!equivalent && visiblePosition.affinity() != textMarkerData.affinity) { > + Position position; > + if (textMarkerData.affinity == UPSTREAM) > + position = deepEquivalent.upstream(); > + else > + position = deepEquivalent.downstream(); > + if (position.deprecatedNode() == textMarkerData.node && position.deprecatedEditingOffset() == textMarkerData.offset) > + equivalent = true; > + } > + return equivalent; Instead of manually adjusting the visible position's deep equivalent, you should just create a new VisiblePosition out of textMarkerData and check whether they match or not. By the way, '&' should be directly adjacent to type in C++ code: https://webkit.org/code-style-guidelines/#pointers-and-references
(In reply to comment #6) > Comment on attachment 271611 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271611&action=review > > > Source/WebCore/accessibility/AXObjectCache.cpp:1416 > > + Position deepEquivalent = visiblePosition.deepEquivalent(); > > + bool equivalent = deepEquivalent.deprecatedNode() == textMarkerData.node && deepEquivalent.deprecatedEditingOffset() == textMarkerData.offset; > > + if (!equivalent && visiblePosition.affinity() != textMarkerData.affinity) { > > + Position position; > > + if (textMarkerData.affinity == UPSTREAM) > > + position = deepEquivalent.upstream(); > > + else > > + position = deepEquivalent.downstream(); > > + if (position.deprecatedNode() == textMarkerData.node && position.deprecatedEditingOffset() == textMarkerData.offset) > > + equivalent = true; > > + } > > + return equivalent; > > Instead of manually adjusting the visible position's deep equivalent, you > should just create a new VisiblePosition out of textMarkerData and check > whether they match or not. > > By the way, '&' should be directly adjacent to type in C++ code: > https://webkit.org/code-style-guidelines/#pointers-and-references The VisiblePosition I have was created from the TextMarkerData so it would always match. I was considering making VisiblePosition::canonicalPosition() a public static method instead of this manual adjustment. What do you think of that?
Comment on attachment 271611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271611&action=review > Source/WebCore/ChangeLog:8 > + Existing tests coverage should be sufficient. This is definitely not true. There are no tests that got fixed by this change and there is no new test in this patch. Please write a accessibility test that gets affected by this code change, or a justification as to why writing such a test is hard/impossible. >>> Source/WebCore/accessibility/AXObjectCache.cpp:1416 >>> + return equivalent; >> >> Instead of manually adjusting the visible position's deep equivalent, you should just create a new VisiblePosition out of textMarkerData and check whether they match or not. >> >> By the way, '&' should be directly adjacent to type in C++ code: https://webkit.org/code-style-guidelines/#pointers-and-references > > The VisiblePosition I have was created from the TextMarkerData so it would always match. I was considering making VisiblePosition::canonicalPosition() a public static method instead of this manual adjustment. What do you think of that? I'd suggest getting rid of this function altogether. Since the only interesting thing VisiblePosition's constructor does is to call canonicalPosition(), calling canonicalPosition here and comparing the result will be a slightly-incorrect tautological check that doesn't buy us anything.
So scrap the comparison and trust the result of creating VisiblePosition? Sounds good to me.
Created attachment 271621 [details] Patch
Created attachment 271762 [details] Patch
Comment on attachment 271762 [details] Patch Clearing flags on attachment: 271762 Committed r196853: <http://trac.webkit.org/changeset/196853>
All reviewed patches have been landed. Closing bug.