Allow hide animations to be started from pop up hide event [chromium/src : main]

Mason Freed (Gerrit)

unread,

Jun 23, 2022, 1:42:58 PM6/23/22

to Robert Flack, blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org

Attention is currently required from: Robert Flack.

Mason Freed would like Robert Flack to review this change.

View Change

Allow hide animations to be started from pop up hide event

See [1] for the origin of this change, which makes it possible to
trigger pop up hide animations from within the `hide` event handler.
For example:

  popup.addEventListener('hide', () => {
    popup.animate({
      transform: 'translateY(-50px)',
      opacity: 0,
    }, 200);
  });

To accomplish that, the hide process now has *two* embedded "wait"
states:
 1. Waiting for the (asynchronous) hide event to fire, then
 2. Waiting for all running animations to complete (pre-existing state).

Here is the full "hide" logic:
  1. Capture any already-running animations via getAnimations(),
     including animations on descendent elements.
  2. Remove the :top-layer pseudo class.
  3. Queue the 'hide' event.
  4. If the hidePopup() call is *not* the result of the popup being
     "forced out" of the top layer, e.g. by a modal dialog or fullscreen
     element:
    a. Restore focus to the previously-focused element.
    b. Wait for the 'hide' event to fire, to allow animations to be
       started from the 'hide' event handler.
    c. Update style. (Animations/transitions start here.)
    d. Call getAnimations() again, remove any from step #1, and then
       wait until all of them finish or are cancelled.
  5. Remove the popup from the top layer, and add the UA display:none
     style.
  6. Update style.

Note that this change, due to the required async wait for the hide
event, changes observable behavior slightly for normal (non-forced) hide
and light dismiss, in that the event loop must spin once before the
popup is actually hidden. That is now reflected in tests.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/3688871/9/third_party/blink/renderer/core/dom/element.cc#2660

Bug: 1307772

Change-Id: I910535b13cfc3c8f8498ed64dae73caa75dd7317
---
M third_party/blink/renderer/core/dom/build.gni
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/dom/popup_animation_finished_event_listener.cc
M third_party/blink/renderer/core/dom/popup_data.h
A third_party/blink/renderer/core/dom/popup_hide_event_listener.cc
A third_party/blink/renderer/core/dom/popup_hide_event_listener.h
M third_party/blink/renderer/core/html/forms/html_form_control_element.cc
M third_party/blink/renderer/core/html/forms/html_select_menu_element.cc
M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-animation-corner-cases.tentative.html
M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html
M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-focus.tentative.html
M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-shadow-dom.tentative.html
13 files changed, 410 insertions(+), 114 deletions(-)

To view, visit change 3708419. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Jun 23, 2022, 1:43:02 PM6/23/22

to blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org, Robert Flack, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov

Attention is currently required from: Robert Flack.

Patch set 3:Auto-Submit +1Commit-Queue +1

View Change

1 comment:

Patchset:
- Patch Set #3:
  Ok, here's the followup to the other big animation patch. I think it's fairly clean, but let me know if you see anything that can be improved. Thanks!

To view, visit change 3708419. To unsubscribe, or for help writing mail filters, visit settings.

Blink W3C Test Autoroller (Gerrit)

unread,

Jun 23, 2022, 1:46:25 PM6/23/22

to Mason Freed, blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org, Robert Flack, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov

Attention is currently required from: Robert Flack.

Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/34561.

When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.

WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process

View Change

To view, visit change 3708419. To unsubscribe, or for help writing mail filters, visit settings.

Robert Flack (Gerrit)

unread,

Jun 24, 2022, 1:35:27 PM6/24/22

to Mason Freed, blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org, Blink W3C Test Autoroller, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov

Attention is currently required from: Mason Freed.

View Change

3 comments:

File third_party/blink/renderer/core/dom/element.cc:
- Patch Set #3, Line 2666: GetDocument().EnqueueAnimationFrameEvent(event);
  I'm a bit curious about the timing of this event, wouldn't it be more in line with other stateful events (e.g. like focus) to dispatch it immediately rather then queuing it?
  It would also be simpler if we can do all of the popup hide logic at this time, then we wouldn't need to hold onto the previously running animations beyond the current stack, and we don't risk paying attention to possibly unrelated animations that happened to start between the hidepopup call and invoking the callback.
- Patch Set #3, Line 2688: might have re-shown this popup
  To be clear, we are ignoring the attempt to re-show the popup by doing this, right?
- Patch Set #3, Line 2717: PseudoStateChanged(CSSSelector::kPseudoPopupHidden);
  I think there are a couple cases where we might need to call takePreviouslyRunningAnimations to ensure we don't hold references to those.
  I think if the popup is shown you do explicitly clean this list, but if the popup was force hidden I think it may be leaked.

To view, visit change 3708419. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Jun 24, 2022, 8:43:14 PM6/24/22

to blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org

Attention is currently required from: Mason Freed.

Mason Freed uploaded patch set #5 to this change.

View Change

Allow hide animations to be started from pop up hide event

See [1] for the origin of this change, which makes it possible to
trigger pop up hide animations from within the `hide` event handler.
For example:

  popup.addEventListener('hide', () => {
    popup.animate({
      transform: 'translateY(-50px)',
      opacity: 0,
    }, 200);
  });

To accomplish that, the hide process now looks like this:



 1. Capture any already-running animations via getAnimations(),
    including animations on descendent elements.
 2. Remove the :top-layer pseudo class.

 3. Fire the 'hide' event.
 4. If the hidePopup() call is *not* the result of the pop-up being


    "forced out" of the top layer, e.g. by a modal dialog or fullscreen
    element:
   a. Restore focus to the previously-focused element.

   b. Update style. (Animations/transitions start here.)
   c. Call getAnimations() again, remove any from step #1, and then wait


      until all of them finish or are cancelled.

 5. Remove the pop-up from the top layer, and add the UA display:none
    style.
 6. Update style.


[1] https://chromium-review.googlesource.com/c/chromium/src/+/3688871/9/third_party/blink/renderer/core/dom/element.cc#2660

Bug: 1307772

Change-Id: I910535b13cfc3c8f8498ed64dae73caa75dd7317
---

M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/dom/popup_animation_finished_event_listener.cc

M third_party/blink/renderer/core/html/forms/html_form_control_element.cc
M third_party/blink/renderer/core/html/forms/html_select_menu_element.cc
M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-animation-corner-cases.tentative.html
M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html

M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-events.tentative.html
M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-shadow-dom.tentative.html
9 files changed, 230 insertions(+), 115 deletions(-)

To view, visit change 3708419. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Jun 24, 2022, 8:43:52 PM6/24/22

to blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org, Blink W3C Test Autoroller, Robert Flack, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov

Attention is currently required from: Robert Flack.

Patch set 5:Auto-Submit +1Commit-Queue +1

View Change

4 comments:

Patchset:
- Patch Set #5:
  Thanks for the review! Great input, as always. Changes have all been made.
File third_party/blink/renderer/core/dom/element.cc:

- Patch Set #3, Line 2666: GetDocument().EnqueueAnimationFrameEvent(event);

- I'm a bit curious about the timing of this event, wouldn't it be more in line with other stateful ev […]
  Yeah, you're probably right. And I think I was avoiding that to avoid the complexities of synchronous events. But the complexity of this patch is way more than it would be with a synchronous hide.
  I've updated the CL to synchronously fire both `show` and `hide`. It simplifies things in all the ways you mention. PTAL.

- Patch Set #3, Line 2688: might have re-shown this popup
  To be clear, we are ignoring the attempt to re-show the popup by doing this, right?

- This code is gone now, but yes, this was likely a bug. Should have been a plain `return`.

- Patch Set #3, Line 2717: PseudoStateChanged(CSSSelector::kPseudoPopupHidden);

- I think there are a couple cases where we might need to call takePreviouslyRunningAnimations to ensu […]
  Let me know if you see any of these places in the new patchset. It is simplified compared to the old code, but I could have missed something.

To view, visit change 3708419. To unsubscribe, or for help writing mail filters, visit settings.

Robert Flack (Gerrit)

unread,

Jun 27, 2022, 3:55:33 PM6/27/22

to Mason Freed, blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org, Blink W3C Test Autoroller, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov

Attention is currently required from: Mason Freed.

View Change

4 comments:

File third_party/blink/renderer/core/dom/element.cc:
- Patch Set #3, Line 2717: PseudoStateChanged(CSSSelector::kPseudoPopupHidden);
  Let me know if you see any of these places in the new patchset. […]
  The new patch is much simpler to verify since you don't keep the previously running animations beyond the HidePopUpInternal frame 😊
File third_party/blink/renderer/core/dom/element.cc:
- Patch Set #5, Line 2670: return PopupHideFinishIfNeeded();
  Seems like this could go in the block on line 2651 after queuing the event, since when force_hide is true we enqueue the event so nothing should have changed resulting in an early return on line 2665 right?
- Patch Set #5, Line 2695: previously_focused_element->Focus(focus_options);
  Is it okay that we focus the previous element before the popup is hidden (i.e. while we still have something matching :top-layer)?
  Conceptually I think this is fine and probably leads to a better experience (e.g. having the immediate focus change while the dialog is fading away), but just wanted to check with you that we won't accidentally block focusing the new element or anything like that.
- Patch Set #5, Line 2707: GetPopupData()->setAnimationFinishedListener(nullptr);
  If we have no animations, since the PopupAnimationFinishedEventListener constructor synchronously calls this method we'll end up here before we've called setAnimationFinishedListener on line 2683 and leak the listener right? Maybe we should instead skip constructing the listener if there are no animations?

To view, visit change 3708419. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Jun 27, 2022, 5:34:51 PM6/27/22

to blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org, Blink W3C Test Autoroller, Robert Flack, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov

Attention is currently required from: Robert Flack.

Patch set 6:Auto-Submit +1

View Change

5 comments:

Patchset:
- Patch Set #6:
  Thanks - all comments addressed.
File third_party/blink/renderer/core/dom/element.cc:

- Patch Set #3, Line 2717: PseudoStateChanged(CSSSelector::kPseudoPopupHidden);

- The new patch is much simpler to verify since you don't keep the previously running animations beyon […]
  Done
File third_party/blink/renderer/core/dom/element.cc:

- Patch Set #5, Line 2670: return PopupHideFinishIfNeeded();

- Seems like this could go in the block on line 2651 after queuing the event, since when force_hide is […]
  Yep, good catch. Done.

- Patch Set #5, Line 2695: previously_focused_element->Focus(focus_options);

- Is it okay that we focus the previous element before the popup is hidden (i.e. […]
  That was my thought exactly - that it would be nice to focus the previous element immediately, so that if the popup is fading away, that doesn't block/delay the user from moving on.
  I don't believe we'll block focusing the new element - the popup-focus.tentative.html test verifies these cases, and to double-check, I've added transitions to all of its popups. That only required adding a bit more logic to the end of the last test, which previously assumed it couldn't focus the immediately-hidden pop-up-contained prior focus element.

- Patch Set #5, Line 2707: GetPopupData()->setAnimationFinishedListener(nullptr);

- If we have no animations, since the PopupAnimationFinishedEventListener constructor synchronously ca […]
  Wow good catch. I thought I was cleaning up the code this way, but I agree I'll put back the check in element. Done.

To view, visit change 3708419. To unsubscribe, or for help writing mail filters, visit settings.

Robert Flack (Gerrit)

unread,

Jun 28, 2022, 11:06:22 AM6/28/22

to Mason Freed, blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org, Blink W3C Test Autoroller, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov

Attention is currently required from: Mason Freed.

Patch set 7:Code-Review +1Commit-Queue +2

View Change

1 comment:

Patchset:
- Patch Set #7:
  LGTM

To view, visit change 3708419. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Jun 28, 2022, 11:09:37 AM6/28/22

to blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org, Robert Flack, Blink W3C Test Autoroller, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov

Patch set 7:Auto-Submit +1Commit-Queue +2

View Change

1 comment:

Patchset:
- Patch Set #7:
  Thanks!

To view, visit change 3708419. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Jun 28, 2022, 11:10:10 AM6/28/22

to Mason Freed, blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org, Robert Flack, Blink W3C Test Autoroller, Alexis Menard, chromium...@chromium.org, Olga Gerchikov

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Mason Freed: Send CL to CQ automatically after approval; Commit
  Robert Flack: Looks good to me; Commit

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3708419
Reviewed-by: Robert Flack <fla...@chromium.org>
Commit-Queue: Mason Freed <mas...@chromium.org>
Commit-Queue: Robert Flack <fla...@chromium.org>
Auto-Submit: Mason Freed <mas...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1018685}


---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/dom/popup_animation_finished_event_listener.cc
M third_party/blink/renderer/core/html/forms/html_form_control_element.cc
M third_party/blink/renderer/core/html/forms/html_select_menu_element.cc
M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-animation-corner-cases.tentative.html
M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html
M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-events.tentative.html

M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-focus.tentative.html
M third_party/blink/web_tests/external/wpt/html/semantics/popups/popup-shadow-dom.tentative.html
10 files changed, 263 insertions(+), 117 deletions(-)

To view, visit change 3708419. To unsubscribe, or for help writing mail filters, visit settings.

Blink W3C Test Autoroller (Gerrit)

unread,

Jun 28, 2022, 11:28:22 AM6/28/22

to Chromium LUCI CQ, Mason Freed, blink-revie...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, dtapuska+...@chromium.org, Robert Flack, Alexis Menard, chromium...@chromium.org, Olga Gerchikov

The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/34561

View Change

To view, visit change 3708419. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward