Bug 41569 – [Chromium] Expose basic DOM information in WebAccessibilityObject

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

[Dominic Mazzoni]

Windows screenreaders want HTML DOM information from accessibility objects, including the tag name, attributes, and computed style, plus some basic properties of the document. These should be exposed in WebAccessibilityObject.

[Dominic Mazzoni]

Created attachment 60453 [details]
Adds methods to WebAccessibilityObject to access basic HTML DOM properties.

[WebKit Review Bot]

Attachment 60453 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebAccessibilityObject.cpp:418:  render_object is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/chromium/src/WebAccessibilityObject.cpp:437:  render_object is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/chromium/src/WebAccessibilityObject.cpp:463:  render_object is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/chromium/src/WebAccessibilityObject.cpp:494:  render_object is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/chromium/src/WebAccessibilityObject.cpp:500:  render_style is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/chromium/src/WebAccessibilityObject.cpp:543:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebKit/chromium/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
WebKit/chromium/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebKit/chromium/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebKit/chromium/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 11 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominic Mazzoni]

Created attachment 60454 [details]
Fix style errors in previous patch.

[Adam Barth]

WEBKIT_API => we need the great and powerful fishd to review.

[Darin Fisher (:fishd, Google)]

Comment on attachment 60454 [details]
Fix style errors in previous patch.

WebKit/chromium/public/WebAccessibilityObject.h:108
 +      WEBKIT_API void attributeAt(unsigned index, WebString* name, WebString* value) const;
nit: webkit style is to use reference types for out params:  WebString& name, WebString& value

WebKit/chromium/public/WebAccessibilityObject.h:111
 +      WEBKIT_API WebString documentUrl() const;
nit: documentUrl -> documentURL

but, why do you need to expose these document properties?  Why not just
expose WebDocument (and add properties to it as needed)?
WebKit/chromium/src/WebAccessibilityObject.cpp:420
 +      Node* node = renderObject->node();
instead of exposing a tagName property, how about just exposing
a WebNode getter?  then, the consumer can call WebNode::nodeName.

WebKit/chromium/src/WebAccessibilityObject.cpp:440
 +      if (!node || !node->hasAttributes())
this also feels like something that should be provided by exposing
an attributes getter on WebElement.  Use the WebNodeList type.

WebKit/chromium/src/WebAccessibilityObject.cpp:507
 +  WebString WebAccessibilityObject::documentUrl() const
this method should definitely be eliminated in favor of accessing
the related WebNode, and then query its document.  from the
WebDocument you can get the WebFrame, and that has a "url" property.

WebKit/chromium/src/WebAccessibilityObject.cpp:532
 +      return document->title();
eliminate this method in favor of querying WebDocument::title.

WebKit/chromium/src/WebAccessibilityObject.cpp:543
 +      if (document && document->isXHTMLDocument())
WebDocument already has an isXHTMLDocument accessor

WebKit/chromium/src/WebAccessibilityObject.cpp:560
 +      DocumentType* doctype = document->doctype();
please expose a "WebDocumentType WebDocument::docType()" accessor.
that means creating the WebDocumentType class and adding the methods
to it that you need (just name for now).  it is fairly easy to 
create a new subclass of WebNode.  just look at WebDocument and
WebElement to see good examples of it.

[Dominic Mazzoni]

Thanks. I'm working on these changes now.

The only thing I'm not clear on is how to do the attributes getter on WebElement.  WebKit::Element::attributes returns a NamedNodeMap, which can't be wrapped by a WebNodeList.

Would it be better to create a new wrapper WebNamedNodeMap, or construct some subclass of NodeList with the attributes that can be wrapped by WebNodeList?

- Dominic

(In reply to comment #5)
> (From update of attachment 60454 [details])
> WebKit/chromium/public/WebAccessibilityObject.h:108
>  +      WEBKIT_API void attributeAt(unsigned index, WebString* name, WebString* value) const;
> nit: webkit style is to use reference types for out params:  WebString& name, WebString& value
> 
> WebKit/chromium/public/WebAccessibilityObject.h:111
>  +      WEBKIT_API WebString documentUrl() const;
> nit: documentUrl -> documentURL
> 
> but, why do you need to expose these document properties?  Why not just
> expose WebDocument (and add properties to it as needed)?
> WebKit/chromium/src/WebAccessibilityObject.cpp:420
>  +      Node* node = renderObject->node();
> instead of exposing a tagName property, how about just exposing
> a WebNode getter?  then, the consumer can call WebNode::nodeName.
> 
> WebKit/chromium/src/WebAccessibilityObject.cpp:440
>  +      if (!node || !node->hasAttributes())
> this also feels like something that should be provided by exposing
> an attributes getter on WebElement.  Use the WebNodeList type.
> 
> WebKit/chromium/src/WebAccessibilityObject.cpp:507
>  +  WebString WebAccessibilityObject::documentUrl() const
> this method should definitely be eliminated in favor of accessing
> the related WebNode, and then query its document.  from the
> WebDocument you can get the WebFrame, and that has a "url" property.
> 
> WebKit/chromium/src/WebAccessibilityObject.cpp:532
>  +      return document->title();
> eliminate this method in favor of querying WebDocument::title.
> 
> WebKit/chromium/src/WebAccessibilityObject.cpp:543
>  +      if (document && document->isXHTMLDocument())
> WebDocument already has an isXHTMLDocument accessor
> 
> WebKit/chromium/src/WebAccessibilityObject.cpp:560
>  +      DocumentType* doctype = document->doctype();
> please expose a "WebDocumentType WebDocument::docType()" accessor.
> that means creating the WebDocumentType class and adding the methods
> to it that you need (just name for now).  it is fairly easy to 
> create a new subclass of WebNode.  just look at WebDocument and
> WebElement to see good examples of it.

[Dominic Mazzoni]

Created attachment 60906 [details]
Rewrote patch to expose WebNode and WebDocument from WebAccessibilityObject and added new methods to those.

[WebKit Review Bot]

Attachment 60906 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebNamedNodeMap.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebAttribute.cpp:37:  "WebAttribute.h" already included at WebKit/chromium/src/WebAttribute.cpp:32  [build/include] [4]
WebKit/chromium/src/WebRenderStyle.cpp:38:  "WebRenderStyle.h" already included at WebKit/chromium/src/WebRenderStyle.cpp:32  [build/include] [4]
WebKit/chromium/src/WebRenderStyle.cpp:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominic Mazzoni]

Created attachment 60933 [details]
Fix style errors in previous patch

[WebKit Review Bot]

Attachment 60933 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebAttribute.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebRenderStyle.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 60933 [details]
Fix style errors in previous patch

overall, this looks great!

WebKit/chromium/public/WebAttribute.h:69
> +      WebCore::Attribute* m_private;
please use the WebPrivatePtr class here.  we are not yet using it
consistently throughout the webkit api, but it is designed to make
this kind of code easier to write and helps avoid some errors.

WebKit/chromium/public/WebNamedNodeMap.h:69
> +      WebCore::NamedNodeMap* m_private;
use WebPrivatePtr here

WebKit/chromium/public/WebRenderStyle.h:68
> +      WebCore::RenderStyle* m_private;
use WebPrivatePtr here

WebKit/chromium/src/WebRenderStyle.cpp:72
> +      return WebString(CSSPrimitiveValue::create(m_private->display())->getStringValue());
one day, we might actually want a WebCSSValue, WebCSSPrimitiveValue, etc.
for now, though... what are doing here is fine.

as mentioned over IM, it looks like WebRenderStyle is unused.  either it
should be used or it should be excluded from this patch.

[Dominic Mazzoni]

Created attachment 60941 [details]
Added new accessors to WebNode and WebElement also

[WebKit Review Bot]

Attachment 60941 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebAttribute.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebNode.cpp:182:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/src/WebNode.cpp:190:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKit/chromium/src/WebNode.cpp:190:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/chromium/src/WebRenderStyle.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebElement.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominic Mazzoni]

Created attachment 61080 [details]
New classes now use WebPrivatePtr

[WebKit Review Bot]

Attachment 61080 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebAttribute.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebRenderStyle.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebElement.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 61080 [details]
New classes now use WebPrivatePtr

WebKit/chromium/public/WebNode.h:106
 +      WEBKIT_API WebElement element() const;
I don't think we need to add this element accessor.  we already
have isElementNode(), which makes it easy to test if a WebNode
is a WebElement.

otherwise, this change LGTM

[Dominic Mazzoni]

Created attachment 61110 [details]
Got rid of WebNode::element()

OK, WebNode::element() is gone.

This may be a stupid question, but is there a cleaner way to cast a WebNode to a WebElement? I added it because I thought this would be ugly:

WebKit::WebElement element(*reinterpret_cast<WebKit::WebElement*>(&node));

If you have a suggestion for a cleaner way to cast WebNode to WebElement, that's great - but it doesn't matter because either way it will get reviewed shortly in a Chromium patch. :)

[WebKit Review Bot]

Attachment 61110 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebAttribute.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebRenderStyle.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebElement.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

(In reply to comment #17)
> Created an attachment (id=61110) [details]
> Got rid of WebNode::element()
> 
> OK, WebNode::element() is gone.
> 
> This may be a stupid question, but is there a cleaner way to cast a WebNode to a WebElement? I added it because I thought this would be ugly:
> 
> WebKit::WebElement element(*reinterpret_cast<WebKit::WebElement*>(&node));
> 
> If you have a suggestion for a cleaner way to cast WebNode to WebElement, that's great - but it doesn't matter because either way it will get reviewed shortly in a Chromium patch. :)

WebNode has templatized "to" and "toConst" methods.

[WebKit Commit Bot]

Comment on attachment 61110 [details]
Got rid of WebNode::element()

Rejecting patch 61110 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 61110, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
ipts/webkitpy/tool/commands/download.py", line 155, in _process_patch
    self._main_sequence.run_and_handle_errors(tool, options, state)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/commands/stepsequence.py", line 80, in run_and_handle_errors
    raise e
webkitpy.common.system.executive.ScriptError: Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1

[Adam Barth]

Comment on attachment 61110 [details]
Got rid of WebNode::element()

Sorry, we had a bug in the cq, which should now be fixed.

[WebKit Commit Bot]

Comment on attachment 61110 [details]
Got rid of WebNode::element()

Clearing flags on attachment: 61110

Committed r63156: <http://trac.webkit.org/changeset/63156>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/63156 might have broken Chromium Linux Release

[James Robinson]

Broke cr linux.  Also, why do you need .display?  Exposing style/layout dependent information via the WK API seems really sketchy since there's no way to know what sort of state layout and style are in (i.e. they both could be arbitrarily out of date when you call them, no?)

[Darin Fisher (:fishd, Google)]

(In reply to comment #25)
> Broke cr linux.  Also, why do you need .display?  Exposing style/layout 
> dependent information via the WK API seems really sketchy since there's no way 
> to know what sort of state layout and style are in (i.e. they both could be 
> arbitrarily out of date when you call them, no?)

Good point.  I wonder if the computedStyle() getter forces layout to be updated.

[Dominic Mazzoni]

(In reply to comment #26)
> (In reply to comment #25)
> > Broke cr linux.  Also, why do you need .display?  Exposing style/layout 
> > dependent information via the WK API seems really sketchy since there's no way 
> > to know what sort of state layout and style are in (i.e. they both could be 
> > arbitrarily out of date when you call them, no?)
> 
> Good point.  I wonder if the computedStyle() getter forces layout to be updated.

Windows screenreaders want computedStyle.display in order to determine whether a particular element is block or inline. It affects how information is presented to a blind user, and how they navigate the page.

Any suggestions for a good way to access this information in an up-to-date manner? Should I make it part of AccessibilityObject, which seems to have a mechanism for keeping its data in sync?