[Webkit-unassigned] [Bug 154366] AX: AXObjectCache::visiblePositionForTextMarkerData() doesn't account for equivalent visibly equivalent positions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=154366

--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 271611
  --> https://bugs.webkit.org/attachment.cgi?id=271611
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160218/ab1708a9/attachment.html>