1.19 KB, text/html
1.18 KB, text/html
774 bytes, text/html
516 bytes, text/html
545 bytes, text/html
235 bytes, text/html
5.40 KB, application/xhtml+xml
12.75 KB, patch
|Details | Diff | Splinter Review|
982 bytes, text/html
Created attachment 128314 [details] Testcase for selecting input inside a label with for attribute Try to type something in the text input field. It's harder than one would expect...
This is invalid HTML. If a control is within a LABEL, then the "for" attribute shouldn't be used.
I agree that the *correct behaviour* for the test case isn't that clear. However, if you want to claim that the attached test case contains invalid HTML, please, post some evidence. http://validator.w3.org/ says it's valid XHTML 1.1. Strict (I know, that validator isn't perfect) and I cannot locate anything in the spec that disallows such construct. The spec says that a label can be associated with exactly one control. I read the as if a LABEL element contains both FOR attribute and control as it's content, then the FOR attribute should override the behaviour and the content should be inaccessible (or at least, one cannot focus it). I *hope* that really isn't the intent of the spec.
> I *hope* that really isn't the intent of the spec. Seems to be pretty clearly the intent to me -- clicking the contents of the label should trigger the label's default click action when the click event bubbles up, and the default click action of a label with a "for" attribute is to focus the control identified by the "for" attribute. The spec does clearly state that the "for" attribute association overrides the "containment" association, so the label is definitly associated with the radio button, not the textfield. So where is the bug, exactly?
I would expect that clicking inside the text control itself should not trigger the label behaviour, but instead the focus should stay with that control.
Should clicking inside the text control fire the label's onfocus event handler? What about its onclick event handler?
Created attachment 128368 [details] Testcase Hmm, perhaps this is correct behaviour after all... It also seems compatible to what Opera and IE does.
*** Bug 222744 has been marked as a duplicate of this bug. ***
*** Bug 249339 has been marked as a duplicate of this bug. ***
*** Bug 254247 has been marked as a duplicate of this bug. ***
I reproduced this bug in 'e-casio' that is major shopping site in Japanese with 2004081208-trunk/WinXP.
Assignee: core.layout.form-controls → nobody
QA Contact: desale → core.layout.form-controls
# In any case, events targeted at form controls within a label must not be # handled by the label itself. -- http://whatwg.org/specs/web-forms/current-work/#label
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'm finding a related problem with some web pages, and was hoping you guys could tell me if it's an exact dupe or not. I'll create an attachment which follows. The bug occurs if you have an input field ("field 2") which is enclosed within a label, and the 'for' property of that label is the same as the 'id' property of a previous field ("field 1", which is not associated with the label). In that situation, the focus for field 2 always jumps back to field 1. It doesn't do it if the input tag for field 2 is outside the label though. Culd you please have a look at the following attachment, and let me know if it's this bug?
Created attachment 211255 [details] This one doesn't show the bug As a contrast, I'm also attaching a version where the <input> tage for field 2 isn't enclosed by the <label>. In this case, the field behaves normally.
Created attachment 220454 [details] Shows that even the for attribute is not required to reproduce this bug This test case shows that even a "for" attribute is not required to reproduce this.
Attachment #220454 - Attachment mime type: text/plain → text/html
Flags: blocking1.9a1? → blocking1.9+
Assignee: nobody → mats.palmgren
Summary: Unable to focus INPUT element inside a LABEL element → [FIX] Unable to focus INPUT element inside a LABEL element
Created attachment 233569 [details] [diff] [review] Patch rev. 1 This patch tries to implement: http://whatwg.org/specs/web-forms/current-work/#label "In any case, events targeted at form controls (or other interactive elements, e.g. links) within a label must not be handled by the label itself."
(In reply to comment #19) > Created an attachment (id=233569)  > Patch rev. 1 That seems to fix all the testcases for me, including my own. Nice!
Hmm... I'm still not that happy with Hixie's spec, but I guess it's the right general idea. I have a couple of questions about the patch: 1) Should we be using originalTarget, rather than target? It would seem to me that we should be. 2) The hardcoding of various types of things is very fragile, and reusing existing interfaces for testing can lead to subtle bugs. For example, the patch as written treats <fieldset> as interactive, while an <img> with an imagemap is not, as far as I can tell. Although maybe the latter case is handled by the events being dispatched to the <area> instead? In any case, there are also inconsistencies as far as <link> elements and <a> with no href in the JS and the C++. 3) <object> and <iframe> can be interactive, depending on what's loaded in them. What I feel we should do is to introduce a method to ask a node whether it's interactive. Probably put it on nsIContent and maybe also expose it on nsIDOMNSSomething (not sure what would be most appropriate) so that the XUL code can work right. Then go through every single one of our concrete element classes and see what they should return. Note that all of this breaks down if the page ever uses an onclick handler, but in that case they should be able to use preventDefault to prevent the default action, right? Which raises the basic question in my mind yet again: why are some of these default actions not simply calling preventDefault? One last thing -- I think this is the sort of patch where more than one review might be a good idea; I suggest jst or sicking.
Comment on attachment 233569 [details] [diff] [review] Patch rev. 1 r- per comment 21
Created attachment 297596 [details] Test case 8 with multiple inputs This test case shows that a label (1) that triggers an input (A) that is in a different label (2) will be forwarded to the input (B) that label 2 refers to (so 1/A causes B to be selected) and this can continue over several inputs/labels. Sorry for the difficulty in explaining it; the source makes more sense. The source demonstrates why this could easily occur by not closing the label tag as well as what happens when it is closed. This is a core issue present in FF3.0b3pre and SeaMonkey 2.0a1pre, Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008011704 Minefield/3.0b3pre and Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008011701 SeaMonkey/2.0a1pre respectively.
No longer blocks: 338303
Duplicate of this bug: 338303
Is the reasoning in comment 21 still valid today? I think that the currently implemented behavior is not sane and some kind of change should be done. As far as I understand, the suggested patch fixes all test cases but it *could* cause regressions in some (uknown?) cases. Is there any real word example of such a page?
> Is the reasoning in comment 21 still valid today? Which reasoning? > As far as I understand, the suggested patch fixes all test cases All the ones attached to this bug. Comment 21 mentions some test cases it breaks. > Is there any real word example of such a page? Are there any real world pages with imagemaps? Probably yes. ;)
More to the point, the idea of code review is to make sure we don't check in buggy code. Trading one set of bugs for another may be required in some cases, but it's so not required in this case. We should just fix this with code that doesn't introduce other bugs.
(In reply to comment #31) > > Is the reasoning in comment 21 still valid today? > > Which reasoning? Comment 22 refers to comment 21 for the reason to fail the review. I was just wondering the the reason to fail the review is still valid with the current code base. (A lot has changed since 2006.) > > Is there any real word example of such a page? > > Are there any real world pages with imagemaps? Probably yes. ;) Does the patch break imagemaps even if there is not a wrapping label? I though that the patch was supposed to fix the "interactive element inside a label" case. I would guess that there are no real world examples of imagemaps inside a label. I do understand the idea of code review and I totally support it. I'd just prefer to see more clear reasoning for failed code review (especially what needs to be changed or what part will not be accepted). In case of open source projects, it's even more important because that failed review may be the best hint to somebody else to improve the code if the original patch author has lost the interest to improve the patch. In case of this specific patch and comment 21 I'm not sure if the review was failed because of implementation quality or because of the logic/algorithms used. This makes a huge difference in case somebody tries to improve the patch.
> I was just wondering the the reason to fail the review is still valid with the > current code base. They are. > I would guess that there are no real world examples of imagemaps inside a > label. I would guess you would be wrong... > especially what needs to be changed or what part will not be accepted I really don't know how more clear the part of comment 21 after the 3-item list can be. It explicitly says what needs to be changed! > I'm not sure if the review was failed because of implementation quality or > because of the logic/algorithms used. I have no idea where you're drawing the distinction between those. The main problem with that patch is that the way it determines whether an element is "interactive" is fundamentally broken.
(In reply to comment #34) > The main problem with that patch is that the way it determines > whether an element is "interactive" is fundamentally broken. OK. I get it now. I just hope that that sentence had been part of comment 21.
FWIWI, the Opera behavior is now specified in HTML specs. Though, I would have prefered the Webkit one.
Summary: [FIX] Unable to focus INPUT element inside a LABEL element → Unable to focus INPUT element inside a LABEL element
Testing this against other browser's behavior, it seems browser behaviors differ but only Firefox focuses the radio button instead of the textfield. Against the first test case (https://bugzilla.mozilla.org/attachment.cgi?id=128314) browsers have the following behaviors when clicking on the INPUT field within the LABEL tag: Mac/Safari 6.0.5: Radio button is selected and textfield is focused. Mac/Chrome 28.0.1500.95: Radio button is selected and textfield is focused. Win/IE 8/9/10: Radio button NOT selected and the textfield is focused. Mac/Opera 12.02: Radio button NOT selected and the textfield is focused. Mac/Firefox 22.0: Radio button selected and the textfield is NOT focused. IMO, the Safari/Chrome behavior is the best from a UX perspective, but even the IE/Opera approach is better than what Firefox is doing currently.
(In reply to Boris Zbarsky [:bz] from comment #21) > What I feel we should do is to introduce a method to ask a node whether it's > interactive. Probably put it on nsIContent and maybe also expose it on > nsIDOMNSSomething (not sure what would be most appropriate) so that the XUL > code can work right. Then go through every single one of our concrete > element classes and see what they should return. Should this be done in webidl now (rather than nsIDOMNSSomething), with [Func="IsChromeOrXBL"] (on HTMLElement.webidl? elsewhere?), if one wanted to have a go at fixing this? Asking for a friend.
> Should this be done in webidl now Yes. Where to put it is non-obvious, but putting it on Element or HTMLElement are the clear options. Element seems better to me.
You need to log in before you can comment on or make changes to this bug.