Bug 50126

This bug needs a reduction/test case and should be updated with links to the relevants spec(s) as well as information about what other browsers do.

http://www.w3.org/TR/html5/the-canvas-element.html ("the user can still focus") Here are two tests, one fires a focus event on the element, on load, the other should fire an event when the tab key is pressed (and tab index navigated to). [body onload="document.getElementById('test').focus()"] [canvas] [input type="checkbox" id="test" onfocus="alert('You found me')" tabindex="1" /] [/canvas] [/body] Bug also posted on Chromium: http://code.google.com/p/chromium/issues/detail?id=64683 Currently, Internet Explorer 9 behaves per the standard, Firefox 4 does not (yet, I believe), but has started work on it.

The test case would be better as an attachment. Good to know IE/FF behavior. Do we have a FF bug link? (Not necessary, bug possibly helpful.)

From David Bolter: "closest bug" https://bugzilla.mozilla.org/show_bug.cgi?id=495912 Bug 495912 - Expose alternative content in Canvas element to ATs

This might be to do with editingIgnoresContent and canHaveChildrenForEditing.

Created attachment 81856 [details] Proposed patch to add support for accessible canvas fallback content. Includes new layout test.

Comment on attachment 81856 [details] Proposed patch to add support for accessible canvas fallback content. Includes new layout test. View in context: https://bugs.webkit.org/attachment.cgi?id=81856&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3283 > The default for allowing children is true. I don't think canvas fits any of the roles that don't allow children, so it seems this is not necessary > Source/WebCore/html/HTMLFormControlElement.cpp:243 > return false; Instead of removing the size check, which may be valid in other contexts, should you just add another check to determine if its size is empty and it's not a child of a canvas area. > Source/WebCore/rendering/RenderObject.cpp:1317 > } Might be a good idea to have a comment why this changed

Comment on attachment 81856 [details] Proposed patch to add support for accessible canvas fallback content. Includes new layout test. View in context: https://bugs.webkit.org/attachment.cgi?id=81856&action=review >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3283 >> > > The default for allowing children is true. I don't think canvas fits any of the roles that don't allow children, so it seems this is not necessary The layout test doesn't pass without this line. A canvas has a roleValue of ImageRole, so the switch statement immediately below this causes it to return false (that it's not allowed to have children). Is there a better role for canvas? Should we create a new role (internally) but then map it to Image at the platform-specific accessibility API level? >> Source/WebCore/html/HTMLFormControlElement.cpp:243 >> return false; > > Instead of removing the size check, which may be valid in other contexts, should you just add another check to determine if its size is empty and it's not a child of a canvas area. Good idea, done. >> Source/WebCore/rendering/RenderObject.cpp:1317 >> } > > Might be a good idea to have a comment why this changed The issue is that clippedOverflowRectForRepaint(repaintContainer) tries to access the containing block and crashes if it doesn't exist. A child of a RenderHTMLCanvas doesn't necessarily have a containing block. Luckily repainting isn't needed for that case, either, so it suffices to just check whether containingBlock() exists. I restructured it slightly and added a comment, how's this?

Created attachment 82201 [details] Updated patch responds to Chris's review

Attachment 82201 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/acce..." exit_code: 1 Source/WebCore/html/HTMLFormControlElement.cpp:257: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #8) > (From update of attachment 81856 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81856&action=review > > >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3283 > >> > > > > The default for allowing children is true. I don't think canvas fits any of the roles that don't allow children, so it seems this is not necessary > > The layout test doesn't pass without this line. A canvas has a roleValue of ImageRole, so the switch statement immediately below this causes it to return false (that it's not allowed to have children). > > Is there a better role for canvas? Should we create a new role (internally) but then map it to Image at the platform-specific accessibility API level? > I should have guessed canvas defaults to an image. I think that we should have a new internal Canvas role that defaults to the Group role on most platforms. I know on the mac, that if an image has a child, VoiceOver will not access it correctly. > >> Source/WebCore/html/HTMLFormControlElement.cpp:243 > >> return false; > > > > Instead of removing the size check, which may be valid in other contexts, should you just add another check to determine if its size is empty and it's not a child of a canvas area. > > Good idea, done. > > >> Source/WebCore/rendering/RenderObject.cpp:1317 > >> } > > > > Might be a good idea to have a comment why this changed > > The issue is that clippedOverflowRectForRepaint(repaintContainer) tries to access the containing block and crashes if it doesn't exist. A child of a RenderHTMLCanvas doesn't necessarily have a containing block. Luckily repainting isn't needed for that case, either, so it suffices to just check whether containingBlock() exists. > > I restructured it slightly and added a comment, how's this? Looks good. Need to fix the style issue as well

Created attachment 82455 [details] Updated patch now adds new canvas role and fixes style error. I like this; it's cleaner to have a separate role for Canvas internally. I mapped the new canvas role to "grouping" as you suggested (and took a guess for the GTK port). Please make sure I didn't miss any spots where I should handle the new role.

Comment on attachment 82455 [details] Updated patch now adds new canvas role and fixes style error. looks good. will r+ when it passes enough bots

Comment on attachment 82455 [details] Updated patch now adds new canvas role and fixes style error. r=me

Comment on attachment 82455 [details] Updated patch now adds new canvas role and fixes style error. Rejecting attachment 82455 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2 Last 500 characters of output: === BUILD AGGREGATE TARGET All OF PROJECT DumpRenderTree WITH CONFIGURATION Release === Check dependencies ** BUILD SUCCEEDED ** Compiling Java tests make: Nothing to be done for `default'. Running tests from /mnt/git/webkit-commit-queue/LayoutTests Testing 22607 test cases. accessibility ............................. accessibility/canvas.html -> failed Exiting early after 1 failures. 29 tests run. 0.75s total testing time 28 test cases (96%) succeeded 1 test case (3%) had incorrect layout Full output: http://queues.webkit.org/results/7907978

Created attachment 82504 [details] Updated patch modifies failing layout test to recognize new role for canvas elements. Since AXGroup is the role of a lot of things other than just canvas, I modified the test to check the title also, to make sure we're checking the role of the right element.

!!!! :-) Thanks Chris.

Comment on attachment 82504 [details] Updated patch modifies failing layout test to recognize new role for canvas elements. Clearing flags on attachment: 82504 Committed r78789: <http://trac.webkit.org/changeset/78789>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/78789 might have broken GTK Linux 64-bit Debug

In LayoutTests/accessibility/canvas-fallback-content.html: [...] + var fallback = document.getElementById("fallback"); + fallback.focus(); + + var accessibilityFocusedElement = accessibilityController.focusedElement; + + var pattern = "AXRole: AXCheckBox"; + if (document.activeElement.id == "fallback" && + accessibilityController.focusedElement.allAttributes().indexOf(pattern) != -1) { + result.innerText += "Test passed\n"; + } + else { + result.innerText += "Test failed\n" + fallbackElement.allAttributes(); + } [...] Shouldn't the test use 'accessibilityFocusedElement' instead of 'fallbackElement' in the 'else' branch? The GTK bots are entering in the 'else' branch and printing the following failure in the output: --- /home/slave/webkitgtk/gtk-linux-32-debug/build/layout-test-results/accessibility/canvas-fallback-content-expected.txt 2011-02-16 22:06:01.629666022 -0800 +++ /home/slave/webkitgtk/gtk-linux-32-debug/build/layout-test-results/accessibility/canvas-fallback-content-actual.txt 2011-02-16 22:06:01.629666022 -0800 @@ -1,3 +1,3 @@ +CONSOLE MESSAGE: line 41: ReferenceError: Can't find variable: fallbackElement -Test passed (see http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r78789%20(13902)/accessibility/canvas-fallback-content-diffs.txt) Another doubt I have is whether we could use 'accessibilityFocusedElement' instead of calling 'accessibilityController.focusedElement' again in the second part of the AND in the if condition. Guess so, but not sure whether perhaps there's a good reason for that. Just wondering. I didn't file a bug for these issues since I'd like to get some confirmation from you first, just to confirm I'm right. In the other hand, another issue is why the Gtk port enters that 'else' branch, and that's what I'll work on right away. In the meanwhile, I will skip the test in the GTK port, since even if we fixed that variable name the GTK bots would keep the test failing because execution would enter through the else branch anyway...

(In reply to comment #21) > [...] > In the other hand, another issue is why the Gtk port enters that 'else' branch, > and that's what I'll work on right away. In the meanwhile, I will skip the test > in the GTK port, since even if we fixed that variable name the GTK bots would > keep the test failing because execution would enter through the else branch > anyway... In case you're interested: https://bugs.webkit.org/show_bug.cgi?id=54626

Yet another thought on this test (In reply to comment #21) > In LayoutTests/accessibility/canvas-fallback-content.html: > > [...] > + var fallback = document.getElementById("fallback"); > + fallback.focus(); > + > + var accessibilityFocusedElement = accessibilityController.focusedElement; > + > + var pattern = "AXRole: AXCheckBox"; ^^^^^^^^^^ As far as I now the actual name of the accessibility roles depends on the platform (e.g. Mac: "AXCheckBox" / Atk: "check box"), so perhaps it would be a good idea to place the expected files for these kind of tests under the platform/ directory. I know there are already many tests printing AX roles not in the platform/ directory, but those are not failing since they're in the Skipped file at the moment. But at some point it would be good to get them fixed and then we'll have the same issue than now, so I guess we could consider doing this for new tests from now on, unless I'm missing something of course :-) Another approach would be to overwrite the general behavior by "adding" another expected file in platform/port when it differs from the general expected file, but as it seems this does not only affect to Mac and GTK, I think the other approach would be better. Opinions?

(In reply to comment #23) > [...] > Opinions? It came to me in a flash that another approach, to leave the expected files in the general directory untouched could be to somehow implement a way to map WebCore/Mac rolenames into Atk rolenames in GTK's DRT, so we always get the same rolenames, regardless of the platform being used. That wouldn't fix the problem for other platforms that might suffer from it, but the solution there would be similar (WebCore->Qt mapping, WebCore->Win mapping...) My two cents after brainstorming with myself. Eager to get some feedback...

I've also been thinking we need more platform agnostic information checks. Unfortunately that sometimes then does't test end to end on a platform. It would also be nice if in DRT you could detect the platform and apply the right result there sometimes

(In reply to comment #25) > I've also been thinking we need more platform agnostic information checks. Yep, that's my idea as well. My point is that, for the very specific case of rolenames, if we consider that the rolenames in WebCore are exactly the same than in Mac (I'm kinda guessing here, not 100% sure about this) when the Mac port wouldn't need to do anything additional and would be each port "who" would need to map those WebCore rolenames to the platform-specific ones. > Unfortunately that sometimes then does't test end to end on a platform. Didn't understand this sentence. Could you rephrase it? (perhaps it's my English skills, but I have the feeling there's something missing in there :-)) > It would also be nice if in DRT you could detect the platform and apply > the right result there sometimes Not sure I follow... DRT is specific fro every platform already

This introduced security bug - https://bugs.webkit.org/show_bug.cgi?id=55393 Reduced testcase:: <feOffset> <canvas> <legend id="test"> <input/> </legend> </canvas> </feOffset> <script> window.setTimeout(function() { document.getElementById('test').innerHTML = 1; }, 0); </script> Stack: WebCore::RenderObject::localToAbsolute(WebCore::FloatPoint localPoint=(0,0), bool fixed=false, bool useTransforms=true) Line 1945 + 0x15 bytes C++ >WebCore::LayoutState::LayoutState(WebCore::RenderObject * root=0x06e7600c) Line 121 C++ WebCore::RenderView::pushLayoutState(WebCore::RenderObject * root=0x06e7600c) Line 719 + 0x28 bytes C++ WebCore::FrameView::layout(bool allowSubtree=true) Line 900 C++ WebCore::FrameView::layoutTimerFired(WebCore::Timer<WebCore::FrameView> * __formal=0x0a094608) Line 1604 C++ WebCore::Timer<WebCore::FrameView>::fired() Line 100 + 0x29 bytes C++ WebCore::ThreadTimers::sharedTimerFiredInternal() Line 112 + 0xf bytes C++ WebCore::ThreadTimers::sharedTimerFired() Line 91 C++ webkit_glue::WebKitClientImpl::DoTimeout() Line 86 + 0xa bytes C++ LayoutState::LayoutState(RenderObject* root) : m_clipped(false) , m_pageLogicalHeight(0) , m_pageLogicalHeightChanged(false) , m_columnInfo(0) , m_next(0) #ifndef NDEBUG , m_renderer(root) #endif { RenderObject* container = root->container(); root is freed. Comment #5 From Dave Hyatt 2011-02-28 13:13:59 PST (-) [reply] (From update of attachment 84094 [details]) I think we should consider reverting the original patch. I don't think it really should have received r+. The original changes were bizarre and not reviewed by a layout expert.

I think the approach taken here was just all wrong. It would probably be better to (when fallback content is present) create an anonymous RenderBlock that fits the RenderHTMLCanvas snugly and also holds the fallback content and that can become the HTML element's renderer. That way you don't have to add hack upon hack to try to turn the RenderHTMLCanvas into a container, and you establish a proper containing block for the content. This patch really should be reverted.

The other option would be to turn RenderHTMLCanvas into a RenderBlock subclass. Then it acts as a proper container (and containing block), and you have a lots of control over what you can do to the children. For example: (1) You can set overflow:hidden when fallback content is present to prevent layout overflow. (2) You can make the canvas block act as a containing block for all children regardless of their positioning by patching container() and containingBlock(). (3) You can make the canvas have a layer and establish a stacking context when fallback content is present. (4) Force visibility:hidden down the descendant tree and you don't even have to patch painting and hit testing. canvas * { visibility:hidden !important }

Backing up even further, who says we even have to render this content? It just has to be focusable. That's it, right? Let's just make it focusable without creating any renderers. That should be easy, right?

IRC snippet: <dhyatt> anyway if we have to implement it and make renderers [2:37pm] enrica_ joined the chat room. [2:37pm] <dhyatt> the best way to fix it is to make RenderHTMLCanvas a RenderBlock and more like a form control [2:37pm] <dhyatt> i.e., inline-block by default, replaced bit set when inline [2:37pm] <dhyatt> etc. [2:38pm] tonikitoo left the chat room. (Read error: Operation timed out) [2:38pm] <dhyatt> not to try to make a RenderReplaced able to have arbitrary child content [2:38pm] <dhyatt> that way lies madness [2:38pm] diegohcg left the chat room. (Ping timeout: 240 seconds) [2:38pm] <dhyatt> RenderReplaced is not designed for that I also think we should separate the render tree changes from the AX side changes and make sure that experts in each review each side in isolation.

Marking reopened since the patch was reverted. Upon further reflection I'm pretty doubtful that this is a good idea.

(In reply to comment #32) > Marking reopened since the patch was reverted. Upon further reflection I'm pretty doubtful that this is a good idea. Please make sure that regression crasher test from c#27 is landed too when new patch goes.

Comment on attachment 82455 [details] Updated patch now adds new canvas role and fixes style error. Cleared Chris Fleizach's review+ from obsolete attachment 82455 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Do we have any new takers on this issue? It's likely that this issue will also crop up with css generated and replaced content. This issue has been implemented in IE9. Example in css: [div style="content: url('image.png') replaced;"][input tabindex=0 type="submit" /][/div] Example in canvas: [canvas][input tabindex=0 type="submit" /][/div] In both cases, the user should be able to tab focus onto the input element, which would fire a focus event on that element.

(In reply to comment #30) > Backing up even further, who says we even have to render this content? It just has to be focusable. That's it, right? Let's just make it focusable without creating any renderers. That should be easy, right? Fr this specific bug, there's no need to have renderers. In generally, we need to expose the canvas's child content to assistive technologies, possibly even including metrics. For some of that, the content may need renderers. I have not yet looked into it in depth, but it would probably be wise to review all the things needed for canvas child DOM accessibility to choose the right approach.

I'm coming at this bug from a slightly different angle here and would like to suggest that we might be better off implementing canvas' default behavior as a shadow DOM. That way, we just replace the fallback content with a shadow DOM in the normal behavior, and will use the original DOM for fallback. And we already have a proven infrastructure for this.

Sorry, maybe my comment was too terse. What I meant is that we can get rid of the RenderReplaced and use the shadow DOM to implement replaced elements instead. For example, in the case of the cavas element, we'll attach a special shadow DOM that "implements the canvas" by default. The fallback content is shown iff the shadow DOM isn't attached and the innate DOM is shown. HTMLCanvasElement -> ShadowRoot -> some-special-element/renderer, inaccessible from ES5 | v fallback contents (accessible from DOM, focus, etc... would just work when shown) That way, we don't need to have all sorts of crazy hacks we have in RenderReplaced and friends today.

(In reply to comment #38) > Sorry, maybe my comment was too terse. > > What I meant is that we can get rid of the RenderReplaced and use the shadow DOM to implement replaced elements instead. For example, in the case of the cavas element, we'll attach a special shadow DOM that "implements the canvas" by default. The fallback content is shown iff the shadow DOM isn't attached and the innate DOM is shown. > > HTMLCanvasElement -> ShadowRoot -> some-special-element/renderer, inaccessible from ES5 > | > v > fallback contents (accessible from DOM, focus, etc... would just work when shown) > > That way, we don't need to have all sorts of crazy hacks we have in RenderReplaced and friends today. Formulating RenderReplaced as an effect of shadow DOM is a pretty brilliant idea, Ryosuke. I am probably not aware of all of the edge cases here, but it seems this just needs to be done.

Filed bug 78616 to track Niwa-san's idea.

Created attachment 138323 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 138323 [details] Patch Attachment 138323 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12477752

OK, here's an ugly-but-working patch to continue the conversation. No need to do a line-by-line review yet, I'd just like to figure out what general approach will be the next step. 1. The patch I just uploaded implements Dave Hyatt's suggestion to make RenderHTMLCanvas subclass RenderBlock. In doing so, I copied some code from RenderReplaced; if we decide to go this route, we should figure out how to share that code. The advantage of this approach is that the vast majority of the changes are only in RenderHTMLCanvas; once you make it a RenderBlock, focusable and accessible fallback content happens basically automatically. 2. Another alternative (suggested by Maciej) would be to not create RenderObjects for the fallback content but to still allow them to be focusable. This would be a larger refactoring of accessibility code to create AccessibilityObjects from Elements instead of RenderObjects, but might require fewer changes to RenderHTMLCanvas. Does this approach sound better? Would this actually work even in unusual corner cases like if someone puts an iframe inside of a canvas? 3. How does Ryosuke's shadow dom idea relate to these two approaches? I don't quite understand.

In my opinion, we should avoid implementing these hacks and just treat replaced contents of replaced elements as shadow content. The fallback content would work elegantly as the light DOM content without much work if we did that.

Ryosuke, wouldn't putting the fallback content into the canvas's shadow dom cause events to be re-targeted to the canvas? Or is there a way to avoid this?

(In reply to comment #46) > Ryosuke, wouldn't putting the fallback content into the canvas's shadow dom cause events to be re-targeted to the canvas? Or is there a way to avoid this? What I'm suggesting is to move the regular canvas content to the shadow DOM and keep the original fallback content in the light DOM (i.e. non-shadow DOM).

(In reply to comment #47) > What I'm suggesting is to move the regular canvas content to the shadow DOM and keep the original fallback content in the light DOM (i.e. non-shadow DOM). Maybe I missed the point but... In this context, just using shadow dom won't help. That is because we need to render the canvas content, and we shouldn't render the fallback content. Shadow DOM doesn't make any non-rendered content focusable. Maybe turning <canvas> into a shadow-dom backed element helps keeping code clean. But we could say that's the right approach only after we have some concrete idea on how the shadow tree of <canvas> is formed. I don'see how it will look like. ---- My 2c: In my understanding, even RenderReplaced subclasses can have child renderers when they override some methods. RenderMedia, a subclass of RenderImage, is one of such classes. So if the reason to switch to RenderBlock subclass is for having render children, we might keep the change smaller by following RenderMedia-like approach. It could be yet another rabbit hole though.

Comment on attachment 138323 [details] Patch Attachment 138323 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12528118 New failing tests: fast/canvas/canvas-incremental-repaint-2.html fast/canvas/shadow-offset-7.html compositing/webgl/webgl-nonpremultiplied-blend.html fast/canvas/canvas-text-alignment.html fast/canvas/image-object-in-canvas.html batterystatus/event-after-navigation.html accessibility/aria-disabled.html fast/canvas/canvas-transforms-during-path.html fast/canvas/drawImage-with-globalAlpha.html fast/canvas/image-pattern-rotate.html fast/canvas/fillrect-gradient-zero-stops.html fast/canvas/quadraticCurveTo.xml fast/canvas/toDataURL-alpha.html compositing/webgl/webgl-background-color.html fast/canvas/shadow-offset-5.html fast/canvas/webgl/canvas-test.html fast/canvas/canvas-composite.html fast/canvas/canvas-resize-after-paint-without-layout.html fast/canvas/canvas-incremental-repaint.html compositing/webgl/webgl-reflection.html fast/canvas/canvas-transform-skewed.html fast/canvas/shadow-offset-6.html fast/canvas/canvas-composite-transformclip.html fast/canvas/shadow-offset-4.html fast/canvas/canvas-composite-fill-repaint.html fast/canvas/patternfill-repeat.html fast/canvas/canvas-text-baseline.html accessibility/canvas.html fast/canvas/arc360.html

Created attachment 138489 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Hajime: the earlier patch from 2011 actually took an approach more similar to RenderMedia - it just takes RenderHTMLCanvas and extends it to allow child elements. It led to some subtle bugs and several people said they didn't like that design. I'm not sure why it makes more sense for RenderMedia. What are the children of RenderMedia?

Maciej, one of your comments is that there's no need to have renderers. I tried hacking on the accessibility code a bit to see what some of the implications would be of having no renderers while still exposing canvas fallback content: 1. There'd be no CSS-generated content, including not only ::before and ::after, but even things like list markers. 2. A textarea wouldn't get wrapped onto multiple lines so it'd appear like one long line. 3. There'd be no style calculations within the fallback content, so we wouldn't know to hide elements that are supposed to be display:none. We could find ways to work around #1 or #2, but it's #3 that bothers me. How much of a hack would it be to compute the style for a node without a renderer, just to determine a few key properties like display and visibility? Does it seem better to go down this route and try to sort out these issues as special cases, or does this make you think that it'd actually be simpler to just create renderers for the fallback content?

Created attachment 141221 [details] Patch

Comment on attachment 141221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141221&action=review looking pretty good to me > Source/WebCore/accessibility/AccessibilityMenuList.cpp:43 > + obj->init(); why do we need to make these changes? > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:194 > +AccessibilityRole AccessibilityNodeObject::determineAccessibilityRole() determineAXRole should account for ARIA roles as well. it will probably be quite common in canvi to use ARIA

(In reply to comment #54) > > Source/WebCore/accessibility/AccessibilityMenuList.cpp:43 > > + obj->init(); > > why do we need to make these changes? Otherwise AccessibilityNodeObject's constructor would need to call determineAccessibilityRole, but in C++ when a constructor calls a virtual method it won't call a derived implementation of that method. > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:194 > > +AccessibilityRole AccessibilityNodeObject::determineAccessibilityRole() > > determineAXRole should account for ARIA roles as well. it will probably be quite common in canvi to use ARIA Sure, I'll add this. Note that either way this is incomplete; we'll need to eventually make it support almost all of the roles and the rest of the methods. - Dominic

Comment on attachment 141221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141221&action=review isFocusable() is pretty hot - it's called in Element::attach() for instance. I don't think we can afford an O(N) tree walk in here for a very uncommon case > Source/WebCore/accessibility/AccessibilityNodeObject.h:84 > + Node* m_node; this is just a raw pointer, so who manages destroying this when the Node goes away? > Source/WebCore/dom/Node.cpp:930 > + while (e) { this is a really hot function to add a really slow walk to

> isFocusable() is pretty hot - it's called in Element::attach() for instance. I don't think we can afford an O(N) tree walk in here for a very uncommon case Good point. Do you think it'd be possible to add an inCanvas bit that could be set when attaching that only needs to propagate once? Other ideas?

Do we have room for a bit on Node? This is going to be exceedingly uncommon, maybe we could have a bit on Document or something like that saying "does anybody in this Document have canvas fallback content?" and check that before the walk.

Comment on attachment 141221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141221&action=review >> Source/WebCore/accessibility/AccessibilityNodeObject.h:84 >> + Node* m_node; > > this is just a raw pointer, so who manages destroying this when the Node goes away? This is important. In the case of RenderObject, the axObjectCache()->remove is called in willBeDestroyed. There doesn't seem anything like this for Node.

(In reply to comment #59) > (From update of attachment 141221 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141221&action=review > > >> Source/WebCore/accessibility/AccessibilityNodeObject.h:84 > >> + Node* m_node; > > > > this is just a raw pointer, so who manages destroying this when the Node goes away? > > This is important. In the case of RenderObject, the axObjectCache()->remove is called in willBeDestroyed. There doesn't seem anything like this for Node. FYI, there is something existing in AXObjectCache that cleans up weak Node pointers. That mechanism can be used for this as well.

Thanks for the feedback. Adding a flag to Document that keeps track of whether or not there are any Canvas elements with fallback content seems like a good solution. You're right, I need to detach (not delete) an AccessibilityNodeObject when the Node it references is deleted. There are many other places in WebCore where ab AXObjectCache method is called to update information based on a renderer; many of these would need to be changed to update using the node instead. I wasn't planning to tackle that in this same change - but I should definitely handle remove().

Comment on attachment 141221 [details] Patch Attachment 141221 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12663648 New failing tests: accessibility/canvas-fallback-content.html

Created attachment 141371 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 144482 [details] Patch

This latest patch: * Now detaches a AccessibilityNodeObject when the node it references is detached. Adds a new layout test to make sure this is working correctly. * Adds a flag to Document so that Node::isFocused doesn't bother to check to see if it's a descendant of a canvas unless there's a canvas with an element child somewhere in the document. * Moves the ARIA role calculation from AccessibilityRenderObject to AccessibilityNodeObject.

Comment on attachment 144482 [details] Patch Attachment 144482 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12840520

Comment on attachment 144482 [details] Patch Attachment 144482 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12843441

Comment on attachment 144482 [details] Patch Attachment 144482 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12852478 New failing tests: accessibility/canvas-fallback-content.html accessibility/accessibility-node-memory-management.html

Created attachment 144571 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 144482 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144482&action=review > Source/WebCore/dom/Node.cpp:1362 > + Document* doc = document(); > + if (AXObjectCache::accessibilityEnabled() && doc && doc->axObjectCacheExists()) > + doc->axObjectCache()->remove(this); I'm confused - Nodes in a canvas' shadow will never be attach()ed since they have no renderers, so how does this logic take care of cleanup for them?

Created attachment 144762 [details] Patch

(In reply to comment #70) > (From update of attachment 144482 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144482&action=review > > > Source/WebCore/dom/Node.cpp:1362 > > + Document* doc = document(); > > + if (AXObjectCache::accessibilityEnabled() && doc && doc->axObjectCacheExists()) > > + doc->axObjectCache()->remove(this); > > I'm confused - Nodes in a canvas' shadow will never be attach()ed since they have no renderers, so how does this logic take care of cleanup for them? I did this because detach() is called even if a node doesn't have a renderer, e.g. in this case it's called from ContainerNode::removeBetween - and I was having a hard time actually deleting a node from a layout test. Agreed, though, that handling it in the destructor is better. I moved it there and figured out how to test it.

Comment on attachment 144762 [details] Patch Attachment 144762 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12842837

Comment on attachment 144762 [details] Patch Attachment 144762 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12844824

Comment on attachment 144762 [details] Patch Attachment 144762 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12847834

Comment on attachment 144762 [details] Patch Attachment 144762 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12842859 New failing tests: accessibility/canvas-fallback-content.html accessibility/accessibility-node-memory-management.html

Created attachment 144787 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 144762 [details] Patch Is there any way to make this *massive* patch something we can do in smaller pieces? This is just too big to be done all at once, and seems like it’s something we could easily do in smaller steps. For example, one patch just to add the “init” function, one patch to do the AccessibilityNodeObject refeactor.

(In reply to comment #78) > (From update of attachment 144762 [details]) > Is there any way to make this *massive* patch something we can do in smaller pieces? This is just too big to be done all at once, and seems like it’s something we could easily do in smaller steps. For example, one patch just to add the “init” function, one patch to do the AccessibilityNodeObject refeactor. Yes, I'd be happy to. Is this looking okay at a high level? If so, I think I can prepare the first small change now. How about in this order: 1. Make canvas fallback content focusable, with no accessibility changes. 2. Accessibility refactoring only, no changes to behavior. 3. Final change that ties things together.

(In reply to comment #79) > (In reply to comment #78) > > (From update of attachment 144762 [details] [details]) > > Is there any way to make this *massive* patch something we can do in smaller pieces? I'm renaming and keeping this as the "final" bug, moving the first step to: https://bugs.webkit.org/show_bug.cgi?id=87898 Please look there for the first proposed smaller patch, thanks.

Duping out to 124592, since it's unlikely this patch will be revisited. *** This bug has been marked as a duplicate of bug 124592 ***