[Webkit-unassigned] [Bug 141862] AX: Expose caret browsing preference to accessibility API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=141862

--- Comment #18 from Doug Russell <d_russell at apple.com> ---
(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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150226/946838d0/attachment-0002.html>