[webkit-reviews] review denied: [Bug 87898] Should be possible to focus elements within canvas fallback content : [Attachment 144925] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Dominic Mazzoni
<dmazzoni at google.com>'s request for review:
Bug 87898: Should be possible to focus elements within canvas fallback content
https://bugs.webkit.org/show_bug.cgi?id=87898

Attachment 144925: Patch
https://bugs.webkit.org/attachment.cgi?id=144925&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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