[webkit-reviews] review granted: [Bug 71788] <area>-tag within <map> can get focus when it is hidden : [Attachment 133194] Patch.

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has granted Antaryami Pandia
<antaryami.pandia at motorola.com>'s request for review:
Bug 71788: <area>-tag within <map> can get focus when it is hidden
https://bugs.webkit.org/show_bug.cgi?id=71788

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

------- Additional Comments from Andy Estes <aestes at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=133194&action=review


This looks good! r=me. I have a few comments that I'd like to see addressed
before you land.

> Source/WebCore/ChangeLog:3
> +	   <area>-tag within <map> can get focus when it is hidden.

I think we could should change the title of this bug to better capture the
issue, since it's the <img> that's hidden, not the <map> or <area>. How about:

"An <area> element remains focusable even though its associated <img> is not
rendered."

> Source/WebCore/ChangeLog:8
> +	   The "HTMLAreaElement::isFocusable" method need to consider

This should read "HTMLAreaElement::isFocusable() needs to consider..."

> Source/WebCore/ChangeLog:16
> +	   * html/HTMLAreaElement.h: Made imageElement() as const.

This should read "Make imageElement() const."

> Source/WebCore/html/HTMLAreaElement.cpp:204
> +    if (!(image && image->renderer()) ||
image->renderer()->style()->visibility() != VISIBLE)

I think this would read better as:

if (!image || image->renderer() || image->renderer()->style()->visibility() !=
VISIBLE)

> LayoutTests/ChangeLog:3
> +	   <area>-tag within <map> can get focus when it is hidden.

Same comment about the bug title.

> LayoutTests/ChangeLog:8
> +	   Tests to test the tab navigation.

"Tests to test..." doesn't sound right. You can remove this line, or say
something like "Test sequential focus navigation."

> LayoutTests/fast/events/tab-test-not-visible-imagemap.html:49
> +    description("Testcase to test that tabbing does not focus area element
which is not visible.");

You can remove "Testcase to test...". It reads fine as "Test that tabbing...".