Bug 154366

Summary: AX: AXObjectCache::visiblePositionForTextMarkerData() doesn't account for equivalent visibly equivalent positions
Product: WebKit Reporter: Doug Russell <d_russell>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, jcraig, jdiggs, mario, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Doug Russell 2016-02-17 16:38:27 PST
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.
Comment 1 Radar WebKit Bug Importer 2016-02-17 16:39:00 PST
<rdar://problem/24710269>
Comment 2 Doug Russell 2016-02-17 16:44:17 PST
Created attachment 271607 [details]
Patch
Comment 3 Doug Russell 2016-02-17 18:16:43 PST
Created attachment 271611 [details]
Patch
Comment 4 chris fleizach 2016-02-17 19:37:40 PST
Comment on attachment 271611 [details]
Patch

I think we need a test for this. We're adding new behavior here
Comment 5 chris fleizach 2016-02-17 19:37:49 PST
Comment on attachment 271611 [details]
Patch

I think we need a test for this. We're adding new behavior here
Comment 6 Ryosuke Niwa 2016-02-17 19:40:10 PST
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
Comment 7 Doug Russell 2016-02-17 19:41:53 PST
(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 8 Ryosuke Niwa 2016-02-17 19:51:07 PST
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.
Comment 9 Doug Russell 2016-02-17 19:52:28 PST
So scrap the comparison and trust the result of creating VisiblePosition? Sounds good to me.
Comment 10 Doug Russell 2016-02-17 20:26:13 PST
Created attachment 271621 [details]
Patch
Comment 11 Doug Russell 2016-02-19 10:11:52 PST
Created attachment 271762 [details]
Patch
Comment 12 WebKit Commit Bot 2016-02-19 20:17:11 PST
Comment on attachment 271762 [details]
Patch

Clearing flags on attachment: 271762

Committed r196853: <http://trac.webkit.org/changeset/196853>
Comment 13 WebKit Commit Bot 2016-02-19 20:17:16 PST
All reviewed patches have been landed.  Closing bug.