Bug 105638

Created attachment 180760 [details] Patch

Comment on attachment 180760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180760&action=review So I really think we should be unskipping these tests for WebKit2 as well. Perhaps we can make the fix general enough for both ports? > Tools/DumpRenderTree/gtk/AccessibilityControllerGtk.cpp:79 > + AtkObject* accessible = DumpRenderTreeSupportGtk::accessibleElementById(mainFrame, id); > + if (!accessible) > + return 0; > + > + return AccessibilityUIElement(accessible); Hrm. I'm not certain, but maybe one way to make this work for WebKit1 and WebKit2 (instead of using DumpRenderTree support) would be to embed an atk text attribute into the object and just walk the tree.

Comment on attachment 180760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180760&action=review > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:308 > +AtkObject* DumpRenderTreeSupportGtk::accessibleElementById(WebKitWebFrame* frame, JSStringRef id) This implementation isn't exactly incorrect, but it's not what I intended - the way I implemented this for Mac and Chromium was to walk the accessibility tree from the top until we reach one with the right id. That way: * It makes sure the element really is in the tree, reachable from the root * It makes sure the element isn't ignored - I could be wrong, but I think this might return an ignored element (which can still have an AccessibleObject, but wouldn't be in the tree) * It creates accessible objects for all other elements along the way - a side-effect that I know at least one of the tests might depend on * Finally, the overall spirit of the tests is to test the external API - to just query the accessible tree directly - rather than reaching behind the scenes to convert a DOM node into an accessible object (something a screen reader could never do) >> Tools/DumpRenderTree/gtk/AccessibilityControllerGtk.cpp:79 >> + return AccessibilityUIElement(accessible); > > Hrm. I'm not certain, but maybe one way to make this work for WebKit1 and WebKit2 (instead of using DumpRenderTree support) would be to embed an atk text attribute into the object and just walk the tree. Yep, a side benefit is that then it should work for both WK1 and WK2.

(In reply to comment #3) > (From update of attachment 180760 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180760&action=review > > > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:308 > > +AtkObject* DumpRenderTreeSupportGtk::accessibleElementById(WebKitWebFrame* frame, JSStringRef id) > > This implementation isn't exactly incorrect, but it's not what I intended - the way I implemented this for Mac and Chromium was to walk the accessibility tree from the top until we reach one with the right id. My bad. I didn't get that intent from the method name. I'm happy to walk the tree for the reasons you specify and will do so. I'm less jazzed about adding an AtkObject Attribute whose purpose is not to support accessibility but to make it possible to run tests in WK2. ;) So if it is possible, I'd prefer to not go that route. For instance, walk the accessible tree, doing an equality check for the accessible object resulting from the DOM node conversion. Would that be a BadIdea(tm)? If so, why? If not, is it something that you just cannot do in WK2? Apologies if my questions are dumb. I know I really need to get up to speed on it and will make some time to do so. In the meantime, thanks in advance for shedding some light on the above. :)

Comment on attachment 180760 [details] Patch So in chatting with Martin about this, I have learned that I cannot get an element by id in the UIProcess. Drat! I continue to not like the idea of exposing something **which no assistive technology needs** (namely the HTML element's ID) as an accessible attribute. That just feels like a gross hack to me. By the same token, the method in question seems to be used by a sufficient number of tests that the greater evil is the skippage. So patch with gross hack coming up.

Created attachment 180832 [details] Patch

Comment on attachment 180832 [details] Patch I hadn't flagged this for review because I wanted to see if the EWS would spit up. EWS seems happy, so reviews would be appreciated. TIA!

Comment on attachment 180832 [details] Patch Unofficial review, looks good. View in context: https://bugs.webkit.org/attachment.cgi?id=180832&action=review > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:451 > + // cannot be done from the UIProcess. Assistive technologies have no need FWIW, Firefox has been exposing the "id" attribute to assistive technology on Windows for a while, it's documented here: https://developer.mozilla.org/en-US/docs/Accessibility/AT-APIs/Gecko/Attrs So maybe it's not *that* much of a hack. I totally understand your concern - one should be cautious about exposing interfaces that are only intended for testing. I think the benefits outweigh the concerns in this case, still - because 'id' is used as an accessibility attribute on some platforms, and also because we really needed some mechanism to uniquely identify an accessible node, in a cross-platform way, without using an attribute that could possibly modify the accessible tree! Still, would it be possible to only expose this attribute when used for testing? Maybe DRT/WKTR could call some global function to enable unit testing support in WebKit/ATK, that way this extra attribute wouldn't be exposed under normal circumstances in a web browser?

Comment on attachment 180832 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180832&action=review > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:455 > + String id = toElement(node)->getIdAttribute().string(); I agree. AccessibilityObject should have a idAttribute() method on it that you can call. It's pretty gnarly for you to check the node of the core object

Comment on attachment 180832 [details] Patch Clearing flags on attachment: 180832 Committed r139250: <http://trac.webkit.org/changeset/139250>

All reviewed patches have been landed. Closing bug.