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

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Emiel]

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.

[Emiel]

Created attachment 114033 [details]
Correct example of bug occurring

The previous file did not have a hidden row in the table.

[Emiel]

Is there anyone who can look into this bug?

[Antaryami Pandia (apandia)]

I am looking at the bug. Will upload a patch soon after preparing layout test.

[Antaryami Pandia (apandia)]

Created attachment 132541 [details]
Proposed Patch.

[WebKit Review Bot]

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

[Antaryami Pandia (apandia)]

Created attachment 132564 [details]
Patch.

Patch.

[Andy Estes]

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.

[Antaryami Pandia (apandia)]

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.

[Antaryami Pandia (apandia)]

Created attachment 132785 [details]
Modified patch.

[Andy Estes]

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

[Andy Estes]

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();

[Antaryami Pandia (apandia)]

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

[Antaryami Pandia (apandia)]

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

[Andy Estes]

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

[Antaryami Pandia (apandia)]

Created attachment 133194 [details]
Patch.

[Andy Estes]

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

[Andy Estes]

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

[Antaryami Pandia (apandia)]

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

[Andy Estes]

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

[Antaryami Pandia (apandia)]

Created attachment 133752 [details]
Patch.

Updated patch after modified bug title and review comments.

[WebKit Review Bot]

Comment on attachment 133752 [details]
Patch.

Clearing flags on attachment: 133752

Committed r112134: <http://trac.webkit.org/changeset/112134>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antaryami Pandia (apandia)]

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

[Andy Estes]

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.