Bug 153939 - AX: Implement word related text marker functions using TextIterator
Summary: AX: Implement word related text marker functions using TextIterator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-02-06 02:07 PST by Nan Wang
Modified: 2016-02-10 16:58 PST (History)
6 users (show)

See Also:


Attachments
patch (63.77 KB, patch)
2016-02-06 02:47 PST, Nan Wang
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (906.18 KB, application/zip)
2016-02-06 03:34 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (814.61 KB, application/zip)
2016-02-06 03:56 PST, Build Bot
no flags Details
patch (76.05 KB, patch)
2016-02-06 11:33 PST, Nan Wang
no flags Details | Formatted Diff | Diff
patch (76.70 KB, patch)
2016-02-06 12:59 PST, Nan Wang
no flags Details | Formatted Diff | Diff
patch (76.24 KB, patch)
2016-02-06 13:14 PST, Nan Wang
no flags Details | Formatted Diff | Diff
patch (80.92 KB, patch)
2016-02-09 14:04 PST, Nan Wang
cfleizach: review+
Details | Formatted Diff | Diff
patch (80.98 KB, patch)
2016-02-09 16:11 PST, Nan Wang
no flags Details | Formatted Diff | Diff
patch (80.94 KB, patch)
2016-02-09 16:31 PST, Nan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2016-02-06 02:07:14 PST
Improve accessibility text navigation.
Change the word related text marker calls to use TextIterator.
Comment 1 Nan Wang 2016-02-06 02:07:26 PST
<rdar://problem/24269605>
Comment 2 Radar WebKit Bug Importer 2016-02-06 02:07:26 PST
<rdar://problem/24538779>
Comment 3 Nan Wang 2016-02-06 02:47:35 PST
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.
Comment 4 Build Bot 2016-02-06 03:34:48 PST
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
Comment 5 Build Bot 2016-02-06 03:34:51 PST
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
Comment 6 Build Bot 2016-02-06 03:56:25 PST
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
Comment 7 Build Bot 2016-02-06 03:56:27 PST
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
Comment 8 Nan Wang 2016-02-06 11:33:56 PST
Created attachment 270801 [details]
patch

Build fix and test support on DumpRenderTree.
Comment 9 Nan Wang 2016-02-06 12:59:57 PST
Created attachment 270802 [details]
patch

Another build fix.
Comment 10 Nan Wang 2016-02-06 13:14:22 PST
Created attachment 270803 [details]
patch

build fix
Comment 11 chris fleizach 2016-02-08 17:37:37 PST
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
Comment 12 Nan Wang 2016-02-08 17:49:04 PST
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.
Comment 13 Nan Wang 2016-02-09 14:04:35 PST
Created attachment 270951 [details]
patch
Comment 14 chris fleizach 2016-02-09 15:53:39 PST
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
Comment 15 Nan Wang 2016-02-09 16:11:36 PST
Created attachment 270959 [details]
patch

Addressed review comments.
Comment 16 Nan Wang 2016-02-09 16:31:05 PST
Created attachment 270962 [details]
patch

fixed some issue in previous patch.
Comment 17 WebKit Commit Bot 2016-02-09 18:33:06 PST
Comment on attachment 270962 [details]
patch

Clearing flags on attachment: 270962

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