Closed Bug 597650 Opened 10 years ago Closed 6 years ago

<label> should not apply on <input type='hidden'>

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: mounir, Assigned: agi)

References

Details

(Keywords: dev-doc-needed, html5)

Attachments

(1 file, 1 obsolete file)

<input type='hidden'> should not be a labelable control. This has been added in whatwg html specs with r5432.
Version: unspecified → Trunk
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";
...
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.
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)
Assignee: nobody → agi.novanta
Status: NEW → ASSIGNED
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.
Ah, this is in my feedback queue which I tend to process after I've done with reviews.
Flags: needinfo?(bugs)
Attachment #8424359 - Flags: feedback?(bugs) → review?(bugs)
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+
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)
Attachment #8437437 - Flags: review?(bugs) → review+
Did a try to check non-mochitest tests: https://tbpl.mozilla.org/?tree=Try&rev=63495bb86a60
Thank you!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/94bf10b5bbeb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.