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

Add compat data for input-password #2235

Merged
merged 5 commits into from
Jun 28, 2018
Merged

Add compat data for input-password #2235

merged 5 commits into from
Jun 28, 2018

Conversation

maboa
Copy link
Contributor

@maboa maboa commented Jun 8, 2018

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/password

@Elchi3 @wbamberg please let me know if I've dealt with the attributes properly, and further whether we should 'retrofit' to other input types. Thanks!

@Elchi3 Elchi3 added the data:html Compat data for HTML elements. https://developer.mozilla.org/docs/Web/HTML label Jun 13, 2018
ddbeck
ddbeck previously requested changes Jun 22, 2018
Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Looks great, @maboa. I don't have an opinion on the subfeatures, apart from my comments—so you might want to wait for the reviews you actually requested before acting on my comments. Thank you!

},
"firefox": {
"version_added": "1",
"notes": "Firefox will, unlike other browsers, by default, <a href='http://stackoverflow.com/q/5985839/432681'>persist the dynamic disabled state and (if applicable) dynamic checkedness</a> of an <code><input></code> across page loads. Setting the value of the <code><a href='https://developer.mozilla.org/docs/Web/HTML/Element/input#attr-autocomplete'>autocomplete</a></code> attribute to <code>off</code> disables this feature; this works even when the <code><a href='https://developer.mozilla.org/docs/Web/HTML/Element/input#attr-autocomplete'>autocomplete</a></code> attribute to <code>off</code> attribute would normally not apply to the <code><input></code> by virtue of its <code><a href='https://developer.mozilla.org/docs/Web/HTML/Element/input#attr-type'>type</a></code>. See <a href='https://bugzil.la/654072'>bug 654072</a>."
Copy link
Collaborator

Choose a reason for hiding this comment

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

<input> needs to be escaped here (i.e., &lt;input&gt;)

"support": {
"chrome": {
"version_added": "5",
"notes": "In 6.0 it only worked with the HTML5 document type, validation support in 7.0 was disabled and re-enabled in 10.0."
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to me like this would be easier to understand as an array of entries:

  • Version added 10
  • Version added 5, version removed 7, with a note about Chrome 6 HTML5 doctype restriction

"support": {
"chrome": {
"version_added": "5",
"notes": "Chrome 10 added support for <code><a href='https://developer.mozilla.org/docs/Web/HTML/Element/select'><select></a></code> element."
Copy link
Collaborator

Choose a reason for hiding this comment

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

<select> needs to be escaped here (i.e., &lt;select&gt;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is relevant to password inputs, is it?

"support": {
"chrome": {
"version_added": null,
"notes": "implementing something similar"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this note should be dropped, since it's not particularly informative

},
"chrome_android": {
"version_added": null,
"notes": "implementing something similar"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this note should be dropped

},
"status": {
"experimental": false,
"standard_track": true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't find any standard to back this up, so I think it ought to be false

},
"status": {
"experimental": false,
"standard_track": true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't find any standard to back this up, so I think it ought to be false

@wbamberg
Copy link
Contributor

This is an odd one. The original table has couple of generic problems:

  1. it includes things that aren't relevant to password inputs (e.g. the note Missing for type="checkbox" and type="radio")

  2. it includes things that are relevant, but are global attributes not specific to password (most of the rows in fact)

For (1) we should just omit them. For (2) I think it comes down to policy: should we include global attributes in each input type? For the other types we haven't: we've only included subfeatures where there is something interesting to say.

On the one hand, completeness argues that we should include them: after all it's relevant (and possibly even true!) that, say, Chrome added support for the form attribute for password inputs in version 9.

On the other hand, it's hard to imagine this is useful information to anyone at this point.

I would vote for omitting these, but would welcome other views too :). @Elchi3 ?

Copy link
Contributor

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

A few comments inline. We should remove the stuff that's not relevant to password inputs, and possibly remove the global attribute data.

},
"firefox": {
"version_added": "1",
"notes": "Firefox will, unlike other browsers, by default, <a href='http://stackoverflow.com/q/5985839/432681'>persist the dynamic disabled state and (if applicable) dynamic checkedness</a> of an <code><input></code> across page loads. Setting the value of the <code><a href='https://developer.mozilla.org/docs/Web/HTML/Element/input#attr-autocomplete'>autocomplete</a></code> attribute to <code>off</code> disables this feature; this works even when the <code><a href='https://developer.mozilla.org/docs/Web/HTML/Element/input#attr-autocomplete'>autocomplete</a></code> attribute to <code>off</code> attribute would normally not apply to the <code><input></code> by virtue of its <code><a href='https://developer.mozilla.org/docs/Web/HTML/Element/input#attr-type'>type</a></code>. See <a href='https://bugzil.la/654072'>bug 654072</a>."
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't at all specific to password inputs. It should be attached to the data for the generic input element.

},
"ie": {
"version_added": "6",
"notes": " Missing for <code>type='checkbox'</code> and <code>type='radio'</code>."
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't relevant to password inputs.

"support": {
"chrome": {
"version_added": "5",
"notes": "Chrome 10 added support for <code><a href='https://developer.mozilla.org/docs/Web/HTML/Element/select'><select></a></code> element."
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is relevant to password inputs, is it?

}
}
},
"crossed-lock": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is appropriate to include in the data for the password input. It is sort of relevant but only indirectly. It seems more like a generic browser UI decision rather than some aspect of the input itself.

},
"insecure-login": {
"__compat": {
"description": "Message displayed next to password field to indicate insecure login page, plus autofill disabled",
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like putting documentation in the description field. This does seem more relevant to password than "crossed-lock", but perhaps it would be better for the description to be kept short and there to be text in the main doc fleshing it out? Like "Special handling of password inputs in insecure login pages"?

Not much shorter :) but I think it is better for the MDN page body to include the description of exactly what the browser does in this situation.

@Elchi3
Copy link
Member

Elchi3 commented Jun 25, 2018

I agree to omitting global attributes or things not specific to input-password for now.
If these things have specific compat data just in the case of input-password, we can add them later on and see them as "overrides" to the generic input data.

@Elchi3
Copy link
Member

Elchi3 commented Jun 27, 2018

@ddbeck can you fix this one up? I'm happy to re-review afterwards.

@ddbeck
Copy link
Collaborator

ddbeck commented Jun 28, 2018

@Elchi3 Yes, will do! I'll tag you when it's done

@ddbeck ddbeck dismissed their stale review June 28, 2018 14:01

I'm too close to it now

@ddbeck
Copy link
Collaborator

ddbeck commented Jun 28, 2018

@Elchi3 OK, I think this is ready for your your review now. Thank you!

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thank you Mark and Daniel! 👍

@Elchi3 Elchi3 dismissed wbamberg’s stale review June 28, 2018 14:47

Review comments addressed. We've removed non-password related things altogether

@Elchi3 Elchi3 merged commit a758091 into mdn:master Jun 28, 2018
germain-gg pushed a commit to germain-gg/browser-compat-data that referenced this pull request Jun 29, 2018
* master: (175 commits)
  Add Server-Timing header (mdn#2148)
  40th alpha version
  Add compat data for input-password (mdn#2235)
  Add addTransceiver() method for RTCPeerConnection. (mdn#2311)
  Update Payments API for Chrome. (mdn#2378)
  Add compat data for MutationObserver (mdn#2094)
  Fix compat data for "let" in Chrome (mdn#1632)
  Safari has more formdata support now (mdn#2376)
  Add compat data for Window sub features from A to F (mdn#2109)
  Add guidelines around worker support and constructors for API subfeatures (mdn#2373)
  Update place-content.json (mdn#2240)
  Add compat data for HTMLDetailsElement (mdn#2315)
  Add compat data for input-month (mdn#2204)
  Add compat data to input-week (mdn#2230)
  Add compat data for input-radio (mdn#2208)
  Add compat data for input-tel (mdn#2226)
  Add compat data for input-image (mdn#2207)
  Add compat data for input-number (mdn#2206)
  Add compat data for input-hidden (mdn#2203)
  Add compat data for input-file (mdn#2202)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:html Compat data for HTML elements. https://developer.mozilla.org/docs/Web/HTML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants