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

Close watchers: history-action user activation is consumed even for the first close watcher #10046

Closed
domenic opened this issue Jan 11, 2024 · 0 comments · Fixed by #10168
Closed

Comments

@domenic
Copy link
Member

domenic commented Jan 11, 2024

What is the issue with the HTML Standard?

Consider a case like

button.onclick = () => {
  const cw = new CloseWatcher();
  cw.oncancel = e => e.preventDefault();
  cw.requestClose();
};

or

button.onclick = () => {
  dialog.showModal();
  dialog.oncancel = e => e.preventDefault();
  // now the user issues a close request
};

The intent of the close request and close watcher infrastructure introduced in #9462 was that the cancel event would fire for these close watchers, consuming the history user-action user activation that was created on clicking the button. Basically, close watchers can be used to extend the required number of back gestures/presses by 1; see this section of the explainer.

However, the spec currently consumes the history user-activation created by clicking the button when establishing the CloseWatcher: https://html.spec.whatwg.org/#establish-a-close-watcher .

We should swap steps 3 and 4 in this algorithm, so that we only consume history user-activation if we've already used up the "one free close watcher" slot.

This will help ameliorate issues like https://bugs.chromium.org/p/chromium/issues/detail?id=1512224, where at least two web developers have managed to fall into the 0.000015% of page views cases that we determined was OK to break when merging #9462. (See also my summary comment on that bug.)

/cc @josepharhar @lukewarlow

@domenic domenic added normative change topic: dialog The <dialog> element. labels Jan 11, 2024
m-akinc added a commit to ni/nimble that referenced this issue Feb 22, 2024
…1848)

## 🤨 Rationale

There is a change to the HTML spec introducing the `CloseWatcher` API,
which affects the way the native `dialog` element behaves. Recently,
Chrome had released their implementation, and it broke our drawer and
dialog (see issue #1713, as well as
resulting [Chromium
issue](https://bugs.chromium.org/p/chromium/issues/detail?id=1512224)).
Though they rolled back their release of that feature (by moving it back
to experimental), it will eventually be implemented in Chrome and other
browsers in some form. We want to at least guard against leaving the
drawer/dialog in a bad state.

Also fixes #1445

## 👩‍💻 Implementation

The issue is that the `cancel` event is intentionally not fired in a
couple cases where the `dialog` is dismissed with ESC.
- One such case was when the there is no user interaction (e.g.
clicking, scrolling) in the `dialog` before pressing ESC. Apparently,
this is [viewed as a mistake and will likely be
changed](whatwg/html#10046).
- Another case that is [more fundamental to the `CloseWatcher`
API](https://github.com/WICG/close-watcher#abuse-analysis) is when there
are two ESC presses without an intervening user interaction.

I have added a `close` event handler in both the drawer and dialog that
resolves and clears the promise, if there is one still set. This ensures
we are not left in a bad state where the drawer/dialog can't be
reopened. Note there are still problems:
- the drawer closes without animation (because we rely on the `cancel`
event to trigger the animation)
- the drawer/dialog can be closed with ESC even if `prevent-dismiss` is
set

Depending on the ultimate `CloseWatcher` specification, we may decide to
deprecate the `prevent-dismiss` option.

OTHER CHANGES:
- Gave the dialog proper focus styling, rather than the default.

## 🧪 Testing

I added tests for the specific case now being handled, though these
won't currently be running against any browser versions that would
exhibit the problem behavior.

I did manual testing using a [version of
Chromium](https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Win_x64/1241130/)
with the `CloseWatcher` API enabled.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
domenic added a commit that referenced this issue Feb 28, 2024
As noted in #10046, the close watcher anti-abuse measures were overly strict, and would prevent firing the cancel event even when the close watcher was created with user interaction.

Additionally, the "is grouped with previous" infrastructure used to track close watcher groups was buggy, since close watchers could be destroyed and this would cause groups to spuriously collapse.

To fix both of these problems, we re-do the close watcher anti-abuse protections. We now track the groups as a list-of-lists, and explicitly track how many groups should be allowed at a given time. We can then compare them when making decisions about whether to group a new close watcher, or whether to fire a cancel event.

Note that the initial fix proposed in #10046 (and implemented in #10048) is incorrect, as it allows indefinite trapping of the user.

Closes #10046.
domenic added a commit that referenced this issue Mar 14, 2024
As noted in #10046, the close watcher anti-abuse measures were overly strict, and would prevent firing the cancel event even when the close watcher was created with user interaction.

Additionally, the "is grouped with previous" infrastructure used to track close watcher groups was buggy, since close watchers could be destroyed and this would cause groups to spuriously collapse.

To fix both of these problems, we re-do the close watcher anti-abuse protections. We now track the groups as a list-of-lists, and explicitly track how many groups should be allowed at a given time. We can then compare them when making decisions about whether to group a new close watcher, or whether to fire a cancel event.

Note that the initial fix proposed in #10046 (and implemented in #10048) is incorrect, as it allows indefinite trapping of the user.

Closes #10046.
rubberyuzu pushed a commit to rubberyuzu/html that referenced this issue Mar 21, 2024
As noted in whatwg#10046, the close watcher anti-abuse measures were overly strict, and would prevent firing the cancel event even when the close watcher was created with user interaction.

Additionally, the "is grouped with previous" infrastructure used to track close watcher groups was buggy, since close watchers could be destroyed and this would cause groups to spuriously collapse.

To fix both of these problems, we re-do the close watcher anti-abuse protections. We now track the groups as a list-of-lists, and explicitly track how many groups should be allowed at a given time. We can then compare them when making decisions about whether to group a new close watcher, or whether to fire a cancel event.

Note that the initial fix proposed in whatwg#10046 (and implemented in whatwg#10048) is incorrect, as it allows indefinite trapping of the user.

Closes whatwg#10046.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
1 participant