Bug 87898

Created attachment 144925 [details] Patch

Comment on attachment 144925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144925&action=review > Source/WebCore/dom/Document.h:1170 > + bool hasCanvasWithElementChildren() { return m_hasCanvasWithElementChildren; } const ? > Source/WebCore/dom/Document.h:1171 > + void setHasCanvasWithElementChildren(bool f) { m_hasCanvasWithElementChildren = f; } we don't use one-letter variable names in WebKit except for things like iteration counters - please give this variable a more descriptive name > Source/WebCore/dom/Node.cpp:916 > + // An node without a renderer is focusable if it's within canvas fallback content. What if the canvas itself isn't focusable (for instance if the <canvas> is display:none)? It doesn't seem to make sense for the canvas' fallback content to be focusable in this case. This case needs tests, no matter which behavior is preferred. > Source/WebCore/html/HTMLCanvasElement.cpp:143 > + if (n->isElementNode()) { Why is this filtering for Elements? It looks like the logic in Node::isFocusable would return true for any Node type in the fallback, not just elements

(In reply to comment #2) > > Source/WebCore/dom/Document.h:1170 > > + bool hasCanvasWithElementChildren() { return m_hasCanvasWithElementChildren; } > > const ? Done. > > Source/WebCore/dom/Document.h:1171 > > + void setHasCanvasWithElementChildren(bool f) { m_hasCanvasWithElementChildren = f; } > > we don't use one-letter variable names in WebKit except for things like iteration counters - please give this variable a more descriptive name I was following the pattern of similar methods in this header file - but I guess there are examples both ways. I switched it to a more descriptive name. > > Source/WebCore/dom/Node.cpp:916 > > + // An node without a renderer is focusable if it's within canvas fallback content. > > What if the canvas itself isn't focusable (for instance if the <canvas> is display:none)? It doesn't seem to make sense for the canvas' fallback content to be focusable in this case. This case needs tests, no matter which behavior is preferred. Good catch. Added a check that the canvas element has a renderer and added a test. I'm not sure if you meant that literally because I don't think it matters if the canvas is *focusable*. But for sure if the canvas is hidden, its descendants should not be focusable. > > Source/WebCore/html/HTMLCanvasElement.cpp:143 > > + if (n->isElementNode()) { > > Why is this filtering for Elements? It looks like the logic in Node::isFocusable would return true for any Node type in the fallback, not just elements The common case I want to optimize for is if a canvas element has only text nodes as children, e.g. <canvas> Your browser does not support canvas </canvas>. If a canvas has any element child, that element could conceivably be focusable at some later point, or it could have focusable descendants. But if the canvas only has nodes as children, nothing within the canvas could ever get focus.

Created attachment 145601 [details] Patch

Comment on attachment 145601 [details] Patch this looks ok to me. anyone else more familiar with Document want to take a look? This is an important step to getting canvas accessibility going, which we are itching to move on

I'm expecting someone else to review this, I don't work enough on DOM stuff these days to do a final review.

Comment on attachment 145601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145601&action=review When do we ever set m_hasCanvasWithElementChildren to false? It feels a bit ugly to keep this state on the Document. Can this be stored on the canvas element instead? > LayoutTests/fast/canvas/fallback-content.html:36 > +<canvas hidden id="hiddenCanvas" width="300" height="300"> > + <a id="linkInHiddenCanvas" href="#">Link</a> > +</canvas> > + > +<canvas style="display:none" id="displayNoneCanvas" width="300" height="300"> > + <a id="linkInDisplayNoneCanvas" href="#">Link</a> > +</canvas> These two are the same. [hidden] just means "display: none" > LayoutTests/fast/canvas/fallback-content.html:44 > + function checkFocusable(id) { Don't put function statements inside blocks. if (window.layoutTestController) window.layoutTestController.dumpAsText(); function checkFocusable(id) { .... } > LayoutTests/fast/canvas/fallback-content.html:75 > + window.previousFocusedElement = document.activeElement; > + window.element = document.getElementById(id); Use a global var and skip window here: var previousFocusedElement, element; function checkNotFocusable(id) { .... previousFocusedElement = ... }

(In reply to comment #7) > (From update of attachment 145601 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145601&action=review > > When do we ever set m_hasCanvasWithElementChildren to false? We don't. If a document includes actual elements within a canvas tag once, we flip the state and pay the performance penalty of checking from then on. > It feels a bit ugly to keep this state on the Document. Can this be stored on the canvas element instead? Wouldn't that mean that the state would then be global, rather than per-document? That seems worse - a document with fallback canvas content in one frame could leak the performance penalty to every other frame. Alternative approaches welcome.

I think the wording on that flag to optimize documents that never do this is wrong. It’s named m_hasCanvasWithElementChildren, but will be true even when we do not have a canvas with element children, but did at some time in the past. A more precise name might be something like m_everHadCanvasWithElementChildren or something along those lines. Or maybe m_canAssumeNoCanvasHasElementChildren is clearer.

(In reply to comment #7) > These two are the same. [hidden] just means "display: none" Removed one. > > LayoutTests/fast/canvas/fallback-content.html:44 > > + function checkFocusable(id) { > > Don't put function statements inside blocks. Done. > Use a global var and skip window here: Done. > var previousFocusedElement, element; > > function checkNotFocusable(id) { > .... > previousFocusedElement = ... > } (In reply to comment #9) > A more precise name might be something like m_everHadCanvasWithElementChildren or something along those lines. Or maybe m_canAssumeNoCanvasHasElementChildren is clearer. I like m_canAssumeNoCanvasHasElementChildren. Done.

Created attachment 147145 [details] Patch

(In reply to comment #8) > > It feels a bit ugly to keep this state on the Document. Can this be stored on the canvas element instead? > > Wouldn't that mean that the state would then be global, rather than per-document? That seems worse - a document with fallback canvas content in one frame could leak the performance penalty to every other frame. > > Alternative approaches welcome. Yeah, I don't have any suggestions here. Besides from the ugly feeling I have no other issues. It looks good to me.

Why do we need to not add renderers? I don't like adding what feels like a hack to our focus code, just for this odd-ball case with <canvas>.

(In reply to comment #13) > Why do we need to not add renderers? I don't like adding what feels like a hack to our focus code, just for this odd-ball case with <canvas>. Good question. Dave Hyatt and Maciej both suggested that to comply with the spec, we don't need renderers, we just need elements in fallback content to be focusable. See this bug for the longer history: https://bugs.webkit.org/show_bug.cgi?id=50126 I did try the approach of adding renderers for children of a canvas element. The most recent patch was this one, where I changed RenderHTMLCanvas to inherit from RenderBlock instead of RenderReplaced. https://bugs.webkit.org/attachment.cgi?id=138323&action=review I think it's a valid criticism that creating renderers for canvas fallback content could be wasteful. It's also probably true that many existing pages have some simple content (usually just static text) inside canvas elements intended for older browsers that don't support canvas at all, and it'd be wasteful to create renderers for those. On the other hand, both IE and Firefox have had this feature implemented for some time now, and they support applying styles to elements int he canvas subtree - for example I can make an element display:none within the canvas subtree to hide it, or I can add generated content with css pseudoclasses that I can access with a screen reader. So that suggests that if we don't create renderers, we'd have more work to do in order to be compatible with other browsers. What do you think?

I guess I just strongly disagree with the canvas spec on this one. I think attempting to make <canvas> play semantic is silly. <canvas> is a bit-buffer. It isn't accessible. I can't imagine that people will actually use fallback content in this way (or bother to keep it up to date), and it seems like a large hack to our engine to support this feature. :(

(In reply to comment #15) > I guess I just strongly disagree with the canvas spec on this one. I think attempting to make <canvas> play semantic is silly. <canvas> is a bit-buffer. It isn't accessible. I can't imagine that people will actually use fallback content in this way (or bother to keep it up to date), and it seems like a large hack to our engine to support this feature. :( You could say the same about other accessibility features, but most popular JavaScript widget libraries implement ARIA because all it takes is one passionate contributor to implement it once, and thousands of sites benefit. I expect the same to happen for canvas; just because most developers won't bother doesn't mean it's not worthwhile. As for the spec, the cat is already out of the bag, as IE and Firefox stable releases already implement it as written. Also note that other proposed additions to the canvas spec, like hit regions, build on the idea of embedded content: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#hit-regions Would it help at all if a canvas had to set an attribute indicating it wants embedded content? That'd be a minor change to the spec and it wouldn't totally break existing implementations in other browsers. It might feel like less of a hack in WebKit because we'd only be changing behavior when the web author explicitly wants to take advantage of it. Either way, this feature is important to those of us who work on accessibility. I'm sorry the only solutions so far feel like hacks, but I'd really like to find a way we could make this work.

I think the idea of hit regions make sense in my mental model of canvas. They're programatic/immediate, just like canvas is. I also think it's OK, the idea of associating hit-regions with elements, as elements are the basic building blocks of a page and currently our only AX API for the web. I think that for simplicity in WebKit, we should have a real render tree, for anything which is accessible/focusable/interactive. If that means the cost of implementing this feature for canvas is too great in the general case, then yes, I think having an option to turn it on would allow the few sites who want this, to use it. I think that authors who are serious about accessibility, are going to need to design their sites with more semantic concepts. Just like on the desktop. Sure you can manually tell VoiceOver (or whatever other AX software) about the content of your custom views, but it's much easier to use the built-in views to get all the AX for free. Similarly, authors who care about AX are going to want to use semantic content (be that SVG, or HTML, or XBL2, or whatever) instead of manual canvas calls. (Just like how they're going to want to use native <video> and subtitle support, instead of flash -- can you imagine someone trying to write an accessible canvas-based video player?) I guess this also feels a bit odd to me, as it feels like a one-off for canvas. It's not as though we're exposing a low-level AX API, like say VoiceOver does to its clients. Sure you can use the low-level general APIs on the desktop, but it's better to use the built-in views. On the web, (correct me if I'm wrong... but?) we only really have "built-in" element-based API, with some ARIA attributes. And then we've built this one-off for canvas, to map it back to that element-based model. Example: https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/Accessibility/cocoaAXAccessEnabling/cocoaAXAccessEnabling.html#//apple_ref/doc/uid/20001059-89496 Anyway, to answer your question, I would feel better if we avoided the "it's to expensive to always create a rendering tree" problem, but having instead an option to turn it on when wanted, and to then just create a real rendering tree for the stuff under canvas, which just does not paint. Maybe that will turn out uglier, but that would be my default approach to solving this.

(In reply to comment #17) > I guess this also feels a bit odd to me, as it feels like a one-off for canvas. It's not as though we're exposing a low-level AX API, like say VoiceOver does to its clients. Sure you can use the low-level general APIs on the desktop, but it's better to use the built-in views. On the web, (correct me if I'm wrong... but?) we only really have "built-in" element-based API, with some ARIA attributes. And then we've built this one-off for canvas, to map it back to that element-based model. On the web you can use ARIA to make arbitrary custom controls accessible, just like on the desktop - you're not limited to only the built-in accessible form controls. For example, it's easy to make a custom listbox just as accessible as a native HTMLSelectElement by managing focus and adding the right ARIA attributes to the parent and child elements. All this change does is make it possible to do the same for a canvas. Right now, if I build a custom listbox using divs and spans, I can make it accessible using ARIA. If I build a custom listbox using a canvas, it's impossible for me to do the same thing because I can't set focus on a child that's within the canvas and give it an ARIA role. This proposal would just level the playing field so that building the visual part of a new custom control using canvas doesn't have to be less accessible. > Anyway, to answer your question, I would feel better if we avoided the "it's to expensive to always create a rendering tree" problem, but having instead an option to turn it on when wanted, and to then just create a real rendering tree for the stuff under canvas, which just does not paint. Maybe that will turn out uglier, but that would be my default approach to solving this. So how about if I create a patch so that if a canvas has an attribute like x-webkit-embedded-content, then it creates renderers for its descendants, but not otherwise? Does that sound like a reasonable path forwards? (I'd also propose this spec change.)

(In reply to comment #18) > (In reply to comment #17) > > I guess this also feels a bit odd to me, as it feels like a one-off for canvas. It's not as though we're exposing a low-level AX API, like say VoiceOver does to its clients. Sure you can use the low-level general APIs on the desktop, but it's better to use the built-in views. On the web, (correct me if I'm wrong... but?) we only really have "built-in" element-based API, with some ARIA attributes. And then we've built this one-off for canvas, to map it back to that element-based model. > > On the web you can use ARIA to make arbitrary custom controls accessible, just like on the desktop - you're not limited to only the built-in accessible form controls. For example, it's easy to make a custom listbox just as accessible as a native HTMLSelectElement by managing focus and adding the right ARIA attributes to the parent and child elements. > > All this change does is make it possible to do the same for a canvas. Right now, if I build a custom listbox using divs and spans, I can make it accessible using ARIA. If I build a custom listbox using a canvas, it's impossible for me to do the same thing because I can't set focus on a child that's within the canvas and give it an ARIA role. This proposal would just level the playing field so that building the visual part of a new custom control using canvas doesn't have to be less accessible. > > > Anyway, to answer your question, I would feel better if we avoided the "it's to expensive to always create a rendering tree" problem, but having instead an option to turn it on when wanted, and to then just create a real rendering tree for the stuff under canvas, which just does not paint. Maybe that will turn out uglier, but that would be my default approach to solving this. > > So how about if I create a patch so that if a canvas has an attribute like x-webkit-embedded-content, then it creates renderers for its descendants, but not otherwise? Does that sound like a reasonable path forwards? > IE and firefox both don't require any special flags. I also don't see why having Renderers is a requirement in this case. Accessibility already makes objects out of Widgets, Scroll views, mock elements, renderers, etc. This just adds another type of object that Accessibility can rely on. I can also see this technique being applied to WebGL and other bit bucket type drawing areas that come out in the future. > (I'd also propose this spec change.)

This spec definitely has some unpleasant implications, but as stated the cat is out of the bag. I'm also not a huge fan of adding a flag to Document for this, but if we intend on supporting this I don't have a better proposal.

Comment on attachment 147145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147145&action=review > Source/WebCore/dom/Node.cpp:917 > + // An node without a renderer is focusable if it's within canvas fallback content > + // and the canvas has a renderer. Isn't it only focusable if canvas has designated it so? > Source/WebCore/html/HTMLCanvasElement.cpp:146 > + if (document()->canAssumeNoCanvasWithElementChildren()) { > + Node* first = beforeChange ? beforeChange->nextSibling() : firstChild(); > + for (Node* n = first; n != afterChange; n = n->nextSibling()) { > + if (n->isElementNode()) { > + document()->setCanAssumeNoCanvasWithElementChildren(false); > + break; > + } Don't you have to add a hit region via JS before these elements need to be hit at all?

Comment on attachment 147145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147145&action=review > Source/WebCore/dom/Document.cpp:498 > + , m_canAssumeNoCanvasWithElementChildren(true) Don't we have a DocumentRareData for per-document seldom used flags?

(In reply to comment #21) > (From update of attachment 147145 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147145&action=review > > > Source/WebCore/dom/Node.cpp:917 > > + // An node without a renderer is focusable if it's within canvas fallback content > > + // and the canvas has a renderer. > > Isn't it only focusable if canvas has designated it so? No, this is as intended. An element that would normally be focusable when outside of a canvas, should continue to be focusable - and yes, even part of the tab order - if inside of a canvas. This is how both IE and Firefox have implemented it. > > Source/WebCore/html/HTMLCanvasElement.cpp:146 > > + if (document()->canAssumeNoCanvasWithElementChildren()) { > > + Node* first = beforeChange ? beforeChange->nextSibling() : firstChild(); > > + for (Node* n = first; n != afterChange; n = n->nextSibling()) { > > + if (n->isElementNode()) { > > + document()->setCanAssumeNoCanvasWithElementChildren(false); > > + break; > > + } > > Don't you have to add a hit region via JS before these elements need to be hit at all? Hit regions makes this feature more generally useful, but an element in the canvas subtree should be focusable without it - including being part of the tab order, being able to manually focus, and receiving events while focused. (In reply to comment #22) > (From update of attachment 147145 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147145&action=review > > > Source/WebCore/dom/Document.cpp:498 > > + , m_canAssumeNoCanvasWithElementChildren(true) > > Don't we have a DocumentRareData for per-document seldom used flags? We don't, not that I could find at least. I think that since there are typically only a handful of Documents (but often many thousands of nodes), the small savings from putting some flags in DocumentRareData wouldn't justify the added complexity at this point. Alternative: I could add a bit to NodeRareData indicating if a node is part of a canvas subtree. I think it could be implemented with only constant overhead: when a node is attached, it inherits the flag from its parent, except for a canvas node where it's always true. The flag is cleared on detach. Does that sound better?

Created attachment 150212 [details] Patch

I tried using an extra bit in ElementRareData to mark elements that are in a canvas subtree, rather than an ugly global flag on the document. This look cleaner?

Comment on attachment 150212 [details] Patch Attachment 150212 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13106538 New failing tests: fast/events/tabindex-focus-blur-all.html

Created attachment 150251 [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

Created attachment 150258 [details] Fix failing test

Comment on attachment 150258 [details] Fix failing test View in context: https://bugs.webkit.org/attachment.cgi?id=150258&action=review i think this patch looks ok. using rare data will ensure it has minimal impact for anyone not trying to make canvas acceptable. i'm ready to r+ unless someone has serious objections. > Source/WebCore/html/HTMLFormControlElement.cpp:315 > + if (renderer() && (!renderer()->isBox() || toRenderBox(renderer())->size().isEmpty())) you might want to add a comment here that non-renderer objects that live inside canavs can also take focus

Comment on attachment 150258 [details] Fix failing test r=me. i think this approach will let us address the accessibility aspect without complicating non-accessibility code. we can revisit if we find any serious implications of this approach

Created attachment 152015 [details] Patch

(In reply to comment #30) > (From update of attachment 150258 [details]) > r=me. > i think this approach will let us address the accessibility aspect without complicating non-accessibility code. > we can revisit if we find any serious implications of this approach Thanks. I added a comment where you suggested, and I think this has addressed all of the feedback, so let's give it a try. The next step will be to make some corresponding changes to the accessibility code.

Created attachment 152140 [details] Patch for landing

Comment on attachment 152140 [details] Patch for landing Clearing flags on attachment: 152140 Committed r122550: <http://trac.webkit.org/changeset/122550>

All reviewed patches have been landed. Closing bug.