Bug 154976

Summary: AX: Implement bounds/position and index related text marker functions using TextIterator
Product: WebKit Reporter: Nan Wang <n_wang>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: bfulgham, bshafiei, commit-queue, n_wang, ryanhaddad, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 155346    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch
cfleizach: review+
patch none

Description Nan Wang 2016-03-03 10:58:28 PST
Improve accessibility text navigation
Change the bounds/position and index related text marker calls to use TextIterator.
Comment 1 Nan Wang 2016-03-03 10:58:38 PST
<rdar://problem/24269521>
Comment 2 Nan Wang 2016-03-03 22:46:34 PST
Created attachment 272834 [details]
patch

Initial patch.
Comment 3 chris fleizach 2016-03-07 16:04:02 PST
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?
Comment 4 Nan Wang 2016-03-07 16:14:41 PST
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.
Comment 5 Nan Wang 2016-03-07 18:31:23 PST
Created attachment 273256 [details]
patch

review comments.
Comment 6 chris fleizach 2016-03-09 14:45:49 PST
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
Comment 7 Nan Wang 2016-03-09 14:50:46 PST
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.
Comment 8 Nan Wang 2016-03-10 17:33:36 PST
Created attachment 273659 [details]
patch

Moved axObjectCache() check to the top of accessibilityAttributeValue: method.
Comment 9 WebKit Commit Bot 2016-03-10 18:37:35 PST
Comment on attachment 273659 [details]
patch

Clearing flags on attachment: 273659

Committed r197982: <http://trac.webkit.org/changeset/197982>
Comment 10 Alexey Proskuryakov 2016-03-10 20:14:10 PST
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]
Comment 11 Nan Wang 2016-03-10 21:02:26 PST
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.