feat: add `start` and `end` hooks to the `fireEvent` helper #1185

shamrt · 2022-01-14T20:43:16Z

REVIEWER NOTE: Use of Hide whitespace option recommended.

Following suggestions in #1182 and #1183, the following PR:

converts the fireEvent helper to a Promise in order to support the hooks system
adds start and end hooks to the fireEvent helper
further adds start:[eventType] and end:[eventType] hooks to the fireEvent helper for finer control
"promisifies" all functions that use the fireEvent helper
adds internal test helpers for asserting the event hooks are accessible
adds tests for the new hooks in all relevant DOM helpers

scalvert · 2022-01-14T22:15:19Z

Thanks for doing this, @shamrt. Would you mind taking a look at the failing CI?

Error: addon-test-support/@ember/test-helpers/dom/tab.ts(233,11): error TS7030: Not all code paths return a value.

scalvert · 2022-01-14T22:27:45Z

In broad strokes this changes seems really great. One issue is I think there may be unrelated changes included in this PR, which makes it a tad more difficult to review. Is there anything we can pull out as a separate, discrete change?

shamrt · 2022-01-15T15:27:40Z

Is there anything we can pull out as a separate, discrete change?

I'd say there are a couple of changes that aren't necessary to this PR, such as:

switches from let to const
refactors to tests not strictly related to the new hooks

I'll revert those changes. If you can think of any others, please let me know.

shamrt · 2022-01-15T15:37:46Z

addon-test-support/@ember/test-helpers/dom/focus.ts

+    })
+    .then(({ focusTarget, previousFocusedElement }) => {
+      if (!focusTarget) {
+        throw new Error('There was a previously focused element');


Throwing an error here (and later catching) in order to mimic the behaviour previously handled by a return statement.

This reverts commit 5b7bf9c.

This reverts commit 2d6170c.

shamrt · 2022-01-15T16:01:30Z

Alright, I've reverted a few things. Hopefully that tightens up the PR a bit.

shamrt · 2022-01-18T14:48:47Z

Checking in to see whether I've covered everything for this PR. I think it's ready for re-review. Thanks!

scalvert · 2022-01-19T22:08:06Z

@shamrt I'll take another look. Thanks so much for simplifying!

scalvert · 2022-01-19T22:10:40Z

addon-test-support/@ember/test-helpers/dom/fire-event.ts

+    .then(() => runHooks('fireEvent', 'start', element))
+    .then(() => runHooks(`fireEvent:${eventType}`, 'start', element))


I like the flexibility this provides. It's a nice composition of general to specific!

scalvert · 2022-01-19T22:13:28Z

addon-test-support/@ember/test-helpers/dom/focus.ts

+            )
+            .then(() => settled());
+    })
+    .catch(() => {});


Should we not let the exception propagate and fail the test if an exception is thrown?

I mention in the comment above that the thrown error is a stand-in for the earlier return statement (see L62 of original code). Given we simply want to short-circuit the Promise chain, I didn't see any purpose in processing or propagating the error.

Additionally, given this function is being executed in a nested Promise within the focus function (see L155 of new code), the parent Promise chain doesn't know whether the child Promise resolved or was rejected (AFAIK).

This should result in the same behaviour we had when this function was synchronous. It doesn't look or feel great, but it gets the job done.

That said, I'm open to suggestions!

scalvert · 2022-01-19T22:17:10Z

Overall this looks really good. It's crazy how much simpler the code would be with async/await...

shamrt · 2022-01-31T13:57:32Z

@rwjblue Any chance you'll have time to review this PR this week? Thanks!

mansona · 2022-04-07T09:00:26Z

Hey folks 👋 I'm just pinging here in case we didn't see my issue from yesterday: #1210

I don't know how widespread this is going to be but if a lot of people are going to hit this problem it might be good to move on it sooner rather than later. Let me know your thoughts 👍

shamrt · 2022-04-08T14:03:18Z

Hi @mansona. I'm very sorry this happened!

I'm taking a look at the issue today, which I've been able to replicate. Hopefully I can come up with a patch for you.

mansona · 2022-04-11T13:38:55Z

No worries, thanks for looking into it @shamrt 🎉

Can you expand a bit what the difference is? and how wide-reaching the regression is likely to be? I'm trying to figure out if this is going to be a widespread enough issue that we should really revert sooner rather than later and add the fix to an updated PR. I don't really know enough about the issue to be able to judge but maybe you have an idea, especially since you have been able to replicate the issue

scalvert · 2022-04-11T16:04:14Z

@shamrt any more info on what the issue is?

shamrt · 2022-04-11T22:03:19Z

I'm still digging into it at work. The apparent source is unclear, but I'll post progress either way tomorrow

shamrt · 2022-04-12T14:10:27Z

TL;DR: I've spent a solid chunk of time trying to debug this issue, but have run up against a wall, and have been forced to time-box it at work.

Debug and reproduction steps:

Does update to @[email protected] result in breakage?
- No
Does update to @[email protected] result in breakage?
- Yes
Check whether it's a focus issue (__focus__ is executed during some clicks)
- not a focus issue
Trace performed actions with browser debugger
- looks like the library now performs a "double-click" on L710 with the click(document.body) strategy — once with mousedown dispatch, then with click dispatch
- Open question: Does the Promisification negate the need for the mousedown and mouseup events in __click__?
  - Seems not
- With old version of @ember/test-helpers (v2.4.2), only the click event performs an actual click event, disappearing the popover
  - selector: body
  - actions:
    - mousedown on body
    - blur on button:nth-child(2)
    - focusout on button:nth-child(2)
    - mouseup on body
    - click on body
  - result: one dialog disappears
- in v2.7.0:
  - selector: body
  - actions:
    - mousedown on body (one dialog disappears)
    - blur on button:nth-child(2) (NOT RUN)
    - focusout on button:nth-child(2) (NOT RUN)
    - mouseup on body
    - click on body (second dialog disappears)
Step through code in browser to trace why __blur__ not being run in v2.7.0
- previousFocusedElement is button:nth-child(2) and focusTarget is null, which performs blur and focusout events
- Something in the mousedown event is causing the Dialog to close
- Seems to be something running in backburner.js and executed by this._boundAutorunEnd (this._end) that closes the Dialog
- Could be there's something added to Ember's event loop that causes an error or the "double-click" behaviour
- Digging further, I've observed the following error, followed by a this.component.destroy() execution, which closes the Dialog:
```
 'Error
 at Backburner.schedule (http://localhost:7357/assets/vendor.js:60158:32)
 at schedule (http://localhost:7357/assets/vendor.js:42214:24)
 at scheduleDestroy (http://localhost:7357/assets/vendor.js:8880:29)
 at http://localhost:7357/assets/vendor.js:43613:75
 at iterate (http://localhost:7357/assets/vendor.js:43504:7)
 at destroy (http://localhost:7357/assets/vendor.js:43613:5)
 at iterate (http://localhost:7357/assets/vendor.js:43504:7)
 at destroy (http://localhost:7357/assets/vendor.js:43611:5)
 at iterate (http://localhost:7357/assets/vendor.js:43501:9)
 at destroy (http://localhost:7357/assets/vendor.js:43611:5)'
```
- I don't know enough about the guts of Ember to be able to debug this, or understand why it's occurring.
Pull back from debugging, and instead simplify @ember/test-helpers implementation locally using async/await
- Converted most helpers so comparison of the logic with @ember/[email protected] is as straightforward as possible.
  - See: https://github.com/rewardops/ember-test-helpers/tree/async-await-helpers
    - NOTE: In your IDE, if you have a comparison function, compare the HEAD of that branch with the v2.6.0 tagged commit. The differences are much easier to follow
- Run yarn prepack in @ember/test-helpers and link with ember-headlessui
- Issue still present in ember-headlessui
- Disable runHooks in fire-event.ts
- Issue still present in ember-headlessui

I'm at a complete loss, at this point, as to why this issue has sprung up. It appears to be a legitimate edge case, but I'm flummoxed.

Side-question for the library maintainers: Given the library now only supports Ember 3.8, Node 10 supports async/await, and Node 10+ has been supported since Ember 3.1.3, is there a reason Node <8 support been hasn't dropped? Doing so would facilitate the move to async/await. I assume there are Ember 3.8 projects out there still using Node 8?

shamrt · 2022-04-12T15:16:08Z

Here's a GIF of the issue when it occurs

Shane Martin added 30 commits January 13, 2022 10:23

feat: add start and end hooks to fireEvent helper

eded8e7

refactor: migrate blur, click, focus, tap to fire async events

5b2c2ae

refactor: migrate fillIn, typeIn to fire async events

1b89095

docs: add details to JSDoc

599ae74

test: move all register hook tests into helper modules

d488116

refactor: promisify triggerEvent helper

31b62f8

chore: remove lingering import

2690988

refactor: promisify doubleClick helper

37a9414

test: add more/improve register hooks helpers

ab797ef

refactor: always return Promise for focus step

96ea125

test: add assertions for fire event hook steps

c6e3ff2

test: use improved hook test helpers

f292678

test: use improved hook test helpers

2e8de27

docs: clarify param description

4318212

refactor: promisify tab fire event func

f410f57

refactor: promisify trigger-key-event helper

0a6e687

refactor: promisify select helper

786c245

test: add event hooks assertions to execution test

f633cf1

test: rename helper eventTypes params to expectedEvents

c7c851f

test: use hook test helpers

2d6170c

chore: remove finished TODOs

93bc5df

refactor: encapsulate specific start/end hooks with generic hooks

9ded7a2

refactor: fully promisify fillIn helper

b70b17a

fix: re-add settled func after events fired

ef8f92b

chore: remove unused imports

b2f08fe

refactor: promisify scrollTo helper

03db0d7

test: add event hooks assertions to execution test

7506700

refactor: restore prior settled behaviour

add69f6

refactor: let -> const

5b7bf9c

chore: remove unused import

03f93db

scalvert self-requested a review January 14, 2022 22:12

refactor: add fallback Promise resolution

27cde93

shamrt commented Jan 15, 2022

View reviewed changes

Shane Martin added 5 commits January 15, 2022 10:42

Revert "refactor: let -> const"

f1ff128

This reverts commit 5b7bf9c.

Revert "test: use hook test helpers"

126d4b0

This reverts commit 2d6170c.

revert: undo unrelated assertion replacements

1bab92a

revert: restore chained settled Promise

1fb879a

revert: restore _registerHook import

61cfbb3

scalvert added the enhancement label Jan 19, 2022

scalvert approved these changes Jan 19, 2022

View reviewed changes

rwjblue approved these changes Apr 4, 2022

View reviewed changes

rwjblue merged commit a395635 into emberjs:master Apr 4, 2022

This was referenced Apr 6, 2022

Breaking change released as a minor version #1210

Open

fix rootElement when running in /tests GavinJoyce/ember-headlessui#139

Merged

shamrt mentioned this pull request Apr 6, 2022

feat: modernize addon, add optional Ember mixin props to decorator, testing helper alexlafroscia/ember-react-components#62

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `start` and `end` hooks to the `fireEvent` helper #1185

feat: add `start` and `end` hooks to the `fireEvent` helper #1185

shamrt commented Jan 14, 2022 •

edited

Loading

scalvert commented Jan 14, 2022

scalvert commented Jan 14, 2022

shamrt commented Jan 15, 2022

shamrt Jan 15, 2022 •

edited

Loading

shamrt commented Jan 15, 2022

shamrt commented Jan 18, 2022

scalvert commented Jan 19, 2022

scalvert Jan 19, 2022

scalvert Jan 19, 2022

shamrt Jan 19, 2022 •

edited

Loading

scalvert commented Jan 19, 2022

shamrt commented Jan 31, 2022

mansona commented Apr 7, 2022

shamrt commented Apr 8, 2022

mansona commented Apr 11, 2022

scalvert commented Apr 11, 2022

shamrt commented Apr 11, 2022

shamrt commented Apr 12, 2022 •

edited

Loading

shamrt commented Apr 12, 2022 •

edited

Loading

		.then(() => runHooks('fireEvent', 'start', element))
		.then(() => runHooks(`fireEvent:${eventType}`, 'start', element))

feat: add start and end hooks to the fireEvent helper #1185

feat: add start and end hooks to the fireEvent helper #1185

Conversation

shamrt commented Jan 14, 2022 • edited Loading

scalvert commented Jan 14, 2022

scalvert commented Jan 14, 2022

shamrt commented Jan 15, 2022

shamrt Jan 15, 2022 • edited Loading

Choose a reason for hiding this comment

shamrt commented Jan 15, 2022

shamrt commented Jan 18, 2022

scalvert commented Jan 19, 2022

scalvert Jan 19, 2022

Choose a reason for hiding this comment

scalvert Jan 19, 2022

Choose a reason for hiding this comment

shamrt Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

scalvert commented Jan 19, 2022

shamrt commented Jan 31, 2022

mansona commented Apr 7, 2022

shamrt commented Apr 8, 2022

mansona commented Apr 11, 2022

scalvert commented Apr 11, 2022

shamrt commented Apr 11, 2022

shamrt commented Apr 12, 2022 • edited Loading

shamrt commented Apr 12, 2022 • edited Loading

feat: add `start` and `end` hooks to the `fireEvent` helper #1185

feat: add `start` and `end` hooks to the `fireEvent` helper #1185

shamrt commented Jan 14, 2022 •

edited

Loading

shamrt Jan 15, 2022 •

edited

Loading

shamrt Jan 19, 2022 •

edited

Loading

shamrt commented Apr 12, 2022 •

edited

Loading

shamrt commented Apr 12, 2022 •

edited

Loading