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

Allow autofocus to work inside <amp-lightbox> on iOS #14676

Merged
merged 3 commits into from
Apr 18, 2018

Conversation

cathyxz
Copy link
Contributor

@cathyxz cathyxz commented Apr 17, 2018

Addresses #13332.

  1. iOS requires focus to be inside a click handler in order to trigger focus and pop up the keyboard.
  2. The focus cannot be in an async callback.
  3. The lightbox needs to be visible (aka not display none) in order for input to be focusable.

Hence, proposing this fix for lack of a better solution (suggestions more than welcome). The downside is that it removes the mutateElement block surrounding the set display to ''.

iOS Safari working demo: https://amp-cathyxz.herokuapp.com/test/manual/amp-lightbox-focus.amp.html

st.setStyle(this.element, 'opacity', '');
});

this.handleAutofocus_();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is basically that we need the tryFocus() to be applied to something visible, BUT due to iOS restrictions on focus needing to be synchronously called inside a click handler, we can't have it inside or waiting for a mutateElement. Would love to know if there's a better solution for this.

@alanorozco alanorozco removed their request for review April 17, 2018 22:13
@aghassemi
Copy link
Contributor

@jridgewell would it be okay to make an exception and allow amp-lightbox do setStyle (removing display:none) not in a mutate block? (and still call mutateElement later so runtime picks up the changes).

I personally can't think of a workaround for this iOS restriction (e.g. focus has to be a sync call following user-interaction and target of focus should be visible)

@jridgewell
Copy link
Contributor

would it be okay to make an exception and allow amp-lightbox do setStyle (removing display:none) not in a mutate block? (and still call mutateElement later so runtime picks up the changes).

In this case, sure. Call #mutateElement with an empty function, and add a TODO for me to expose a better API.

@cathyxz
Copy link
Contributor Author

cathyxz commented Apr 17, 2018

More iOS isolated examples here:

  1. Vanilla onClick focus works: https://codepen.io/cathyxz/pen/XELrjO
  2. Promise with dom manipulation then focus works: https://codepen.io/cathyxz/pen/bMbypd
  3. Promise waiting for setTimeout then focus does NOT work: https://codepen.io/cathyxz/pen/odvRpm
  4. Promise with rAF then focus does NOT work: https://codepen.io/cathyxz/pen/QrLRvK
  5. setTimeout focus does NOT work: https://codepen.io/cathyxz/pen/debEog

I couldn't find anything that exactly specifies what iOS does when programmatically triggering focus (no webkit bugs match this exact use case either), but I'll dig a bit more.

Some resources on the subject:
https://medium.com/@brunn/autofocus-in-ios-safari-458215514a5f
vuejs/vue#7546
https://stackoverflow.com/questions/12204571/mobile-safari-javascript-focus-method-on-inputfield-only-works-with-click

});
Services.vsyncFor(this.win).mutate(() => {
Copy link
Contributor Author

@cathyxz cathyxz Apr 17, 2018

Choose a reason for hiding this comment

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

@jridgewell can we put this in a mutateElement block? I don't actually know why this still uses a vsync. I'm assuming that the original intention here was to queue this into the next rAF mutation.

So instead of calling mutateElement with an empty block, could we do this instead of the current vsync?

this.mutateElement(() => {
    st.setStyle(this.element, 'opacity', '');
});

Or do we need to nest this like so?

this.mutateElement(() => {
    this.mutateElement(() => {
        st.setStyle(this.element, 'opacity', '');
    });
});

Copy link
Contributor

Choose a reason for hiding this comment

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

We just need a #mutateElement call so that the measurement cache is busted. Anything will work, and if there's nothing that needs to be mutated, just use an empty function.

So, in this case, just a single mutate with a setStyle will do.

@cathyxz
Copy link
Contributor Author

cathyxz commented Apr 17, 2018

@jridgewell PTAL? =)


this.handleAutofocus_();

// TODO (jridgewell): expose an API accomodating this per PR #14676
Copy link
Contributor

Choose a reason for hiding this comment

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

@cathyxz cathyxz merged commit 0b96423 into ampproject:master Apr 18, 2018
@cathyxz cathyxz deleted the bugfix/ios-focus branch April 18, 2018 00:40
glevitzky pushed a commit that referenced this pull request Apr 27, 2018
* Add lightbox autofocus ios test

* Try adding autofocus in lightbox activate

* Add mutateElement
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

4 participants