Summary: | An <area> element remains focusable even though its associated <img> is not rendered. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Emiel <emiel116> | ||||||||||||||||
Component: | Layout and Rendering | Assignee: | 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: |
|
Created attachment 114033 [details]
Correct example of bug occurring
The previous file did not have a hidden row in the table.
Is there anyone who can look into this bug? I am looking at the bug. Will upload a patch soon after preparing layout test. Created attachment 132541 [details]
Proposed Patch.
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 Created attachment 132564 [details]
Patch.
Patch.
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.
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. Created attachment 132785 [details]
Modified patch.
(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 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(); (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. (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. (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. Created attachment 133194 [details]
Patch.
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...". (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. (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. (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. Created attachment 133752 [details]
Patch.
Updated patch after modified bug title and review comments.
Comment on attachment 133752 [details] Patch. Clearing flags on attachment: 133752 Committed r112134: <http://trac.webkit.org/changeset/112134> All reviewed patches have been landed. Closing bug. 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 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. |
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.