Closed
Bug 597650
Opened 13 years ago
Closed 10 years ago
<label> should not apply on <input type='hidden'>
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: mounir, Assigned: agi)
References
Details
(Keywords: dev-doc-needed, html5)
Attachments
(1 file, 1 obsolete file)
5.07 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
<input type='hidden'> should not be a labelable control. This has been added in whatwg html specs with r5432.
Reporter | ||
Updated•13 years ago
|
Version: unspecified → Trunk
Comment 1•13 years ago
|
||
But what should happen then when the type of a labelled input is changed from "hidden" to something else, or vise-versa? ... <td><label for="foo" id="fooLabel">Foo:</label></td> <td><input type="text" name="foo" id="foo" /></td> <td><label for="bar" id="barLabel">Bar:</label></td> <td><input type="hidden" name="bar" id="bar" /></td> ... document.getElementsById("foo").type = "hidden"; document.getElemetnsById("bar").type = "text"; ...
Reporter | ||
Comment 2•13 years ago
|
||
I guess it should not be labeled anymore. That's not a big deal when @for is used. It might be annoying when you have: <label> <input type='text'> <input> </label> The first input is labeled. Then, if you change the type, the second should be.
I ran into this in the wild with this structure: <label> <input type="hidden" name="foo" value="0" /> <input type="checkbox" name="foo" value="1" /> Checkbox label text </label> Chrome and Safari allow toggling the checkbox with the label. Firefox and IE10 do not.
Assignee | ||
Comment 4•10 years ago
|
||
I'm confused, shouldn't this be sufficient? Olli can you take a look at this? It's just marking input type="hidden" as not labelable. This is the loop that looks for a labelable item to apply the label to in HTMLLabelElement Element* element = doc->GetElementById(elementId); if (element && element->IsLabelable()) { return static_cast<nsGenericHTMLElement*>(element); } so it looks like it should be ok to just exclude hidden input controls. Thanks!
Attachment #8424359 -
Flags: feedback?(bugs)
Updated•10 years ago
|
Assignee: nobody → agi.novanta
Status: NEW → ASSIGNED
Comment 5•10 years ago
|
||
It would be nice if you could address this patch one way or another, Olli.
Flags: needinfo?(bugs)
The patch looks right to me, as long as IsLabelable is used for association by structure as well as assocation by ID.
Comment 7•10 years ago
|
||
Ah, this is in my feedback queue which I tend to process after I've done with reviews.
Flags: needinfo?(bugs)
Updated•10 years ago
|
Attachment #8424359 -
Flags: feedback?(bugs) → review?(bugs)
Comment 8•10 years ago
|
||
Comment on attachment 8424359 [details] [diff] [review] Mark input type="hidden" controls as not labelable > nsGenericHTMLFormElement::IsLabelable() const > { > // TODO: keygen should be in that list, see bug 101019. >- // TODO: NS_FORM_INPUT_HIDDEN should be removed, see bug 597650. > uint32_t type = GetType(); >- return type & NS_FORM_INPUT_ELEMENT || >+ return type & NS_FORM_INPUT_ELEMENT && >+ type != NS_FORM_INPUT_HIDDEN || Please use () (expr1 && expr2) || expr3 || ... This needs tests.
Attachment #8424359 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 9•10 years ago
|
||
Thanks for the review! I added some tests. Can you take a quick look at them Olli? Try: https://tbpl.mozilla.org/?tree=Try&rev=54a9f771cec4
Attachment #8424359 -
Attachment is obsolete: true
Attachment #8437437 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8437437 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Did a try to check non-mochitest tests: https://tbpl.mozilla.org/?tree=Try&rev=63495bb86a60 Thank you!
Keywords: checkin-needed
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/94bf10b5bbeb
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/94bf10b5bbeb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•