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

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

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

Comment 1

[Dave Webber]

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";
...

Comment 2

[Mounir Lamouri (:mounir)]

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

[bluej100]

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.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Mark input type="hidden" controls as not labelable

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!

Comment 5

[Josh Matthews [:jdm]]

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

Comment 6

[bluej100]

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

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Label controls don't apply to input controls with type="hidden"

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

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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