Mounir Lamouri (:mounir) Reporter
	Description • 14 years ago

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

	Dave Webber
	Comment 1 • 14 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";
...

	Mounir Lamouri (:mounir) Reporter
	Comment 2 • 14 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.

	bluej100
	Comment 3 • 10 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.

	[ex-Mozilla] Agi Sferro | :agi 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!

	Josh Matthews [:jdm]
	Comment 5 • 10 years ago

It would be nice if you could address this patch one way or another, Olli.

	bluej100
	Comment 6 • 10 years ago

The patch looks right to me, as long as IsLabelable is used for association by structure as well as assocation by ID.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 10 years ago

Ah, this is in my feedback queue which I tend to process after I've done with reviews.

	Olli Pettay [:smaug][bugs@pettay.fi]
	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.

	[ex-Mozilla] Agi Sferro | :agi 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

	[ex-Mozilla] Agi Sferro | :agi Assignee
	Comment 10 • 10 years ago

Did a try to check non-mochitest tests: https://tbpl.mozilla.org/?tree=Try&rev=63495bb86a60
Thank you!

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/94bf10b5bbeb

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/94bf10b5bbeb