Improve accessibility text navigation Change the bounds/position and index related text marker calls to use TextIterator.
<rdar://problem/24269521>
Created attachment 272834 [details] patch Initial patch.
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 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.
Created attachment 273256 [details] patch review comments.
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 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.
Created attachment 273659 [details] patch Moved axObjectCache() check to the top of accessibilityAttributeValue: method.
Comment on attachment 273659 [details] patch Clearing flags on attachment: 273659 Committed r197982: <http://trac.webkit.org/changeset/197982>
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]
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.