Bug 154366 – AX: AXObjectCache::visiblePositionForTextMarkerData() doesn't account for equivalent visibly equivalent positions

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

[Doug Russell]

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.

[Radar WebKit Bug Importer]

<rdar://problem/24710269>

[Doug Russell]

Created attachment 271607 [details]
Patch

[Doug Russell]

Created attachment 271611 [details]
Patch

[chris fleizach]

Comment on attachment 271611 [details]
Patch

I think we need a test for this. We're adding new behavior here

[chris fleizach]

Comment on attachment 271611 [details]
Patch

I think we need a test for this. We're adding new behavior here

[Ryosuke Niwa]

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

[Doug Russell]

(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?

[Ryosuke Niwa]

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.

[Doug Russell]

So scrap the comparison and trust the result of creating VisiblePosition? Sounds good to me.

[Doug Russell]

Created attachment 271621 [details]
Patch

[Doug Russell]

Created attachment 271762 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 271762 [details]
Patch

Clearing flags on attachment: 271762

Committed r196853: <http://trac.webkit.org/changeset/196853>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.