Bug 154976 – AX: Implement bounds/position and index related text marker functions using TextIterator

• Component: Accessibility
• Status: NEW

[Nan Wang]

Improve accessibility text navigation
Change the bounds/position and index related text marker calls to use TextIterator.

[Nan Wang]

<rdar://problem/24269521>

[Nan Wang]

Created attachment 272834 [details]
patch

Initial patch.

[chris fleizach]

Comment on attachment 272834 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272834&action=review

> Source/WebCore/ChangeLog:9
> +        VoiceOver navigation issue.

issues.

what are those issues?

> Source/WebCore/accessibility/AXObjectCache.cpp:2329
> +    InlineBox* inlineBox;

initialize to nullptr just in case the getInlineBox doesn't do it

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1967
> +    LayoutRect ourrect = rect1;

ourRect

> LayoutTests/ChangeLog:8
> +        * accessibility/mac/text-marker-for-index-expected.txt: Added.

do you have test for textMarkerForPosition, line navigation and text marker bounds?

[Nan Wang]

Comment on attachment 272834 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272834&action=review

>> Source/WebCore/ChangeLog:9
>> +        VoiceOver navigation issue.
> 
> issues.
> 
> what are those issues?

Like word boundary for multilanguage, left/right word should consider the space at node boundary, nextCharacterOffset should match VisiblePosition behavior. These are all covered in modified tests.

>> Source/WebCore/accessibility/AXObjectCache.cpp:2329
>> +    InlineBox* inlineBox;
> 
> initialize to nullptr just in case the getInlineBox doesn't do it

Ok.

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1967
>> +    LayoutRect ourrect = rect1;
> 
> ourRect

Ok

>> LayoutTests/ChangeLog:8
>> +        * accessibility/mac/text-marker-for-index-expected.txt: Added.
> 
> do you have test for textMarkerForPosition, line navigation and text marker bounds?

Position and bounds are covered by existing tests. Line navigation has not been implemented yet.

[Nan Wang]

Created attachment 273256 [details]
patch

review comments.

[chris fleizach]

Comment on attachment 273256 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273256&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:2352
> +    RenderObject* renderer;

i would initialize this to nullptr just to be safe

> Source/WebCore/accessibility/AXObjectCache.cpp:2374
> +

extra line not necessary

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3983
> +            IntRect webCoreRect = [self screenToContents:enclosingIntRect(rect)];

this is the same exact code for StartText and EndText... is that correct? if so can we combine

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4064
> +        if (!cache)

it feels like we should put

AXObjectCache* cache = m_object->axObjectCache();
 4064        if (!cache)

near the top of this accessibilityAttributeValue: method so that we don't have to do all these checks

if an object does not have a cache it seems OK to return nil values for it

[Nan Wang]

Comment on attachment 273256 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273256&action=review

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3983
>> +            IntRect webCoreRect = [self screenToContents:enclosingIntRect(rect)];
> 
> this is the same exact code for StartText and EndText... is that correct? if so can we combine

cache->characterOffsetForBounds(webCoreRect, true) has a boolean parameter to check if it's start or end.

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4064
>> +        if (!cache)
> 
> it feels like we should put
> 
> AXObjectCache* cache = m_object->axObjectCache();
>  4064        if (!cache)
> 
> near the top of this accessibilityAttributeValue: method so that we don't have to do all these checks
> 
> if an object does not have a cache it seems OK to return nil values for it

Ok, I'll double check.

[Nan Wang]

Created attachment 273659 [details]
patch

Moved axObjectCache() check to the top of accessibilityAttributeValue: method.

[WebKit Commit Bot]

Comment on attachment 273659 [details]
patch

Clearing flags on attachment: 273659

Committed r197982: <http://trac.webkit.org/changeset/197982>

[Alexey Proskuryakov]

This broke Windows build, please fix ASAP. 

In the future, please always wait for EWS to finish before landing WebKit patches.

C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/PassRefPtr.h(42): error C2027: use of undefined type 'WebCore::Range' (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\accessibility\win\AccessibilityObjectWin.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\dom\Position.h(43): note: see declaration of 'WebCore::Range' (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\accessibility\win\AccessibilityObjectWin.cpp)
  C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/RefPtr.h(59): note: see reference to function template instantiation 'void WTF::derefIfNotNull<T>(T *)' being compiled
          with
          [
              T=WebCore::Range
          ] (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\accessibility\win\AccessibilityObjectWin.cpp)
  C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/RefPtr.h(59): note: while compiling class template member function 'WTF::RefPtr<WebCore::Range>::~RefPtr(void)' (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\accessibility\win\AccessibilityObjectWin.cpp)
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\accessibility\AccessibilityObject.h(844): note: see reference to function template instantiation 'WTF::RefPtr<WebCore::Range>::~RefPtr(void)' being compiled (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\accessibility\win\AccessibilityObjectWin.cpp)
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\accessibility\AccessibilityObject.h(844): note: see reference to class template instantiation 'WTF::RefPtr<WebCore::Range>' being compiled (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\accessibility\win\AccessibilityObjectWin.cpp)
C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/PassRefPtr.h(42): error C2227: left of '->deref' must point to class/struct/union/generic type (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\accessibility\win\AccessibilityObjectWin.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

[Nan Wang]

I was planning to rollout the patch. Seems this has been fixed in https://trac.webkit.org/changeset/197991

Thanks, will pay attention of this kind of build issues in the future.