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

Update autocapitalize props #30734

Merged
merged 17 commits into from
Dec 4, 2023
Merged

Update autocapitalize props #30734

merged 17 commits into from
Dec 4, 2023

Conversation

ryo-manba
Copy link
Contributor

Description

According to HTML Standard, it also accepts the following off and on keywords.
https://html.spec.whatwg.org/multipage/interaction.html#autocapitalization

This is a follow-up to the following PR.

@ryo-manba ryo-manba requested a review from a team as a code owner December 2, 2023 05:50
@ryo-manba ryo-manba requested review from chrisdavidmills and removed request for a team December 2, 2023 05:50
@github-actions github-actions bot added the Content:HTML Hypertext Markup Language docs label Dec 2, 2023
Copy link
Contributor

github-actions bot commented Dec 2, 2023

Preview URLs

(comment last updated: 2023-12-04 12:00:31)

- `none`: No automatic capitalization.
- `sentences` (default): Capitalize the first letter of each sentence.
- `none` or `off`: No automatic capitalization.
- `sentences` or `on` (default): Capitalize the first letter of each sentence.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
- `sentences` or `on` (default): Capitalize the first letter of each sentence.
- `sentences` or `on`: Capitalize the first letter of each sentence.

default
The user agent and input method should use make their own determination of whether or not to enable autocapitalization.

ref: https://html.spec.whatwg.org/multipage/interaction.html#autocapitalization

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Hi there @ryo-manba , and thank you for your contribution!

I think your change is correct, but there are a few other problems with the autocapitalize documentation as I see it, which you could also consider adding/updating. I wrote up a quick test case at https://autocapitalize-test.glitch.me/. From my research, and from testing across different desktop and mobile browsers, I observed the following:

  • As far as I can tell, it doesn't work on desktop browsers.
  • It does work across mobile browsers: I tested Firefox Android, Chrome Android, and Safari iOS. All of the values work as expected.
  • The mobile browsers do differ in terms of what their default behaviors is when no autocapitalize is specified. On Chrome Android and Safari iOS the default behavior is on/sentences. On Firefox Android the default is off/none. This is worth documenting.
  • It is in a spec and supported across multiple browsers, so you can get rid of the {{non-standard_inline}} macro call and the mention of non-standard in the text. Also maybe update "used by iOS Safari" to "used by mobile browsers"?
  • Also, whatever changes you make here should also be made at https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#autocapitalize (also move it out fo the non-standard list to the standard list?) and https://developer.mozilla.org/en-US/docs/Web/HTML/Element/textarea (it is covered here at all, so add it?)
  • On the <input> page, you should document this bit from the spec: "The autocapitalize attribute never causes autocapitalization to be enabled for input elements whose type attribute is in one of the URL, Email, or Password states. (This behavior is included in the used autocapitalization hint algorithm below.)"

What do you think? Are you OK to work on these changes?

The browser compat data also needs updating to mark desktop browsers as not supporting it, and add it to element where it isn't included? This can be found at https://github.com/mdn/browser-compat-data. I appreciate that this is really another task, not part of this same one.

@ryo-manba
Copy link
Contributor Author

@chrisdavidmills
Thank you for the feedback! I agree with your suggestions and think they are very appropriate. I am definitely willing to work on these changes.

@ryo-manba
Copy link
Contributor Author

@chrisdavidmills
I have tried to fix it and would appreciate a review!

@teoli2003
Copy link
Contributor

teoli2003 commented Dec 4, 2023

I think the easiest way to solve this is to get this PR locally (using gh pr checkout 30724 run npx prettier -w **/*.md in html/element, and then commit the changes (git commit -m "Run prettier") and push back using git pull.

More efficient than trial and error.

The Github CLI is very useful for such cases.

- : Automatically capitalize every character.

> **Note:** When `autocapitalize` is set on a `<form>`, it controls the autocapitalization behavior of all contained {{htmlelement("input")}} (except `url`, `email`, and `password` types) and {{htmlelement("textarea")}} elements, overriding any `autocapitalize` values set on contained elements.

Copy link
Contributor

Choose a reason for hiding this comment

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

[mdn-linter] reported by reviewdog 🐶

Suggested change

@chrisdavidmills
Copy link
Contributor

I think the easiest way to solve this is to get this PR locally (using gh pr checkout 30724 run npx prettier -w **/*.md in html/element, and then commit the changes (git commit -m "Run prettier") and push back using git pull.

@teoli2003 Yup, I'm going to tackle the linting errors now, by pulling it locally, don't worry. I was making some changes to the text anyway.

@chrisdavidmills
Copy link
Contributor

I think the easiest way to solve this is to get this PR locally (using gh pr checkout 30724 run npx prettier -w **/*.md in html/element, and then commit the changes (git commit -m "Run prettier") and push back using git pull.

@teoli2003 Sorry, can you help me with this? I'm really struggling to get it to work. I don't use the github cli, so your commands won't work for me. And I can't figure out what the magical incantation is to get this to work in raw git commands.

@teoli2003
Copy link
Contributor

teoli2003 commented Dec 4, 2023

Replace upstream by origin in the following if you kept the default upstream remote name, but this should work:

git fetch upstream pull/30734/head:patch-1
git checkout patch-1

@chrisdavidmills
Copy link
Contributor

chrisdavidmills commented Dec 4, 2023

git fetch upstream pull/30734/head:patch-1
git checkout patch-1

@teoli2003 great, so this worked! Thanks. (I'm pretty sure this is what I was doing before, not sure why it wasn't working, but hey)

So now I've run prettier, added, committed, and I'm trying to push the changes back to this PR. How do I do that successfully?

  • git push won't work because it says the remote isn't set up for the local branch
  • git push upstream patch-1 doesn't work; it seems to try to create a new branch rather than pushing to the existing one
  • I can't push to Ryo's branch directly because I don't have permission to do that.

I really hate git sometimes.

@teoli2003
Copy link
Contributor

I did a test (using gh), and git push there pushes it to ryo-manba:patch-1, so I think you should push to ryo-manba (but I have not found the git command).
Capture d’écran 2023-12-04 à 12 54 38

That's why I'm using the GitHub CLI for this, it set up everything correctly and git push work.

(I've added a test that I will try to remove with a git command)

@teoli2003
Copy link
Contributor

teoli2003 commented Dec 4, 2023

So, it worked when I did:

git fetch upstream pull/30734/head:patch-1
git checkout patch-1

followed by your changes and then:

git remote add ryo-manba https://github.com/ryo-manba/content.git 
git push ryo-manba

Copy link
Contributor

@chrisdavidmills chrisdavidmills 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 your great work on the content @ryo-manba! It was very close to perfect; I just decided to make some fixes to the grammar before merging it.

Also a huge thanks to @teoli2003 for helping me with my git issues. I have taken note of your method, and I will look into the GitHub CLI too.

@chrisdavidmills chrisdavidmills merged commit 60fde74 into mdn:main Dec 4, 2023
7 checks passed
@ryo-manba ryo-manba deleted the patch-1 branch December 4, 2023 13:28
estelle pushed a commit to estelle/content that referenced this pull request Dec 5, 2023
* Update autocapitalize props

* Apply suggestions from code review

* Update autocapitalize attribute documentation for mobile browser compatibility

* Update textarea and input documentation for autocapitalize attribute

* fix attribute types

* fix: link

* Tweaks to <input> text

* Tweaks to <textarea>

* Couple more tweaks

* Small tweak

* Tweak <form>

* Fix note boxes

* Test

* Remove test

---------

Co-authored-by: Chris Mills <[email protected]>
Co-authored-by: Jean-Yves Perrier <[email protected]>
@vlakoff
Copy link
Contributor

vlakoff commented Jan 16, 2024

Just a nitpick, but now that this PR has been merged, could you please delete the mdn/patch-1 branch, that was created with the Git manipulations above?

Because currently, when creating a PR using the GitHub website, contributors end up with an autogenerated branch named patch-2, instead of the usual patch-1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:HTML Hypertext Markup Language docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants