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 inputs with the autofocus attribute #11908

Merged
merged 2 commits into from Nov 2, 2017

Conversation

cvializ
Copy link
Contributor

@cvializ cvializ commented Nov 2, 2017

Fixes #11892

It was a quick hit so I went ahead and made a PR

@@ -251,6 +257,13 @@ export class AmpForm {

/** @private */
installEventHandlers_() {
this.viewer_.whenFirstVisible().then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably want autofocus to take effect everytime document becomes visible (think viewer swipes). whenNextVisible does that (and covers first visibility as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah you're right; I thought it was just once after the next time it became visible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the document state reset between viewer swipes? i.e. would the user expect the field to autofocus again if they swipe to the next document and then swipe back?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh naming! :) if you can think of a shorter version of ifAlreadyVisibleOrWhenNextVisible , we can rename. it is a fairly new feature and not used in many places yet.

Copy link
Contributor Author

@cvializ cvializ Nov 2, 2017

Choose a reason for hiding this comment

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

haha yeah one of the hardest problems

@cvializ cvializ merged commit 491fe7d into ampproject:master Nov 2, 2017
neko-fire pushed a commit to 3qnexx/amphtml that referenced this pull request Nov 17, 2017
* Fix inputs with the autofocus attribute

* Use whenNextVisible instead
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
* Fix inputs with the autofocus attribute

* Use whenNextVisible instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants