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

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: mounir, Assigned: agi)

Tracking

({dev-doc-needed, html5})

Trunk
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

9 years ago
<input type='hidden'> should not be a labelable control. This has been added in whatwg html specs with r5432.
Reporter

Updated

9 years ago
Version: unspecified → Trunk

Comment 1

9 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

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

Comment 3

5 years ago
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)

Updated

5 years ago
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)

Comment 6

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.