Bug 97246 – AX: Chromium doesn't pass accessibility text range and line number tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Dominic Mazzoni]

These four tests are skipped by chromium currently, we should finish the implementation and make these tests pass:

BUGCR10322 SKIP : accessibility/textarea-insertion-point-line-number.html = TEXT
BUGCR10322 SKIP : accessibility/textarea-line-for-index.html = TEXT
BUGCR10322 SKIP : accessibility/textarea-selected-text-range.html = TEXT
BUGCR10322 SKIP : accessibility/textbox-role-reports-selection.html = TEXT

[Dominic Mazzoni]

Created attachment 164974 [details]
Patch

[Dominic Mazzoni]

Created attachment 165146 [details]
Rebase

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Dominic Mazzoni]

Chris, this should be ready for review when you have a chance - thanks.

[Eric Seidel (no email)]

Comment on attachment 165146 [details]
Rebase

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

> Source/WebCore/ChangeLog:9
> +        Adds a method to get all line breaks at once, so retrieving all
> +        line breaks isn't an O(n^2) operation for Chromium.

Is it O(N^2) for other platforms? :)

[Dominic Mazzoni]

(In reply to comment #5)
> (From update of attachment 165146 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165146&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Adds a method to get all line breaks at once, so retrieving all
> > +        line breaks isn't an O(n^2) operation for Chromium.
> 
> Is it O(N^2) for other platforms? :)

Good question.

Short answer: no, this optimization is only needed for Chromium.

Longer answer:

In the Mac port of WebKit, with VoiceOver, the line break functions are called as needed when a user is interacting with something like a textarea. Each call to get the line number is O(N) in the number of lines, but there are only a constant number of such calls per character typed. The GTK port is the same.

In Chromium, we cache the information about the line breaks in the browser process so that screen reader queries about line breaks can be satisfied from the cache without waiting on an IPC to the renderer - so we have to pay the penalty of computing all of the line breaks each time the textbox is modified.

[chris fleizach]

Comment on attachment 165146 [details]
Rebase

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

mostly minor nits. i think this looks ok once those are addressed

> Source/WebCore/accessibility/AccessibilityObject.h:637
> +    virtual void allLineBreaks(Vector<int>&) const { }

i think "all" is implicit in the name. Just naming it "lineBreaks" feels like it's enough

> Source/WebKit/chromium/src/WebAccessibilityObject.cpp:770
> +    for (size_t i = 0; i< lineBreaksVector.size(); i++)

i would move .size() call out of for loop so it doesn't have to be called each iteration

> Tools/DumpRenderTree/chromium/TestRunner/AccessibilityUIElementChromium.cpp:647
> +    while (line < static_cast<int>(lineBreaks.size()) && lineBreaks[line] <= index)

move .size() call out of loop

[Dominic Mazzoni]

Created attachment 165637 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 165637 [details]
Patch for landing

Clearing flags on attachment: 165637

Committed r129527: <http://trac.webkit.org/changeset/129527>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.