Bug 71788

Summary: An <area> element remains focusable even though its associated <img> is not rendered.
Product: WebKit Reporter: Emiel <emiel116>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, antaryami.pandia, darin, dglazkov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Example of bug occurring.
none
Correct example of bug occurring
none
Proposed Patch.
webkit.review.bot: commit-queue-
Patch.
aestes: review-, aestes: commit-queue-
Modified patch.
aestes: review-, aestes: commit-queue-
Patch.
aestes: review+, aestes: commit-queue-
Patch. none

Description Emiel 2011-11-08 02:48:44 PST
Created attachment 114021 [details]
Example of bug occurring.

When a <map> is included in an element that is not visible, you can still use tab to reach the different areas within the map. For the user, this looks like focus has been lost while using tab to go through a form.

See the attached file for an example.
Comment 1 Emiel 2011-11-08 03:48:20 PST
Created attachment 114033 [details]
Correct example of bug occurring

The previous file did not have a hidden row in the table.
Comment 2 Emiel 2011-12-13 05:31:07 PST
Is there anyone who can look into this bug?
Comment 3 Antaryami Pandia (apandia) 2012-03-18 21:13:26 PDT
I am looking at the bug. Will upload a patch soon after preparing layout test.
Comment 4 Antaryami Pandia (apandia) 2012-03-18 21:52:23 PDT
Created attachment 132541 [details]
Proposed Patch.
Comment 5 WebKit Review Bot 2012-03-18 22:26:03 PDT
Comment on attachment 132541 [details]
Proposed Patch.

Attachment 132541 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11983418

New failing tests:
fast/events/tab-imagemap.html
fast/events/imagemap-norender-crash.html
Comment 6 Antaryami Pandia (apandia) 2012-03-19 02:49:53 PDT
Created attachment 132564 [details]
Patch.

Patch.
Comment 7 Andy Estes 2012-03-20 01:35:38 PDT
Comment on attachment 132564 [details]
Patch.

This doesn't seem like the right approach. For one, I don't think checking for a renderer handles all visibility cases properly (e.g. visibility:hidden).

To me, the bug appears to be that HTMLAreaElement::isKeyboardFocusable() is lying to us by returning true even though the element is not visible and therefore not focusable. Other focusable nodes return false from isKeyboardFocusable() in this case (e.g. HTMLFormControlElement), so HTMLAraeElement should do this as well. In fact, it looks like the logic you want already exists in Node::isFocusable(), so the bug appears to be that HTMLAreaElement isn't properly delegating to its superclass.
Comment 8 Antaryami Pandia (apandia) 2012-03-20 03:09:35 PDT
Thanks for the review. Will upload a modified patch.

Just one query regs "Node::isFocusable".
if (!renderer() || renderer()->style()->visibility() != VISIBLE)

I think only "if (!renderer())" should work. As per my understanding hidden and display none element doesn't have renderer.

(In reply to comment #7)
> (From update of attachment 132564 [details])
> This doesn't seem like the right approach. For one, I don't think checking for a renderer handles all visibility cases properly (e.g. visibility:hidden).
> 
> To me, the bug appears to be that HTMLAreaElement::isKeyboardFocusable() is lying to us by returning true even though the element is not visible and therefore not focusable. Other focusable nodes return false from isKeyboardFocusable() in this case (e.g. HTMLFormControlElement), so HTMLAraeElement should do this as well. In fact, it looks like the logic you want already exists in Node::isFocusable(), so the bug appears to be that HTMLAreaElement isn't properly delegating to its superclass.
Comment 9 Antaryami Pandia (apandia) 2012-03-20 03:11:53 PDT
Created attachment 132785 [details]
Modified patch.
Comment 10 Andy Estes 2012-03-20 11:32:08 PDT
(In reply to comment #8)
> Thanks for the review. Will upload a modified patch.
> 
> Just one query regs "Node::isFocusable".
> if (!renderer() || renderer()->style()->visibility() != VISIBLE)
> 
> I think only "if (!renderer())" should work. As per my understanding hidden and display none element doesn't have renderer.

No, that's not right. Elements with visibility:hidden have renderers. That's why Node::isFocusable() handles that case separately.
Comment 11 Andy Estes 2012-03-20 11:36:34 PDT
Comment on attachment 132785 [details]
Modified patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=132785&action=review

r- because of the visibility:hidden issue. You should also add test coverage for visibility:hidden.

> Source/WebCore/html/HTMLAreaElement.cpp:203
> +    HTMLImageElement* imageElem = imageElement();

We discourage abbreviations when naming variables. You could just call this 'image'.

> Source/WebCore/html/HTMLAreaElement.cpp:205
> +    if (!(imageElem && imageElem->renderer()))
> +        return false;

As I said before, this check won't handle the case where the image has 'visibility:hidden' set, but that case is already handled by Node's implementation of isFocusable(). I think you can just change these two lines to:

    return image->isFocusable();
Comment 12 Antaryami Pandia (apandia) 2012-03-20 23:57:18 PDT
(In reply to comment #11)
> No, that's not right. Elements with visibility:hidden have renderers. That's why Node::isFocusable() handles that case separately.

Thanks for the information.

> r- because of the visibility:hidden issue. You should also add test coverage for visibility:hidden.

will add visibility:hidden case.

> > Source/WebCore/html/HTMLAreaElement.cpp:203
> > +    HTMLImageElement* imageElem = imageElement();
> 
> We discourage abbreviations when naming variables. You could just call this 'image'.

oops.

> As I said before, this check won't handle the case where the image has 'visibility:hidden' set, but that case is already handled by Node's implementation of isFocusable(). I think you can just change these two lines to:
> 
>     return image->isFocusable();

will do.
Comment 13 Antaryami Pandia (apandia) 2012-03-21 02:45:50 PDT
(In reply to comment #11)
> > Source/WebCore/html/HTMLAreaElement.cpp:205
> > +    if (!(imageElem && imageElem->renderer()))
> > +        return false;
> 
> As I said before, this check won't handle the case where the image has 'visibility:hidden' set, but that case is already handled by Node's implementation of isFocusable(). I think you can just change these two lines to:
> 
>     return image->isFocusable();

If I do "image->isFocusable()" the below test cases fails:-

  fast/events/imagemap-norender-crash.html = TEXT
  fast/events/mouse-focus-imagemap.html = TEXT
  fast/events/tab-imagemap.html = TEXT

The reason being, in "Node::supportsFocus()" the check for "hasRareData()" fails.

So i think we can check for the display/visibility case in "HTMLAreaElement::isFocusable()" as:-

bool HTMLAreaElement::isFocusable() const
{
    HTMLImageElement* image = imageElement();
    if (!(image && image->renderer()) || image->renderer()->style()->visibility() != VISIBLE)
        return false;

    return supportsFocus() && Element::tabIndex() >= 0;
}

Please provide your feedback.
Comment 14 Andy Estes 2012-03-21 14:18:26 PDT
(In reply to comment #13)
> (In reply to comment #11)
> > > Source/WebCore/html/HTMLAreaElement.cpp:205
> > > +    if (!(imageElem && imageElem->renderer()))
> > > +        return false;
> > 
> > As I said before, this check won't handle the case where the image has 'visibility:hidden' set, but that case is already handled by Node's implementation of isFocusable(). I think you can just change these two lines to:
> > 
> >     return image->isFocusable();
> 
> If I do "image->isFocusable()" the below test cases fails:-
> 
>   fast/events/imagemap-norender-crash.html = TEXT
>   fast/events/mouse-focus-imagemap.html = TEXT
>   fast/events/tab-imagemap.html = TEXT
> 
> The reason being, in "Node::supportsFocus()" the check for "hasRareData()" fails.
> 
> So i think we can check for the display/visibility case in "HTMLAreaElement::isFocusable()" as:-
> 
> bool HTMLAreaElement::isFocusable() const
> {
>     HTMLImageElement* image = imageElement();
>     if (!(image && image->renderer()) || image->renderer()->style()->visibility() != VISIBLE)
>         return false;
> 
>     return supportsFocus() && Element::tabIndex() >= 0;
> }
> 
> Please provide your feedback.

Looks good to me.
Comment 15 Antaryami Pandia (apandia) 2012-03-21 23:22:22 PDT
Created attachment 133194 [details]
Patch.
Comment 16 Andy Estes 2012-03-22 00:42:41 PDT
Comment on attachment 133194 [details]
Patch.

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...".
Comment 17 Andy Estes 2012-03-22 00:46:27 PDT
(In reply to comment #16)
> (From update of attachment 133194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133194&action=review
> > 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)

Oops, my suggestion broke your patch! I meant:

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

I think this is more straight-forward than negating a subexpression.
Comment 18 Antaryami Pandia (apandia) 2012-03-22 01:34:52 PDT
(In reply to comment #16)
> 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."

Right. But I can't modify the title, since I am not the reporter.
Can you or the reporter please do the same.
 
> > Source/WebCore/ChangeLog:8
> > +        The "HTMLAreaElement::isFocusable" method need to consider
> 
> This should read "HTMLAreaElement::isFocusable() needs to consider..."

will do.

> > Source/WebCore/ChangeLog:16
> > +        * html/HTMLAreaElement.h: Made imageElement() as const.
> 
> This should read "Make imageElement() const."

will do.
 
> > 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)

will do as your suggested in last comment.
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.

will do.

> > 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."

ok.

> > 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...".

Ok.

I will upload a new patch once the title is modified.
Comment 19 Andy Estes 2012-03-22 09:08:52 PDT
(In reply to comment #18)
> (In reply to comment #16)
> > 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."
> 
> Right. But I can't modify the title, since I am not the reporter.
> Can you or the reporter please do the same.

Done.
Comment 20 Antaryami Pandia (apandia) 2012-03-26 02:12:33 PDT
Created attachment 133752 [details]
Patch.

Updated patch after modified bug title and review comments.
Comment 21 WebKit Review Bot 2012-03-26 11:52:09 PDT
Comment on attachment 133752 [details]
Patch.

Clearing flags on attachment: 133752

Committed r112134: <http://trac.webkit.org/changeset/112134>
Comment 22 WebKit Review Bot 2012-03-26 11:52:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Antaryami Pandia (apandia) 2012-03-26 22:50:51 PDT
Hi Andy,
Just a offbeat request. I have requested for "EditBugs" permission on webkit-committers list. I am not sure if that is the right mailing list . Can you please help me getting "Edit Bugs" permission.

Thanks,
-Antaryami
Comment 24 Andy Estes 2012-03-27 11:28:39 PDT
Antaryami, I don't have the privileges in Bugzilla to set the EditBugs bit, and I don't see an email from you to webkit-committers. I'd recommend you email your request to webkit-dev; someone there should be able to do this for you.