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

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: zcorpan, Assigned: bzbarsky)

Tracking

({dev-doc-complete, site-compat})

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
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
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)?
Attachment #8755537 - Flags: review?(surkov.alexander)
Attachment #8755537 - Flags: review?(hsivonen)
Attachment #8755537 - Flags: review?(bkelly)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.)
Attachment #8755537 - Flags: review?(hsivonen) → review+
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.
Attachment #8755537 - Flags: review?(bkelly) → review+
(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?
> 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?
Flags: needinfo?(surkov.alexander)
(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.
Flags: needinfo?(surkov.alexander) → needinfo?(mzehe)
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.
Attachment #8755537 - Flags: review?(surkov.alexander) → review+
(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.
Flags: needinfo?(mzehe)

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c4bde82ab593
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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.
Flags: needinfo?(bzbarsky)
Keywords: site-compat
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.
Flags: needinfo?(bzbarsky)
The documentation for HTMLLabelElement.form is still missing. We should use this opportunity to create it. Sheppy, can you do it? Thx.
Flags: needinfo?(eshepherd)
(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.
Flags: needinfo?(eshepherd)
https://hg.mozilla.org/projects/htmlparser/rev/351b3eb0334d5667b6363d25bf9586f12e90ac38
Mozilla bug 1268852 - Change <label> elements to not be form-associated anymore. r=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.
My apologies; I hadn't realized there was a separate htmlparser repo I needed to land in.  :(
You need to log in before you can comment on or make changes to this bug.