Bug 43044 - WebAccessibilityObject should provide access to computedStyle
: WebAccessibilityObject should provide access to computedStyle
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Accessibility
: 528+ (Nightly build)
: PC Windows Vista
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-27 06:58 PDT by Dominic Mazzoni
Modified: 2010-09-03 00:00 PDT (History)
7 users (show)

See Also:


Attachments
Implementation of proposed functionality. (9.39 KB, patch)
2010-07-27 07:12 PDT, Dominic Mazzoni
cfleizach: review-
Details | Formatted Diff | Diff
Return computedStyleDisplay directly from WebNode. (3.76 KB, patch)
2010-07-27 14:30 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Get rid of reference to WebRenderStyle from previous patch (3.38 KB, patch)
2010-07-27 14:37 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Moved to WebElement, now calls document->updateStyleAsNeeded to make sure the style is not stale. (3.53 KB, patch)
2010-07-28 07:57 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Moved to WebAccessibilityObject (3.87 KB, patch)
2010-07-28 09:47 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Mazzoni 2010-07-27 06:58:21 PDT
Windows screenreaders want to know if an AccessibilityObject is block or inline in order to determine how content should be read or navigated.

For chromium, WebAccessibilityObject now has access to its corresponding WebNode. All that's needed now is for WebNode to provide access to the computed style.
Comment 1 Dominic Mazzoni 2010-07-27 07:12:18 PDT
Created attachment 62687 [details]
Implementation of proposed functionality.
Comment 2 WebKit Review Bot 2010-07-27 07:14:23 PDT
Attachment 62687 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebRenderStyle.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebRenderStyle.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Adam Barth 2010-07-27 08:01:31 PDT
+fishd for WebKit API review.
Comment 4 chris fleizach 2010-07-27 12:48:26 PDT
Comment on attachment 62687 [details]
Implementation of proposed functionality.

can you fix the style issue
Comment 5 James Robinson 2010-07-27 13:29:08 PDT
I doubt the lifetime of the WebRenderStyle is what the caller will expect.  In particular this RenderStyle pointer will be deleted if styles on the node are re-resolved, which will be invisible to users of this API.  They'll just all of a sudden have a WebRenderStyle that points to deleted memory.

WebCore::RenderStyles have very complex lifetimes and are shared between multiple nodes.  I think it makes much more sense to let the caller query specific properties directly from the node and to copy these values out rather than exposing pointers to RenderStyle objects.  In particular, for this case I think all you need is to add these two functions to WebNode:

bool hasComputedStyle();
WebString computedStyleDisplay();

If you end up needing to grab all sorts of different properties off of a node this will get cumbersome, but we should address that when we have a compelling need.
Comment 6 Darin Fisher (:fishd, Google) 2010-07-27 13:37:29 PDT
(In reply to comment #5)
> In particular, for this case I think all you need is to add these two functions 
> to WebNode:
> 
> bool hasComputedStyle();
> WebString computedStyleDisplay();
> 
> If you end up needing to grab all sorts of different properties off of a node 
> this will get cumbersome, but we should address that when we have a compelling 
> need.

This just doesn't seem like something that should be a property of a DOM node (WebNode).  Maybe it could be a property of a DOM element (WebElement).  I think we should assume that we'll have more properties in the future, and so returning a WebComputedStyle object seems better to me (given past experience with the way the WebKit API has grown).
Comment 7 James Robinson 2010-07-27 13:51:02 PDT
In that case it should do a fully copy of the RenderStyle underneath the hood and not attempt to point to the node's RenderStyle.  However even in that case I think pretty much all users of WebComputedStyle will be incorrect.
Comment 8 Dominic Mazzoni 2010-07-27 14:30:03 PDT
Created attachment 62746 [details]
Return computedStyleDisplay directly from WebNode.

Here's a patch that returns computedStyleDisplay directly from WebNode. Maybe we can go with this for now and revisit if/when we end up needing more style attributes from a WebNode later?
Comment 9 Dominic Mazzoni 2010-07-27 14:37:12 PDT
Created attachment 62748 [details]
Get rid of reference to WebRenderStyle from previous patch

Sorry, uploaded too soon...fixed and tested.

Here's a patch that returns computedStyleDisplay directly from WebNode. Maybe we can go with this for now and revisit if/when we end up needing more style attributes from a WebNode later?
Comment 10 James Robinson 2010-07-27 14:41:58 PDT
This feels safer (although the value you get out depends on the style resolution state, which AFAIK is not actually exposed through the API, and so it may be arbitrarily stale).
Comment 11 Darin Fisher (:fishd, Google) 2010-07-27 14:45:18 PDT
Comment on attachment 62748 [details]
Get rid of reference to WebRenderStyle from previous patch

WebKit/chromium/public/WebNode.h:104
 +      WEBKIT_API bool hasComputedStyle() const;
what about my suggestion to move these methods to WebElement?
does it make sense for there to be computed style on other
types of nodes?

it is possible to call WebFrame::layout() before calling
computedStyleDisplay().
Comment 12 James Robinson 2010-07-27 14:49:12 PDT
In WebCore the computed style on a Node that's not an Element is just its parents computed style.  Not sure if that makes sense here.

Forcing layout is more heavyweight than just forcing style resolution, but that would produce correct results.
Comment 13 Dominic Mazzoni 2010-07-27 15:02:01 PDT
(In reply to comment #12)
> In WebCore the computed style on a Node that's not an Element is just its parents computed style.  Not sure if that makes sense here.
> 
> Forcing layout is more heavyweight than just forcing style resolution, but that would produce correct results.

Note that to produce the initial accessibility tree for Chromium, we need to walk the entire tree and call this method for every node. Does it make sense to call WebFrame::layout() every single time? Or would it be better to just add a comment here that the caller should call WebFrame::layout first?
Comment 14 Dominic Mazzoni 2010-07-27 15:04:58 PDT
(In reply to comment #11)
> what about my suggestion to move these methods to WebElement?
> does it make sense for there to be computed style on other
> types of nodes?

I just thought that since WebCore::Node provides getComputedStyle, why not expose that and keep it simpler for the calling function? If it's better to use Element::getComputedStyle, why does Node::getComputedStyle exist?

But I'm happy either way, I'll move it to WebElement instead if you want.

Just let me know if you really want me to call layout() when I know this is going to be used in an inner loop...it seems inefficient.

- Dominic
Comment 15 Dominic Mazzoni 2010-07-28 07:57:26 PDT
Created attachment 62820 [details]
Moved to WebElement, now calls document->updateStyleAsNeeded to make sure the style is not stale.
Comment 16 Dimitri Glazkov (Google) 2010-07-28 08:54:06 PDT
I really don't like such a low-level hook into WebCore. Is there any way we could move the accessibility tree traversal logic into WebCore? In other words, move the stuff that deals with WebCore objects into WebCore and leave only shipping/exchange API in the WebKit API?
Comment 17 Dimitri Glazkov (Google) 2010-07-28 09:12:56 PDT
Per IM conversation with Dominic, to limit the number of possible uses of this API (because I don't really want Chromium folks to assume that this is a free accessor, because it's actually rather expensive anb side-effectey), can we move it to WebAccessibilityObject?
Comment 18 Dominic Mazzoni 2010-07-28 09:47:14 PDT
Created attachment 62836 [details]
Moved to WebAccessibilityObject
Comment 19 Dimitri Glazkov (Google) 2010-07-28 10:05:59 PDT
Comment on attachment 62836 [details]
Moved to WebAccessibilityObject

ok.
Comment 20 WebKit Commit Bot 2010-07-28 20:12:25 PDT
Comment on attachment 62836 [details]
Moved to WebAccessibilityObject

Clearing flags on attachment: 62836

Committed r64260: <http://trac.webkit.org/changeset/64260>
Comment 21 WebKit Commit Bot 2010-07-28 20:12:30 PDT
All reviewed patches have been landed.  Closing bug.