-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 the ability to not perform dialog focusing steps #2197
Conversation
This closes #1929 by providing an opt-out for the use cases discussed there, via show({ skipFocusing: true }) and the same for showModal(). This was originally discussed in https://www.w3.org/Bugs/Public/show_bug.cgi?id=24718.
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.
looks good and matches my proposed implementation:
https://codereview.chromium.org/2560553002/
My main concern is naming, as @smaug---- mentioned in #1929 (comment) |
@nt1m fixed. LGTY? |
|
<li><p>If the <code data-x="">skipFocusing</code> member of <var>options</var> is not present, or | ||
if it is present but its value is false, run the <span>dialog focusing steps</span> for this | ||
<code>dialog</code> element.</p></li> | ||
<li><p>If the <code data-x="">skipInitialFocusing</code> member of <var>options</var> is not |
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.
This should be skipInitialFocus right ?
<li><p>If the <code data-x="">skipFocusing</code> member of <var>options</var> is not present, or | ||
if it is present but its value is false, run the <span>dialog focusing steps</span> for | ||
<var>subject</var>.</p></li> | ||
<li><p>If the <code data-x="">skipInitialFocusing</code> member of <var>options</var> is not |
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.
Same here.
@domenic Looks good, with the two typos fixed. |
OK cool, so this just needs web-platform-tests before we can merge. @danbeam since you're working on a patch already, would you be up for contributing those? |
@domenic i suppose I can update web-platform-tests they're a bit broken currently: I've sent a PR to fix up a little before I add a case for skipInitialFocus: |
Marking "do not merge" and "needs implementer interest" as per the latest discusisons in #1929 it appears Blink no longer supports this feature. |
This closes #1929 by providing an opt-out for the use cases discussed
there, via show({ skipFocusing: true }) and the same for showModal().
This was originally discussed in
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24718.
Looking for sign-off on this from Blink (@danbeam) and Gecko (@nt1m).
💥 Error: Wattsi server error 💥
PR Preview failed to build. (Last tried on Jan 15, 2021, 7:57 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.