Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix form inheritance behavior for autocapitalize #3373

Merged
merged 1 commit into from
Jan 24, 2018

Conversation

rlanday
Copy link
Contributor

@rlanday rlanday commented Jan 18, 2018

Kent Tamura (tkent@chromium.org) noticed a discrepancy between the autocapitalize form inheritance behavior I'm implementing in Chrome (and which iOS Safari has already implemented) and the behavior as defined by the spec:
https://chromium-review.googlesource.com/c/chromium/src/+/872090

In particular, the spec currently says only <input> and <textarea> elements inherit autocapitalize (both the IDL attribute and the used behavior) from their form owner. iOS Safari implements this behavior for all form control elements. This discrepancy creates some subtle differences in behavior:

  • The IDL attribute for non-<input> non-<textarea> elements (e.g. <select>, <fieldset>, etc.) in a <form> with autocapitalize set won’t be inherited according to the current spec behavior, but will according to the behavior as actually implemented in WebKit.

  • Chrome supports contenteditable on <output> elements (it appears iOS Safari does not). This means if you write something like this:

<form autocapitalize="characters">
    <output contenteditable=true>value</output>
</form>

the spec says that the <output> should not inherit autocapitalize="characters", but WebKit does inherit it for the IDL attribute. (This case is kind of strange because although the <output> is a form field, its contents are only editable because it has contenteditable on it).

This pull request changes the spec to match the iOS Safari behavior and the behavior I intend to implement in Chrome. The new behavior is that the IDL attribute is inherited for all form-associated elements with a form owner, and for the oddball <output contenteditable> case, this is reflected in the used autocapitalization value as well.


/form-elements.html ( diff )
/forms.html ( diff )
/indices.html ( diff )
/input.html ( diff )
/interaction.html ( diff )

@rlanday
Copy link
Contributor Author

rlanday commented Jan 18, 2018

I'm actually not sure "form-associated element" is what we want either. These are the form-associated elements:

https://html.spec.whatwg.org/multipage/forms.html#categories

  • button
  • fieldset
  • input
  • object
  • output
  • select
  • textarea
  • img

However, WebKit currently only applies form inheritance to elements it considers to be "form controls" (elements implemented by subclasses of HTMLFormControlElement). This includes:

  • button
  • fieldset
  • input
  • output
  • select
  • textarea

I.e. Safari doesn't inherit for img or object elements. Does this list make sense to use in the spec? It seems a tad arbitrary but I also don't see any compelling reason to deviate from the behavior as already implemented.

@annevk
Copy link
Member

annevk commented Jan 19, 2018

I wonder what @hober and @othermaciej think. It seems better to reuse an existing set if we can. Listed/submittable are both pretty close.

(@rlanday you also need to make it public you're a member of the https://github.com/google organization so @whatbot knows the IPR is in order.)

@othermaciej
Copy link

@cdumez might know about this or may know who to ask.

@annevk annevk added topic: forms interop Implementations are not interoperable with each other labels Jan 19, 2018
@domenic
Copy link
Member

domenic commented Jan 19, 2018

FWIW I could go either way. We already divide form-associated elements up into several subcategories, and adding a new one called "Autocapitalizable elements" (or something more generic?) would be reasonable; trying to specify autocapitalization for img and object doesn't make a lot of sense. Note that "listed elements" = "form-associated elements" minus img, so that gets us a bit closer at least.

On the other hand, not inventing a new category just for this sounds kind of nice as well.

@rlanday
Copy link
Contributor Author

rlanday commented Jan 22, 2018

In the interest of coming to a conclusion so I can move forward with implementing this in Chrome, I propose we pick one of the following two options:

  1. Optimize for simplicity/logicalness by leaving the spec as-is, so form inheritance only applies to and <textarea> elements, which were the original two elements to support autocapitalize, and I think are the only elements where Safari really intended to support this behavior.

  2. Optimize for consistency of implementation by describing the behavior as currently implemented by iOS Safari, which is the only current implementation of form inheritance for autocapitalize.

I would be fine with either option, but would like to be able to resolve this fairly soon.

@othermaciej
Copy link

othermaciej commented Jan 22, 2018

@rlanday Out of those two options, (2) is clearly better. Changing the behavior creates compatibility risk and the only benefit is spec simplicity. Seems like compat should clearly win, unless we can do something to evaluate the risk and demonstrate the simplicity value.

As for changing to use the set of form-associated elements: it seems like the difference between HTML's concept of "form-associated elements" and WebKit's set of "form elements" is the inclusion of img and object. While img is a form-associated element, it is not listed or submitted. I can't figure out if this has any effect other than just carrying around a form owner field.

The list of "listed form-associated elements" is a closer match, except for the inclusion of object. This might be ok, but I am not sure what the implications would be. I don't know how object interacts with forms. Is it only in the case where it represents a plugin, or does frame, image or fallback content somehow participate in form submission as well?

@domenic
Copy link
Member

domenic commented Jan 22, 2018

@othermaciej my understanding is the choice of list here, whether it's { input, textarea } or listed elements or form-associated elements, is the behavior of the IDL attribute on e.g. imgEl.autocapitalize and objectEl.autocapitalize. In particular, it's a choice of whether, when they are associated with a <form> that has its own autocapitalize="" attribute, that form's autocapitalize="" attribute is consulted as a fallback or not.

Concretely:

<form autocapitalize="words">
  <input type="text" id="textInput">
  <img id="image">
  <object id="object">
</form>

We know that textInput.autocapitalize === "words", but the question is whether image.autocapitalize and object.autocapitalize are "words" (per their form owner) or are "" (the default state).

No matter how that question is answered, I don't think anyone is planning to actually use the autocapitalization information to attempt to auto-capitalize stuff inside images or objects somehow.

So I don't think it has much practical impact, and this is mostly about getting edge-case interop.

@rlanday
Copy link
Contributor Author

rlanday commented Jan 22, 2018

I agree with @domenic. I don't think there's much actual compatibility risk here, but if we think there might be some in only including <input> and <textarea>, we should use exactly the same list Safari uses.

@domenic
Copy link
Member

domenic commented Jan 22, 2018

I guess the more I stare at it, the more I think just using form-associated elements is the "conceptually cleanest" approach: we have decided that part of the computation of the el.autocapitalize IDL attribute is to consult the form owner, and it makes sense to do that for all elements that have a form owner, i.e. all form-associated elements.

But, I can understand the desire for shortest-path-to-interop. In that case we can just update the patch with a new category ("form-associated autocapitalizable elements", I suppose).

@othermaciej
Copy link

It’s probably less risky to expand the set Safari is shipping than reduce it.

Safari’s choice of elements that inherit autocapitalize behavior is likely an artifact of implementation and not any kind of purposeful choice. But sadly mobile-centric sites sometimes come to depend on our quirks, even when it seems kind of improbable that they would. So we usually try to do some kind of testing before making changes.

@rlanday
Copy link
Contributor Author

rlanday commented Jan 23, 2018

@othermaciej, based on what you're saying, I think we should just update the spec to exactly match the current implementation. I don't think we have a compelling reason to change the behavior, other than just not having to list the elements in the spec.

If everyone is OK with this, I will update this pull request after I update the test case I'm working on to cover this behavior and verify that it passes in iOS Safari.

@rlanday
Copy link
Contributor Author

rlanday commented Jan 24, 2018

I've updated the pull request and verified that this is the correct of elements that inherit the attribute in iOS Safari.

@domenic
Copy link
Member

domenic commented Jan 24, 2018

Hmm, I'd rather define a new category, as I discussed earlier. That avoids the awkward note.

@rlanday
Copy link
Contributor Author

rlanday commented Jan 24, 2018

Ah, I apologize. Updated again.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the extra work here; I forgot how tricky adding a new category is. (One day, maybe, all that stuff will be auto-generated.)

One small issue but otherwise LGTM

source Outdated
<var>element</var>'s <span>form owner</span>.</p></li>
<li><p>If <var>element</var> is an <span
data-x="category-autocapitalize">autocapitalize-inheriting
element</span>, return the <span>own autocapitalization hint</span> of <var>element</var>'s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you still need the form owner check? E.g. document.createElement("button") has no form owner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, did not mean to remove that. Fixed

@domenic domenic merged commit b7c21b1 into whatwg:master Jan 24, 2018
@annevk
Copy link
Member

annevk commented Jan 25, 2018

Is #3373 (comment) talking about web-platform-tests?

@rlanday
Copy link
Contributor Author

rlanday commented Jan 25, 2018

Yes, I’m adding a WPT in https://chromium-review.googlesource.com/c/chromium/src/+/872090.

@rlanday rlanday deleted the autocapitalize_fix branch January 25, 2018 07:25
@annevk
Copy link
Member

annevk commented Jan 25, 2018

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: forms
Development

Successfully merging this pull request may close these issues.

4 participants