1268852 - Spec change: Remove <label form> and redefine label.form IDL attribute

Status: RESOLVED FIXED

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

Description

[Simon Pieters [:zcorpan]]

label.form IDL attribute should be an alias for label.control.form.
<label form> content attribute should have no effect.

Spec change
https://github.com/whatwg/html/pull/1120

Tests
https://github.com/w3c/web-platform-tests/pull/2926

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Change <label> elements to not be form-associated anymore

Ben, can you review this in general?  Henri, can you review the HTML parser changes?  Alexander, can you review the behavior change in Accessible::RelationByType (see commit message)?

Comment 2

[Henri Sivonen (:hsivonen)]

Comment on attachment 8755537 [details] [diff] [review]
Change <label> elements to not be form-associated anymore

Review of attachment 8755537 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the parser changes. (That is, as requested, I only looked at the parser changes.)

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8755537 [details] [diff] [review]
Change <label> elements to not be form-associated anymore

Review of attachment 8755537 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, although it was all new code to me.  I did not review the parser code.

::: dom/html/HTMLFormControlsCollection.cpp
@@ +64,5 @@
>    //
>    // NS_FORM_INPUT_IMAGE
> +  //
> +  // XXXbz maybe we should just check for that type here instead of the big
> +  // switch?

How about we just make this a:

  MOZ_ASSERT(aFormControl->GetType() == NS_FORM_INPUT_IMAGE)

Or NS_ASSERTION().  That seems to be the intent of the comment here.  Better to just make a strict assertion if we can.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Created attachment 8755537 [details] [diff] [review]
> Change <label> elements to not be form-associated anymore
> 
> Alexander, can you review the behavior change in
> Accessible::RelationByType (see commit message)?

it seems the patch misses a11y files?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> it seems the patch misses a11y files?

No, the whole point is the patch does not change any a11y files.  The behavior change is because <label> used to QI to nsIFormControl and no longer does, and the a11y code is QIing to nsIFormControl.  So if we wanted to preserve its behavior for <label> we'd need to check for <label> explicitly.  But was that behavior purposeful or accidental?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky [:bz] from comment #5)
> > it seems the patch misses a11y files?
> 
> No, the whole point is the patch does not change any a11y files.  The
> behavior change is because <label> used to QI to nsIFormControl and no
> longer does, and the a11y code is QIing to nsIFormControl.  So if we wanted
> to preserve its behavior for <label> we'd need to check for <label>
> explicitly.  But was that behavior purposeful or accidental?

I see, it's rather accidental behavior I would say, the labels don't have to have default_button relation, at least I don't see a reason why they would need it. Having said that I can't guarantee there's no some legacy code relying on it, but it should be safe to make the change. CC'ing Marco for extra opinion.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 8755537 [details] [diff] [review]
Change <label> elements to not be form-associated anymore

Review of attachment 8755537 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, no need to block the patch, if this is indeed an issue for a11y (quite unlikely I think), then it can be handled as follow up.

Comment 8

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #6)
> (In reply to Boris Zbarsky [:bz] from comment #5)
> > > it seems the patch misses a11y files?
> > 
> > No, the whole point is the patch does not change any a11y files.  The
> > behavior change is because <label> used to QI to nsIFormControl and no
> > longer does, and the a11y code is QIing to nsIFormControl.  So if we wanted
> > to preserve its behavior for <label> we'd need to check for <label>
> > explicitly.  But was that behavior purposeful or accidental?
> 
> I see, it's rather accidental behavior I would say, the labels don't have to
> have default_button relation, at least I don't see a reason why they would
> need it. Having said that I can't guarantee there's no some legacy code
> relying on it, but it should be safe to make the change. CC'ing Marco for
> extra opinion.
I agree. I am not aware of any AT ever using it. The default button relation makes much more sense on a focusable element like an input, if a screen reader exposes a function like "give me the default button for this form". But for labels... No. This is safe to land I believe.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4bde82ab593

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/c4bde82ab593

Comment 11

[Kohei Yoshino [:kohei]]

What was the actual change here? The summary reads "remove <label form>" but the comment 1 says "<label form> content attribute should have no effect". I'm confused.

Comment 12

[Boris Zbarsky [:bzbarsky]]

The change is as follows:

1)  The "form" content attribute on <label> doesn't do anything special anymore.  At least no more so than the "foo" or "grandpa" attributes.

2)  The .form IDL property on HTMLLabelElement instances returns the .form of the labeled control, if the labeled control has a .form IDL property, and null otherwise.

Comment 13

[Kohei Yoshino [:kohei]]

Thanks! Now I got it. Posted the site compat doc at https://www.fxsitecompat.com/en-CA/docs/2016/label-no-longer-has-form-attribute/

Comment 14

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLLabelElement
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label

Added to Firefox 49 for developers.

Comment 15

[Jean-Yves Perrier [:teoli]]

The documentation for HTMLLabelElement.form is still missing. We should use this opportunity to create it. Sheppy, can you do it? Thx.

Comment 16

[Eric Shepherd [:sheppy]]

(In reply to Jean-Yves Perrier [:teoli] from comment #15)
> The documentation for HTMLLabelElement.form is still missing. We should use
> this opportunity to create it. Sheppy, can you do it? Thx.

Will do. I didn't do this originally because none of the subpages for the interface have been written yet, because the page was written to the previous style where it was all on one page. I have updated the interface page and will write the subpages later tonight or tomorrow morning.

Comment 17

[Eric Shepherd [:sheppy]]

Page updated to use the current layout for interface reference overview pages:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLLabelElement

Pages created:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLLabelElement/control
https://developer.mozilla.org/en-US/docs/Web/API/HTMLLabelElement/form
https://developer.mozilla.org/en-US/docs/Web/API/HTMLLabelElement/htmlFor

Now this should be finished.

Comment 18

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/projects/htmlparser/rev/351b3eb0334d5667b6363d25bf9586f12e90ac38
Mozilla bug 1268852 - Change <label> elements to not be form-associated anymore. r=hsivonen.

Comment 19

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #18)
> https://hg.mozilla.org/projects/htmlparser/rev/
> 351b3eb0334d5667b6363d25bf9586f12e90ac38
> Mozilla bug 1268852 - Change <label> elements to not be form-associated
> anymore. r=hsivonen.

I looks like this didn't get landed to the htmlparser repo at the time of landing to m-c, so I landed the Java changes to the htmlparser repo now.

Comment 20

[Boris Zbarsky [:bzbarsky]]

My apologies; I hadn't realized there was a separate htmlparser repo I needed to land in.  :(