Bug 153939 – AX: Implement word related text marker functions using TextIterator

• Component: Accessibility
• Status: RESOLVED FIXED

[Nan Wang]

Improve accessibility text navigation.
Change the word related text marker calls to use TextIterator.

[Nan Wang]

<rdar://problem/24269605>

[Radar WebKit Bug Importer]

<rdar://problem/24538779>

[Nan Wang]

Created attachment 270791 [details]
patch

The word related text marker tests are not included in DumpRenderTree, not sure if we still need to support that.
I'll add those to DumpRenderTree later if the tests failed.

[Build Bot]

Comment on attachment 270791 [details]
patch

Attachment 270791 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/790308

New failing tests:
accessibility/mac/text-marker-word-nav.html

[Build Bot]

Created attachment 270793 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 270791 [details]
patch

Attachment 270791 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/790339

New failing tests:
accessibility/mac/text-marker-word-nav.html

[Build Bot]

Created attachment 270794 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Nan Wang]

Created attachment 270801 [details]
patch

Build fix and test support on DumpRenderTree.

[Nan Wang]

Created attachment 270802 [details]
patch

Another build fix.

[Nan Wang]

Created attachment 270803 [details]
patch

build fix

[chris fleizach]

Comment on attachment 270803 [details]
patch

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

> Source/WebCore/ChangeLog:10
> +        logics from previousBoundary and nextBoundary in VisibleUnits class.

... reused logic from ...

> Source/WebCore/accessibility/AXObjectCache.cpp:1554
> +    return AccessibilityObject::replacedNodeNeedsCharacter(node) || node->hasTagName(brTag);

do you need to worry that node is nullptr here

> Source/WebCore/editing/VisibleUnits.h:118
> +WEBCORE_EXPORT unsigned forwardSearchForBoundaryWithTextIterator(TextIterator&, Vector<UChar, 1024>&, unsigned, BoundarySearchFunction);

these probably don't need to be WEBCORE_EXPORT, since the only people using are within WebCore

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:979
> +    AccessibilityTextMarker* marker = 0;

nullptr and ditto for everything below

> LayoutTests/accessibility/mac/text-marker-word-nav.html:14
> +Test Content<span id="target" contenteditable="true">editable is working.</span>

can you add a test for content that is <pre> with line breaks, and then some content with <br> inside

> LayoutTests/accessibility/mac/text-marker-word-nav.html:18
> +c <img src="#" aria-label="blah" style="background-color: #aaaaaa; width: 100px; height: 100px;">d

can you add a test for an <audio> or <video> element

> LayoutTests/accessibility/mac/text-marker-word-nav.html:24
> +å·§ååæ¯é£ç©åï¼

which language is this? 
can you also add arabic

> LayoutTests/accessibility/mac/text-marker-word-nav.html:78
> +        function advanceAndVerify(currentMarker, offset, obj) {

can you add a test that runs from the start of the document to the end and the back again in reverse with word markers. that way we get some coverage of bigger movements

[Nan Wang]

Comment on attachment 270803 [details]
patch

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

Will address other issues.

>> LayoutTests/accessibility/mac/text-marker-word-nav.html:24
>> +å·§ååæ¯é£ç©åï¼
> 
> which language is this? 
> can you also add arabic

This is Chinese, probably this page is not utf-8 encoded.

[Nan Wang]

Created attachment 270951 [details]
patch

[chris fleizach]

Comment on attachment 270951 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1568
> +    return AccessibilityObject::replacedNodeNeedsCharacter(node) || node->hasTagName(brTag);

you can probably make this into one line

return node && ...

> Source/WebCore/accessibility/AXObjectCache.cpp:1724
> +        CharacterOffset co = CharacterOffset();

i would change co -> charOffset

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4078
> +        CharacterOffset characterOffset = [self characterOffsetForTextMarker:textMarker];

you can move the characterOffset creation after the check for the cache, since it's not needed until then

[Nan Wang]

Created attachment 270959 [details]
patch

Addressed review comments.

[Nan Wang]

Created attachment 270962 [details]
patch

fixed some issue in previous patch.

[WebKit Commit Bot]

Comment on attachment 270962 [details]
patch

Clearing flags on attachment: 270962

Committed r196352: <http://trac.webkit.org/changeset/196352>