-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Conversation
7684efc
to
5112842
Compare
st.setStyle(this.element, 'opacity', ''); | ||
}); | ||
|
||
this.handleAutofocus_(); |
There was a problem hiding this comment.
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.
@jridgewell would it be okay to make an exception and allow 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) |
In this case, sure. Call |
More iOS isolated examples here:
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: |
}); | ||
Services.vsyncFor(this.win).mutate(() => { |
There was a problem hiding this comment.
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', '');
});
});
There was a problem hiding this comment.
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.
@jridgewell PTAL? =) |
|
||
this.handleAutofocus_(); | ||
|
||
// TODO (jridgewell): expose an API accomodating this per PR #14676 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Add lightbox autofocus ios test * Try adding autofocus in lightbox activate * Add mutateElement
Addresses #13332.
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 setdisplay
to''
.iOS Safari working demo: https://amp-cathyxz.herokuapp.com/test/manual/amp-lightbox-focus.amp.html