Summary: | [Chromium] Expose basic DOM information in WebAccessibilityObject | ||
---|---|---|---|
Product: | WebKit | Reporter: | Dominic Mazzoni <dmazzoni> |
Component: | Accessibility | Assignee: | 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
Dominic Mazzoni
2010-07-03 09:21:34 PDT
Created attachment 60453 [details]
Adds methods to WebAccessibilityObject to access basic HTML DOM properties.
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.
Created attachment 60454 [details]
Fix style errors in previous patch.
WEBKIT_API => we need the great and powerful fishd to review. 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.
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. Created attachment 60906 [details]
Rewrote patch to expose WebNode and WebDocument from WebAccessibilityObject and added new methods to those.
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.
Created attachment 60933 [details]
Fix style errors in previous patch
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 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. Created attachment 60941 [details]
Added new accessors to WebNode and WebElement also
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.
Created attachment 61080 [details]
New classes now use WebPrivatePtr
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 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
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. :)
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.
(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 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 on attachment 61110 [details]
Got rid of WebNode::element()
Sorry, we had a bug in the cq, which should now be fixed.
Comment on attachment 61110 [details] Got rid of WebNode::element() Clearing flags on attachment: 61110 Committed r63156: <http://trac.webkit.org/changeset/63156> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/63156 might have broken Chromium Linux Release 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?) (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. (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? |