Bug 41569

Summary: [Chromium] Expose basic DOM information in WebAccessibilityObject
Product: WebKit Reporter: Dominic Mazzoni <dmazzoni>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, ctguil, eric, fishd, jamesr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Bug Depends on: 42129    
Bug Blocks:    
Attachments:
Description Flags
Adds methods to WebAccessibilityObject to access basic HTML DOM properties.
none
Fix style errors in previous patch.
fishd: review-
Rewrote patch to expose WebNode and WebDocument from WebAccessibilityObject and added new methods to those.
none
Fix style errors in previous patch
fishd: review-
Added new accessors to WebNode and WebElement also
none
New classes now use WebPrivatePtr
fishd: review-
Got rid of WebNode::element() none

Description Dominic Mazzoni 2010-07-03 09:21:34 PDT
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.
Comment 1 Dominic Mazzoni 2010-07-03 09:27:00 PDT
Created attachment 60453 [details]
Adds methods to WebAccessibilityObject to access basic HTML DOM properties.
Comment 2 WebKit Review Bot 2010-07-03 09:27:58 PDT
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.
Comment 3 Dominic Mazzoni 2010-07-03 09:33:17 PDT
Created attachment 60454 [details]
Fix style errors in previous patch.
Comment 4 Adam Barth 2010-07-07 03:55:15 PDT
WEBKIT_API => we need the great and powerful fishd to review.
Comment 5 Darin Fisher (:fishd, Google) 2010-07-07 08:57:03 PDT
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.
Comment 6 Dominic Mazzoni 2010-07-07 12:03:41 PDT
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.
Comment 7 Dominic Mazzoni 2010-07-08 10:15:46 PDT
Created attachment 60906 [details]
Rewrote patch to expose WebNode and WebDocument from WebAccessibilityObject and added new methods to those.
Comment 8 WebKit Review Bot 2010-07-08 10:21:41 PDT
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.
Comment 9 Dominic Mazzoni 2010-07-08 12:48:34 PDT
Created attachment 60933 [details]
Fix style errors in previous patch
Comment 10 WebKit Review Bot 2010-07-08 12:51:30 PDT
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.
Comment 11 Darin Fisher (:fishd, Google) 2010-07-08 13:15:27 PDT
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.
Comment 12 Dominic Mazzoni 2010-07-08 13:16:18 PDT
Created attachment 60941 [details]
Added new accessors to WebNode and WebElement also
Comment 13 WebKit Review Bot 2010-07-08 13:19:06 PDT
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.
Comment 14 Dominic Mazzoni 2010-07-09 13:17:59 PDT
Created attachment 61080 [details]
New classes now use WebPrivatePtr
Comment 15 WebKit Review Bot 2010-07-09 13:20:38 PDT
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.
Comment 16 Darin Fisher (:fishd, Google) 2010-07-09 14:38:21 PDT
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
Comment 17 Dominic Mazzoni 2010-07-09 15:55:45 PDT
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. :)
Comment 18 WebKit Review Bot 2010-07-09 15:58:08 PDT
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.
Comment 19 Darin Fisher (:fishd, Google) 2010-07-12 12:46:54 PDT
(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.
Comment 20 WebKit Commit Bot 2010-07-12 13:31:52 PDT
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
Comment 21 Adam Barth 2010-07-12 18:07:05 PDT
Comment on attachment 61110 [details]
Got rid of WebNode::element()

Sorry, we had a bug in the cq, which should now be fixed.
Comment 22 WebKit Commit Bot 2010-07-12 18:46:01 PDT
Comment on attachment 61110 [details]
Got rid of WebNode::element()

Clearing flags on attachment: 61110

Committed r63156: <http://trac.webkit.org/changeset/63156>
Comment 23 WebKit Commit Bot 2010-07-12 18:46:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 WebKit Review Bot 2010-07-12 18:55:32 PDT
http://trac.webkit.org/changeset/63156 might have broken Chromium Linux Release
Comment 25 James Robinson 2010-07-12 19:06:11 PDT
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?)
Comment 26 Darin Fisher (:fishd, Google) 2010-07-12 22:00:12 PDT
(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.
Comment 27 Dominic Mazzoni 2010-07-12 23:07:41 PDT
(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?