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).
<rdar://problem/19912263>
Created attachment 247025 [details] Patch
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
(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?
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
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
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
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
Created attachment 247035 [details] Patch
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
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
(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
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)
Created attachment 247170 [details] Patch
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
Created attachment 247196 [details] Patch
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.
(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
Created attachment 247398 [details] Patch
Created attachment 247404 [details] Patch
Created attachment 247408 [details] Patch
Created attachment 247430 [details] Caret Browsing
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()....
Created attachment 247441 [details] Patch for landing
Comment on attachment 247441 [details] Patch for landing I don't think we got a positive review yet. can you set review? first thanks
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
Comment on attachment 247441 [details] Patch for landing oh yea, no change logs in this one
Created attachment 247458 [details] Patch for landing (with change logs)
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
Created attachment 247464 [details] More detailed change logs
Comment on attachment 247464 [details] More detailed change logs Clearing flags on attachment: 247464 Committed r180718: <http://trac.webkit.org/changeset/180718>
All reviewed patches have been landed. Closing bug.