Bug 141862 – AX: Expose caret browsing preference to accessibility API

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

[Doug Russell]

Exposing the caret browsing setting on WebCore::Frame via the accessibility API would allow assistive tech apps to enable it contextually (for example, when the assistive tech app is running).

[Radar WebKit Bug Importer]

<rdar://problem/19912263>

[Doug Russell]

Created attachment 247025 [details]
Patch

[chris fleizach]

Comment on attachment 247025 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.h:978
> +    virtual bool caretBrowsingEnabled() const { return false; }

I would just put these implementations into AccessibilityObject and remove from AccessibilityRenderObject. It doesn't seem like there's a benefit to making those virtual

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:1404
> +    return 0;

return nullptr

> LayoutTests/platform/mac/accessibility/caret-browsing-attribute.html:13
> +    var webArea = accessibilityController.focusedElement;

the body focus() idiom i think might have some issues on GTK. I usually prefer accessibilityController.rootElement.childAtIndex(0) I think. Or maybe the rootElement is the web area

> LayoutTests/platform/mac/accessibility/caret-browsing-tab-selection.html:22
> +            shouldBe("selectedElement(webArea)", "");

can you (in general) put some new lines between tests units, to make it a bit easier parse

> LayoutTests/platform/mac/accessibility/resources/accessibility-helper.js:11
> +function selectedElement(webArea) {

can you name this a little better so its clear its related to text marker selection

[Doug Russell]

(In reply to comment #3)
> Comment on attachment 247025 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247025&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.h:978
> > +    virtual bool caretBrowsingEnabled() const { return false; }
> 
> I would just put these implementations into AccessibilityObject and remove
> from AccessibilityRenderObject. It doesn't seem like there's a benefit to
> making those virtual

The methods are virtual because AccessibilityObject doesn't know about the Frame that caretBrowsingEnabled is ultimately set on.

FrameView* AccessibilityRenderObject::documentFrameView() const

> 
> > Tools/DumpRenderTree/AccessibilityUIElement.cpp:1404
> > +    return 0;
> 
> return nullptr

Wil do

> 
> > LayoutTests/platform/mac/accessibility/caret-browsing-attribute.html:13
> > +    var webArea = accessibilityController.focusedElement;
> 
> the body focus() idiom i think might have some issues on GTK. I usually
> prefer accessibilityController.rootElement.childAtIndex(0) I think. Or maybe
> the rootElement is the web area

Wil do

> 
> > LayoutTests/platform/mac/accessibility/caret-browsing-tab-selection.html:22
> > +            shouldBe("selectedElement(webArea)", "");
> 
> can you (in general) put some new lines between tests units, to make it a
> bit easier parse

Wil do

> 
> > LayoutTests/platform/mac/accessibility/resources/accessibility-helper.js:11
> > +function selectedElement(webArea) {
> 
> can you name this a little better so its clear its related to text marker
> selection

Does

function elementAtStartMarkerOfSelectedTextMarkerRange(webArea)

work as far as clarity?

[Build Bot]

Comment on attachment 247025 [details]
Patch

Attachment 247025 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5191210964615168

New failing tests:
platform/mac/accessibility/caret-browsing-tab-selection.html
platform/mac/accessibility/caret-browsing-arrow-nav.html
platform/mac/accessibility/caret-browsing-attribute.html
accessibility/parent-delete.html

[Build Bot]

Created attachment 247031 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 247025 [details]
Patch

Attachment 247025 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4911595809406976

New failing tests:
accessibility/parent-delete.html

[Build Bot]

Created attachment 247032 [details]
Archive of layout-test-results from ews101 for mac-mavericks

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

[Doug Russell]

Created attachment 247035 [details]
Patch

[Build Bot]

Comment on attachment 247035 [details]
Patch

Attachment 247035 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5173335411392512

New failing tests:
platform/mac/accessibility/caret-browsing-tab-selection.html
platform/mac/accessibility/caret-browsing-arrow-nav.html
platform/mac/accessibility/caret-browsing-attribute.html

[Build Bot]

Created attachment 247036 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[chris fleizach]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 247025 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=247025&action=review
> > 
> > > Source/WebCore/accessibility/AccessibilityObject.h:978
> > > +    virtual bool caretBrowsingEnabled() const { return false; }
> > 
> > I would just put these implementations into AccessibilityObject and remove
> > from AccessibilityRenderObject. It doesn't seem like there's a benefit to
> > making those virtual
> 
> The methods are virtual because AccessibilityObject doesn't know about the
> Frame that caretBrowsingEnabled is ultimately set on.
> 
> FrameView* AccessibilityRenderObject::documentFrameView() const
> 

documentFrameView exists on AccessibilityObject.h

virtual FrameView* documentFrameView() const;

it seems like this can be

> > 
> > > Tools/DumpRenderTree/AccessibilityUIElement.cpp:1404
> > > +    return 0;
> > 
> > return nullptr
> 
> Wil do
> 
> > 
> > > LayoutTests/platform/mac/accessibility/caret-browsing-attribute.html:13
> > > +    var webArea = accessibilityController.focusedElement;
> > 
> > the body focus() idiom i think might have some issues on GTK. I usually
> > prefer accessibilityController.rootElement.childAtIndex(0) I think. Or maybe
> > the rootElement is the web area
> 
> Wil do
> 
> > 
> > > LayoutTests/platform/mac/accessibility/caret-browsing-tab-selection.html:22
> > > +            shouldBe("selectedElement(webArea)", "");
> > 
> > can you (in general) put some new lines between tests units, to make it a
> > bit easier parse
> 
> Wil do
> 
> > 
> > > LayoutTests/platform/mac/accessibility/resources/accessibility-helper.js:11
> > > +function selectedElement(webArea) {
> > 
> > can you name this a little better so its clear its related to text marker
> > selection
> 
> Does
> 
> function elementAtStartMarkerOfSelectedTextMarkerRange(webArea)
> 
> work as far as clarity?

sounds good

[chris fleizach]

Comment on attachment 247025 [details]
Patch

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

> Tools/ChangeLog:18
> +        (AccessibilityUIElement::setCaretBrowsingEnabled):

I think tests are failing because you need to make these changes in WebKitTestRunner as well (the WK2 testing tool)

[Doug Russell]

Created attachment 247170 [details]
Patch

[chris fleizach]

Comment on attachment 247170 [details]
Patch

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

Looks like its failing on linux
../../Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:218:63: error: no 'void WTR::AccessibilityUIElement::clearSelectedTextMarkerRange()' member function declared in class 'WTR::AccessibilityUIElement'
 void AccessibilityUIElement::clearSelectedTextMarkerRange(void) { }

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:1413
> +void AccessibilityUIElement::resetSelectedTextMarkerRange(void)

the (void) as an argument should be unnecessary

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:1703
> +        { "caretBrowsingEnabled", getCaretBrowsingEnabledCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },

caretBrowsingEnabled should probably be a property rather than a function

> Tools/DumpRenderTree/AccessibilityUIElement.h:255
> +    void resetSelectedTextMarkerRange(void);

ditto about void

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1586
> +    if (start) {

you should use the early return idiom here which is preferred

if (!start)
    return;

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1587
> +        NSArray* textMarkers = [NSArray arrayWithObjects:start, start, nil];

I think @[ start, start ]

might be nicer here

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1589
> +        if (textMarkerRange) {

ditto about early return

[Doug Russell]

Created attachment 247196 [details]
Patch

[Darin Adler]

Comment on attachment 247196 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:2609
> +bool AccessibilityObject::caretBrowsingEnabled() const
> +{
> +    if (FrameView *frameView = documentFrameView())
> +        return frameView->frame().mainFrame().settings().caretBrowsingEnabled();
> +    return false;
> +}

This should not involve the FrameView, nor need it call mainFrame. Instead it should just be:

    Frame* frame = this->frame();
    return frame && frame->settings().caretBrowsingEnabled();

> Source/WebCore/accessibility/AccessibilityObject.cpp:2614
> +    if (FrameView *frameView = documentFrameView())
> +        return frameView->frame().mainFrame().settings().setCaretBrowsingEnabled(on);

Same thing here:

    Frame* frame = this->frame();
    if (!frame)
        return;
    frame->settings().setCaretBrowsingEnabled(on);

Not normal WebKit style to use “return” with an expression of type void outside a template context.

> Source/WebCore/accessibility/AccessibilityObject.h:979
> +    bool caretBrowsingEnabled() const;
> +    void setCaretBrowsingEnabled(bool);

It’s kind of sloppy to put these operations on all accessibility objects when the intention is to only expose them for web area objects.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3359
> +// Used to set attributes synchronously on accessibility elements within tests.
> +// For use with DumpRenderTree only.
> +- (void)_accessibilitySetTestValue:(id)value forAttribute:(NSString*)attributeName
> +{
> +    [self _accessibilitySetValue:value forAttribute:attributeName];
>  }

How is this forwarding method helpful? Can’t the client code call _accessibilitySetValue:forAttribute: directly instead?

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:1414
> +{
> +    
> +}

Blank lines in empty function bodies like this are not WebKit coding style.

> Tools/DumpRenderTree/ios/AccessibilityUIElementIOS.mm:453
> +{
> +    
> +}

Blank lines in empty function bodies like this are not WebKit coding style.

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:96
> +void setBoolAttributeValue(JSStringRef attribute, bool value) { }

This needs to go down below in the !COCOA section.

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:216
> +PassRefPtr<AccessibilityTextMarkerRange> AccessibilityUIElement::selectedTextMarkerRange(void) { return nullptr; }

I don’t see a version of this for PLATFORM(IOS).

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:226
> +void AccessibilityUIElement::setBoolAttributeValue(JSStringRef attribute, bool value)

We normally omit any unused arguments’ names.

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:229
> +{
> +    
> +}

Blank lines in empty function bodies like this are not WebKit coding style.

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:82
> +// Set attribute values synchronously for tests

WebKit comment style is to use a period at the end of a sentence style comment like this. Also not sure this comment is helpful, since it’s repeated where the method is defined.

[Doug Russell]

(In reply to comment #17)
> Comment on attachment 247196 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247196&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:2609
> > +bool AccessibilityObject::caretBrowsingEnabled() const
> > +{
> > +    if (FrameView *frameView = documentFrameView())
> > +        return frameView->frame().mainFrame().settings().caretBrowsingEnabled();
> > +    return false;
> > +}
> 
> This should not involve the FrameView, nor need it call mainFrame. Instead
> it should just be:
> 
>     Frame* frame = this->frame();
>     return frame && frame->settings().caretBrowsingEnabled();
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:2614
> > +    if (FrameView *frameView = documentFrameView())
> > +        return frameView->frame().mainFrame().settings().setCaretBrowsingEnabled(on);
> 
> Same thing here:
> 
>     Frame* frame = this->frame();
>     if (!frame)
>         return;
>     frame->settings().setCaretBrowsingEnabled(on);
> 
> Not normal WebKit style to use “return” with an expression of type void
> outside a template context.
> 
> > Source/WebCore/accessibility/AccessibilityObject.h:979
> > +    bool caretBrowsingEnabled() const;
> > +    void setCaretBrowsingEnabled(bool);
> 
> It’s kind of sloppy to put these operations on all accessibility objects
> when the intention is to only expose them for web area objects.

There’s not really an accessibility object that corresponds solely to web areas. We could put this on AccessibilityNodeObject or AccessibilityRenderObject,
but that’s also covering a lot of types of elements. We thought that AXObject was a good place because then we don’t have to worry about casting it to one of those other types, and technically there’s nothing stopping us from using this on different elements if the need arised

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3359
> > +// Used to set attributes synchronously on accessibility elements within tests.
> > +// For use with DumpRenderTree only.
> > +- (void)_accessibilitySetTestValue:(id)value forAttribute:(NSString*)attributeName
> > +{
> > +    [self _accessibilitySetValue:value forAttribute:attributeName];
> >  }
> 
> How is this forwarding method helpful? Can’t the client code call
> _accessibilitySetValue:forAttribute: directly instead?

I wanted there to be an explicit method that documented that tests were dependent on having a way to set attribute values synchronously in the event that _accessibilitySetValue: needs to change it’s behavior for other reasons.

> 
> > Tools/DumpRenderTree/AccessibilityUIElement.cpp:1414
> > +{
> > +    
> > +}
> 
> Blank lines in empty function bodies like this are not WebKit coding style.
> 
> > Tools/DumpRenderTree/ios/AccessibilityUIElementIOS.mm:453
> > +{
> > +    
> > +}
> 
> Blank lines in empty function bodies like this are not WebKit coding style.
> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:96
> > +void setBoolAttributeValue(JSStringRef attribute, bool value) { }
> 
> This needs to go down below in the !COCOA section.
> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:216
> > +PassRefPtr<AccessibilityTextMarkerRange> AccessibilityUIElement::selectedTextMarkerRange(void) { return nullptr; }
> 
> I don’t see a version of this for PLATFORM(IOS).
> 
> > Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:226
> > +void AccessibilityUIElement::setBoolAttributeValue(JSStringRef attribute, bool value)
> 
> We normally omit any unused arguments’ names.
> 
> > Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:229
> > +{
> > +    
> > +}
> 
> Blank lines in empty function bodies like this are not WebKit coding style.
> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:82
> > +// Set attribute values synchronously for tests
> 
> WebKit comment style is to use a period at the end of a sentence style
> comment like this. Also not sure this comment is helpful, since it’s
> repeated where the method is defined.

Everything else fixed in a forthcoming patch

[Doug Russell]

Created attachment 247398 [details]
Patch

[Doug Russell]

Created attachment 247404 [details]
Patch

[Doug Russell]

Created attachment 247408 [details]
Patch

[Doug Russell]

Created attachment 247430 [details]
Caret Browsing

[chris fleizach]

Comment on attachment 247430 [details]
Caret Browsing

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

> Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:119
> +    if (!frame)

this can probably be written as
if (Frame* frame = this->frame())
   frame->settings()....

[Doug Russell]

Created attachment 247441 [details]
Patch for landing

[chris fleizach]

Comment on attachment 247441 [details]
Patch for landing

I don't think we got a positive review yet. can you set review? first
thanks

[WebKit Commit Bot]

Comment on attachment 247441 [details]
Patch for landing

Rejecting attachment 247441 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 247441, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
141862.
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.appspot.com/results/6337000428797952

[chris fleizach]

Comment on attachment 247441 [details]
Patch for landing

oh yea, no change logs in this one

[Doug Russell]

Created attachment 247458 [details]
Patch for landing (with change logs)

[chris fleizach]

Comment on attachment 247458 [details]
Patch for landing (with change logs)

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

> Source/WebCore/ChangeLog:8
> +        Exposing the caret browsing setting on WebCore::Frame via the accessibility API would allow assistive tech apps to enable it contextually (for example, when the assistive tech app is running).

Doug can you also add a comment as to why we would want to do this for AT clients

[Doug Russell]

Created attachment 247464 [details]
More detailed change logs

[WebKit Commit Bot]

Comment on attachment 247464 [details]
More detailed change logs

Clearing flags on attachment: 247464

Committed r180718: <http://trac.webkit.org/changeset/180718>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.