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

Add dialog light dismiss behavior #10737

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Add dialog light dismiss behavior #10737

wants to merge 17 commits into from

Conversation

mfreed7
Copy link
Contributor

@mfreed7 mfreed7 commented Oct 31, 2024

This adds a few things:

  • A closedby attribute that can be used to control the light dismiss behavior of dialogs:

    1. <dialog closedby=none> - no user-triggered closing of dialogs at all.
    2. <dialog closedby=closerequest> - user pressing ESC (or other close trigger) closes the dialog
    3. <dialog closedby=any> - user clicking outside the dialog, or pressing ESC, closes the dialog. Akin to popover=auto behavior.
    4. <dialog> - i.e. no closedby attribute - default/old behavior. Behaves like closedby=closerequest for modal dialogs, and closedby=none for modeless dialogs.
  • This PR also (per the request) adds a dialog.requestClose() which fires the cancel event, and then (if that isn't cancelled) fires the close event and closes the dialog.

  • I also consolidated the "light dismiss" activities into one algorithm, which will need to be called by Add popover light dismiss integration w3c/pointerevents#460 if that ever gets some attention.

Some interesting related links:

Closes #9373

(See WHATWG Working Mode: Changes for more details.)


/dnd.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/interaction.html ( diff )
/interactive-elements.html ( diff )
/popover.html ( diff )

@domenic
Copy link
Member

domenic commented Nov 1, 2024

This is exciting! Unfortunately I am going OOO for a week starting ~now, so won't get a chance to review too soon. Hopefully others will be able to help with some reviews, but I would like to get a chance to carefully review the close watcher integration, etc. before this lands.

Would you mind commenting on what approach you took to the mutability of the closedby attribute? We discussed that a bit in #9373, and I think we came up with a tentative proposal, but I'm not sure it was battle-tested.

@keithamus
Copy link
Contributor

Would you mind commenting on what approach you took to the mutability of the closedby attribute? We discussed that a bit in #9373, and I think we came up with a tentative proposal, but I'm not sure it was battle-tested.

Looks like CloseWatchers get a mutable enabled value, which is toggled whenever closedby is set to the applicable values - when false it skips the events and returns early. IMO this seems to nicely side-skirt the stacking issues around mutation of the attribute without adding lots of complexity in either the consuming elements and/or having some messy algorithms to insert mid-way into a stack.

Copy link
Contributor

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

This looks great to me!

source Outdated Show resolved Hide resolved
@mfreed7
Copy link
Contributor Author

mfreed7 commented Nov 1, 2024

This is exciting! Unfortunately I am going OOO for a week starting ~now, so won't get a chance to review too soon. Hopefully others will be able to help with some reviews, but I would like to get a chance to carefully review the close watcher integration, etc. before this lands.

That's totally fine - no rush. I did this one in the opposite order that I usually do: spec first, prototype after. So I'm moving on to prototype it now, which might reveal other flaws. But I thought since @lukewarlow had gotten the spec into such decent shape, that I'd clean it up and open the PR in case folks saw other flaws there that I could address in the prototype.

Would you mind commenting on what approach you took to the mutability of the closedby attribute? We discussed that a bit in #9373, and I think we came up with a tentative proposal, but I'm not sure it was battle-tested.

As @keithamus said, I just kept what @lukewarlow did in the original PR, and used a new enabled state on CloseWatcher. I haven't implemented that yet, but it seems reasonably straightforward. But let me know if you see any flaws.

@annevk annevk added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests topic: dialog The <dialog> element labels Nov 5, 2024
source Outdated Show resolved Hide resolved
source Outdated
Comment on lines 61702 to 61707
<p>The <code data-x="attr-dialog-closedby">closedby</code> attribute's <i data-x="invalid value
default">invalid value default</i> and <i data-x="missing value default">missing value
default</i> are both the <dfn data-x="attr-closedby-auto-state">Auto</dfn> state. The <span
data-x="attr-closedby-auto-state">auto</span> state matches <span
data-x="attr-closedby-closerequest-state">closerequest</span> when the element is modal;
otherwise <span data-x="attr-closedby-none-state">none</span>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

There's very inconsistent spelling of states here. They should match the capitalization and words of how they are introduced. And should typically be followed by the word state (though that's not linked).

Copy link
Member

Choose a reason for hiding this comment

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

Also, perhaps the statement of fact is better as a note?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's very inconsistent spelling of states here. They should match the capitalization and words of how they are introduced. And should typically be followed by the word state (though that's not linked).

Yep, I agree. I fixed them all up, I think. Please let me know if you see any others I missed. All are now lowercase.

Also, perhaps the statement of fact is better as a note?

Sorry - can you help me understand this? Which "statement of fact"?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 5, 2024
This CL puts the feature flag in place, adds (flag guarded) closedBy
and requestClose() methods to <dialog>, connects the pointer
events handling to a new dialog light dismiss method, and adds a
basic set of tests. None of the actual functionality is here yet,
this is just a shell. Subsequent CLs will flesh out the behavior.

See spec PR for details:
  whatwg/html#10737

Here's the chromestatus:
  https://chromestatus.com/feature/5097714453577728

Bug: 376516550
Change-Id: I3727ca21476a2a3340fd18597970395d64ef7176
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 5, 2024
This CL puts the feature flag in place, adds (flag guarded) closedBy
and requestClose() methods to <dialog>, connects the pointer
events handling to a new dialog light dismiss method, and adds a
basic set of tests. None of the actual functionality is here yet,
this is just a shell. Subsequent CLs will flesh out the behavior.

See spec PR for details:
  whatwg/html#10737

Here's the chromestatus:
  https://chromestatus.com/feature/5097714453577728

Bug: 376516550
Change-Id: I3727ca21476a2a3340fd18597970395d64ef7176
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 5, 2024
This CL puts the feature flag in place, adds (flag guarded) closedBy
and requestClose() methods to <dialog>, connects the pointer
events handling to a new dialog light dismiss method, and adds a
basic set of tests. None of the actual functionality is here yet,
this is just a shell. Subsequent CLs will flesh out the behavior.

See spec PR for details:
  whatwg/html#10737

Here's the chromestatus:
  https://chromestatus.com/feature/5097714453577728

Bug: 376516550
Change-Id: I3727ca21476a2a3340fd18597970395d64ef7176
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5985491
Commit-Queue: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1378633}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 5, 2024
This CL puts the feature flag in place, adds (flag guarded) closedBy
and requestClose() methods to <dialog>, connects the pointer
events handling to a new dialog light dismiss method, and adds a
basic set of tests. None of the actual functionality is here yet,
this is just a shell. Subsequent CLs will flesh out the behavior.

See spec PR for details:
  whatwg/html#10737

Here's the chromestatus:
  https://chromestatus.com/feature/5097714453577728

Bug: 376516550
Change-Id: I3727ca21476a2a3340fd18597970395d64ef7176
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5985491
Commit-Queue: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1378633}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 5, 2024
This implements reflection of `closedBy` including the "limited to
known values" behavior.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: Iddefd573fe19fd39f4b3aebe13390235fea969b9
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Nov 6, 2024
…ht dismiss [1/N], a=testonly

Automatic update from web-platform-tests
Implement basic structure for dialog light dismiss [1/N]

This CL puts the feature flag in place, adds (flag guarded) closedBy
and requestClose() methods to <dialog>, connects the pointer
events handling to a new dialog light dismiss method, and adds a
basic set of tests. None of the actual functionality is here yet,
this is just a shell. Subsequent CLs will flesh out the behavior.

See spec PR for details:
  whatwg/html#10737

Here's the chromestatus:
  https://chromestatus.com/feature/5097714453577728

Bug: 376516550
Change-Id: I3727ca21476a2a3340fd18597970395d64ef7176
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5985491
Commit-Queue: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1378633}

--

wpt-commits: 1d5037cc3ff4524fd5d9f1a461afd20aebbb0839
wpt-pr: 48976
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 6, 2024
This implements reflection of `closedBy` including the "limited to
known values" behavior.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: Iddefd573fe19fd39f4b3aebe13390235fea969b9
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 6, 2024
This implements reflection of `closedBy` including the "limited to
known values" behavior.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: Iddefd573fe19fd39f4b3aebe13390235fea969b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5988652
Commit-Queue: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1379228}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 6, 2024
This implements reflection of `closedBy` including the "limited to
known values" behavior.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: Iddefd573fe19fd39f4b3aebe13390235fea969b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5988652
Commit-Queue: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1379228}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 6, 2024
This implements reflection of `closedBy` including the "limited to
known values" behavior.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: Iddefd573fe19fd39f4b3aebe13390235fea969b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5988652
Commit-Queue: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1379228}
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 6, 2024
This enables `closedby=none` to *stop* the dialog from closing when
the ESC key (or other close signal) is received.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: I0bf7fc7a50acaffb89e183fe6e19f411de4da576
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5990171
Reviewed-by: David Baron <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1379279}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 7, 2024
…ht dismiss [1/N], a=testonly

Automatic update from web-platform-tests
Implement basic structure for dialog light dismiss [1/N]

This CL puts the feature flag in place, adds (flag guarded) closedBy
and requestClose() methods to <dialog>, connects the pointer
events handling to a new dialog light dismiss method, and adds a
basic set of tests. None of the actual functionality is here yet,
this is just a shell. Subsequent CLs will flesh out the behavior.

See spec PR for details:
  whatwg/html#10737

Here's the chromestatus:
  https://chromestatus.com/feature/5097714453577728

Bug: 376516550
Change-Id: I3727ca21476a2a3340fd18597970395d64ef7176
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5985491
Commit-Queue: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1378633}

--

wpt-commits: 1d5037cc3ff4524fd5d9f1a461afd20aebbb0839
wpt-pr: 48976
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 7, 2024
…alog closedBy [2/N], a=testonly

Automatic update from web-platform-tests
Add attribute reflection behavior for dialog closedBy [2/N]

This implements reflection of `closedBy` including the "limited to
known values" behavior.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: Iddefd573fe19fd39f4b3aebe13390235fea969b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5988652
Commit-Queue: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1379228}

--

wpt-commits: 28946b88d5b2e037bb2506ae4984e8725ba5560b
wpt-pr: 48989
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 8, 2024
This implements dialog.requestClose() and adds a test.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: Iaac3d89c28844d2b54ff5b1a7b68dc356d1fd172
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Nov 16, 2024
…=testonly

Automatic update from web-platform-tests
Implement dialog.requestClose() [4/N]

This implements dialog.requestClose() and adds a test.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: Iaac3d89c28844d2b54ff5b1a7b68dc356d1fd172
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5991017
Commit-Queue: Mason Freed <masonfchromium.org>
Reviewed-by: David Baron <dbaronchromium.org>
Auto-Submit: Mason Freed <masonfchromium.org>
Cr-Commit-Position: refs/heads/main{#1380450}

--

wpt-commits: 97a34c26ea3a3bfeacc0f40fc733d464c3f4d377
wpt-pr: 49050

UltraBlame original commit: 4caf91ac4ff80bb8e77480009090bbaa21eae410
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Nov 16, 2024
…=testonly

Automatic update from web-platform-tests
Implement dialog.requestClose() [4/N]

This implements dialog.requestClose() and adds a test.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: Iaac3d89c28844d2b54ff5b1a7b68dc356d1fd172
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5991017
Commit-Queue: Mason Freed <masonfchromium.org>
Reviewed-by: David Baron <dbaronchromium.org>
Auto-Submit: Mason Freed <masonfchromium.org>
Cr-Commit-Position: refs/heads/main{#1380450}

--

wpt-commits: 97a34c26ea3a3bfeacc0f40fc733d464c3f4d377
wpt-pr: 49050

UltraBlame original commit: 4caf91ac4ff80bb8e77480009090bbaa21eae410
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Nov 16, 2024
…=testonly

Automatic update from web-platform-tests
Implement dialog.requestClose() [4/N]

This implements dialog.requestClose() and adds a test.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: Iaac3d89c28844d2b54ff5b1a7b68dc356d1fd172
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5991017
Commit-Queue: Mason Freed <masonfchromium.org>
Reviewed-by: David Baron <dbaronchromium.org>
Auto-Submit: Mason Freed <masonfchromium.org>
Cr-Commit-Position: refs/heads/main{#1380450}

--

wpt-commits: 97a34c26ea3a3bfeacc0f40fc733d464c3f4d377
wpt-pr: 49050

UltraBlame original commit: 4caf91ac4ff80bb8e77480009090bbaa21eae410
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Nov 16, 2024
…alog closedBy [2/N], a=testonly

Automatic update from web-platform-tests
Add attribute reflection behavior for dialog closedBy [2/N]

This implements reflection of `closedBy` including the "limited to
known values" behavior.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: Iddefd573fe19fd39f4b3aebe13390235fea969b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5988652
Commit-Queue: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1379228}

--

wpt-commits: 28946b88d5b2e037bb2506ae4984e8725ba5560b
wpt-pr: 48989
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Nov 16, 2024
…=testonly

Automatic update from web-platform-tests
Implement dialog.requestClose() [4/N]

This implements dialog.requestClose() and adds a test.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: Iaac3d89c28844d2b54ff5b1a7b68dc356d1fd172
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5991017
Commit-Queue: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1380450}

--

wpt-commits: 97a34c26ea3a3bfeacc0f40fc733d464c3f4d377
wpt-pr: 49050
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 17, 2024
…ctual click outside part) 5/5, a=testonly

Automatic update from web-platform-tests
Add dialog light dismiss behavior (the actual click outside part) 5/5

This adds to the prior CLs to actually allow "click outside" to
function as a light dismiss trigger.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: I62158e092de9acac182777f2ad9864e818128907
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6013845
Commit-Queue: David Baron <[email protected]>
Reviewed-by: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1383792}

--

wpt-commits: 5420c40fa454adde8aa1e810ddb97ed79fd15c0e
wpt-pr: 49201
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Nov 18, 2024
…ctual click outside part) 5/5, a=testonly

Automatic update from web-platform-tests
Add dialog light dismiss behavior (the actual click outside part) 5/5

This adds to the prior CLs to actually allow "click outside" to
function as a light dismiss trigger.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: I62158e092de9acac182777f2ad9864e818128907
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6013845
Commit-Queue: David Baron <[email protected]>
Reviewed-by: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1383792}

--

wpt-commits: 5420c40fa454adde8aa1e810ddb97ed79fd15c0e
wpt-pr: 49201
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 18, 2024
1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 18, 2024
1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1384345}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 18, 2024
1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1384345}
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Nov 20, 2024
…ctual click outside part) 5/5, a=testonly

Automatic update from web-platform-tests
Add dialog light dismiss behavior (the actual click outside part) 5/5

This adds to the prior CLs to actually allow "click outside" to
function as a light dismiss trigger.

See spec PR for details:
  whatwg/html#10737

Bug: 376516550
Change-Id: I62158e092de9acac182777f2ad9864e818128907
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6013845
Commit-Queue: David Baron <[email protected]>
Reviewed-by: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1383792}

--

wpt-commits: 5420c40fa454adde8aa1e810ddb97ed79fd15c0e
wpt-pr: 49201
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 20, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Sorry for the big delay! Overall this looks good, with the only really substantial suggestion being to move to making enabled an algorithm to avoid the attribute changed steps.

Do you have a link for the tests? I've found that writing comprehensive-feeling tests for close watcher stuff is hard (as you want to test at least two, sometimes more, nested). And then you have all the permutations of attribute values, what if someone modifies the attribute while it's closed, etc. In my experience, without tests it's quite difficult to make sure the behavior is reasonable.

For example, it'd be worth testing (maybe interactively, instead of in WPTs, at least at first) that you get reasonable user behavior in situations like one closedby=none dialog sandwiched in the middle of two closedby=any dialogs.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated
<li><p>If <span>this</span> is not <span>connected</span>, then throw an
<span>"<code>InvalidStateError</code>"</span> <code>DOMException</code>.</p></li>

<li><p>If <span>this</span>'s <span>node document</span> is not <span>fully active</span>, then
throw an <span>"<code>InvalidStateError</code>"</span> <code>DOMException</code>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

What is the reasoning for this reordering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I wasn't sure about that, thanks for asking. I don't think the ordering of these two steps matters, and it just seemed more logical to me to check first whether it was connected, and then check for a fully active owner document. I can put it back - no big deal.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Contributor Author

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

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

Thanks for the very thorough review! I think I got everything, but let me know what I missed or did incorrectly.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated
<li><p>If <span>this</span> is not <span>connected</span>, then throw an
<span>"<code>InvalidStateError</code>"</span> <code>DOMException</code>.</p></li>

<li><p>If <span>this</span>'s <span>node document</span> is not <span>fully active</span>, then
throw an <span>"<code>InvalidStateError</code>"</span> <code>DOMException</code>.</p></li>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I wasn't sure about that, thanks for asking. I don't think the ordering of these two steps matters, and it just seemed more logical to me to check first whether it was connected, and then check for a fully active owner document. I can put it back - no big deal.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@mfreed7
Copy link
Contributor Author

mfreed7 commented Nov 21, 2024

Sorry for the big delay! Overall this looks good, with the only really substantial suggestion being to move to making enabled an algorithm to avoid the attribute changed steps.

No problem, and thanks again!

Do you have a link for the tests? I've found that writing comprehensive-feeling tests for close watcher stuff is hard (as you want to test at least two, sometimes more, nested). And then you have all the permutations of attribute values, what if someone modifies the attribute while it's closed, etc. In my experience, without tests it's quite difficult to make sure the behavior is reasonable.
For example, it'd be worth testing (maybe interactively, instead of in WPTs, at least at first) that you get reasonable user behavior in situations like one closedby=none dialog sandwiched in the middle of two closedby=any dialogs.

So I've written tests for several levels of "nesting", including popover/dialog/popover/dialog with mixed modal/modeless dialogs (that's the -complex test). I also added just popover/dialog with different values of closedby (that's the -simple test). But I didn't include closedby permutations in the complex test. I can do that, if you think it'd be helpful.

i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Nov 21, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Nov 22, 2024
…=testonly

Automatic update from web-platform-tests
Fix dialog.requestClose() in two ways

1. The prior implementation ignored the argument to requestClose.
   With this CL, requestClose(string) stores away string and then
   returns it from the CloseWatcher close event.
2. If the dialog's closedBy state is None (either explicitly or
   through the "Auto" state), requestClose throws an exception.

I also added a more significant set of tests for requestClose() in
all states.

This goes along with recent discussion about throwing exceptions if
requestClose() is called when the dialog isn't in the right state,
relative to closedBy:

  whatwg/html#10164 (comment)

The spec PR has been updated accordingly:

  whatwg/html#10737

Bug: 376516550
Change-Id: I023845844e6afb4da9a71637d517ac78d2861329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026242
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1384345}

--

wpt-commits: 6f4f09f92913154e70e9214636ee0e60c65f5d05
wpt-pr: 49223
@domenic
Copy link
Member

domenic commented Nov 22, 2024

Did you see my comments on #9373 (comment)? I guess we should figure out the desired behavior for nested dialogs over there...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests topic: dialog The <dialog> element
Development

Successfully merging this pull request may close these issues.

Add light dismiss functionality to <dialog>
4 participants