fix(@react-aria/overlays): prevent exception on Cypress #2341

alirezamirian · 2021-09-16T10:38:44Z

Cypress screenshot command triggers a scroll event where the event target is window, not a DOM element.

Closes #2340

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Let me know if needed.

LFDanLu · 2021-09-23T23:39:07Z

packages/@react-aria/overlays/src/useCloseOnScroll.ts

-      let target = e.target as HTMLElement;
-      if (!triggerRef.current || !target.contains(triggerRef.current)) {
+      let target = e.target;
+      if (!triggerRef.current || !(target instanceof  HTMLElement) || !target.contains(triggerRef.current)) {


Won't this prevent our overlays from automatically closing when the user scrolls the window?

I don't think so. If target is an instance of HTMLElement then it behaves as before, and if it's not, it just prevents a runtime error which would happen by accessing .contains On something that doesn't have it.
Technically, the check could be on Node rather than HTMLElement but in IE, it's defined only on HTMLElement instances, and I thought there ia no harm in checking on HTMLElement, even though IE is not supported by react-aria

Hm, I tested this out locally in our storybook via our DialogTrigger "type: popover" story + making the body scrollable and it no longer closes the overlay on scroll.

Perhaps

Suggested change

if (!triggerRef.current || !(target instanceof HTMLElement) || !target.contains(triggerRef.current)) {

if (!triggerRef.current || ((target instanceof HTMLElement) && !target.contains(triggerRef.current))) {

since window.scroll is a valid call. Thanks @snowystinger for pointing the above out

You are right. The reason that's broken is that target can be document which is not an instance of HTMLElement. .contains is actually on Node's prototype and document is an instance of Node. It's only in IE9 that .contains is on HTMLElement's prototype. So I think The right change would be to check if the target is an instance of Node.

Pushed some update that should fix both the issue in cypress and the regression.
Note that while the suggested change here fix the regression, and also fixes the runtime error on cypress, it doesn't completely solve the issue on cypress. For some reason, the screenshot command in cypress triggers an artificial scroll event where the target is set to window. Checking with instanceof Node and bailing out otherwise would skip unwanted closure in cypress and works in other cases too.

Sorry for the regression BTW, and thanks for catching it. I shouldn't have considered IE9 in the first place :P

Haha, no worries, thanks for the fix!

Cypress screenshot command triggers a scroll event where the event target is window, not a DOM element. closes adobe#2340

snowystinger

lgtm, though we should probably have a test to prevent that regression found in the future
would you mind leaving a copy of the code that caused the regression so we can write a test against it?

alirezamirian · 2021-09-27T18:39:16Z

Good point @snowystinger
Pushed an update which adds two test cases. the first one covers the regression that happened first in this PR. the second one captures the fix for the cypress issue that this PR solves.

related to adobe#2340. The first test case is related to the regression mentioned here: adobe#2341 (comment) The second test case captures the fix for the cypress issue.

snowystinger

Hey, thanks again for submitting this. We had a team talk about the issue and we'd like for window scroll events to behave the same as the document scroll. I know the previous implementation was blowing up due to the window scroll, and that is still something that should be fixed.
See #1852 (comment) and #900 for more information on why closing overlays on scroll is important for us.
There are cases other than Cypress where the window may be the target of the scroll event.

Thanks again for your patience, if you don't have time to make the change, I'm happy to do it. Just let me know

snowystinger · 2021-10-13T18:01:04Z

packages/@react-aria/overlays/test/useOverlayPosition.test.js

+    expect(onClose).toHaveBeenCalledTimes(1);
+  });
+
+  it('should not throw or close when target is window in a scroll event', function () {


so we'll want this test to be the same as the document scroll test, still good to have both tests

alirezamirian · 2021-10-13T18:53:47Z

@snowystinger No problem. So if we want to close on window scroll events too, considering that contains is meaningless and unsupported for window, we would just need to check if the target is not window, when we are bailing out. So the change seems simple. Although the problem with Cypress will remain in that case, since the target is actually window in those artificial scroll events that are raised by the Cypress screenshot command.
But can it ever happen for a real scroll event to have window as the target? I mean even when there is no intermediate scrollable element, and the whole window is scrollable, the target of scroll events is still document, not window, in whatever scenario I test.

snowystinger · 2021-10-13T19:47:27Z

I'll be honest, I'm having trouble figuring out if window.scroll is valid. It's certainly available to JS, which is what Cypress is taking advantage of. But is Cypress the only one taking advantage of it? I don't know. We think it's safer if it behaves in the same expected manner as document.scroll.

The real fix that we eventually want to make is probably an invisible underlay for all of our popover type components, this would block any scrolling and then we'd be able to drop this scroll listener. This is coming from the comment I linked earlier #1852 (comment)

alirezamirian requested review from dannify, devongovett and snowystinger as code owners September 16, 2021 10:38

LFDanLu added the small review Easy to review PR label Sep 22, 2021

LFDanLu reviewed Sep 23, 2021

View reviewed changes

fix(@react-aria/overlays): prevent exception on Cypress

8be7d91

Cypress screenshot command triggers a scroll event where the event target is window, not a DOM element. closes adobe#2340

alirezamirian force-pushed the 2340-overlays-cypress-issue branch from dafc3ce to 8be7d91 Compare September 27, 2021 17:59

LFDanLu previously approved these changes Sep 27, 2021

View reviewed changes

snowystinger previously approved these changes Sep 27, 2021

View reviewed changes

Merge branch 'main' into 2340-overlays-cypress-issue

a7abbdb

test(@react-aria/overlays): add two test case for close on scroll

c5f5266

related to adobe#2340. The first test case is related to the regression mentioned here: adobe#2341 (comment) The second test case captures the fix for the cypress issue.

alirezamirian dismissed stale reviews from snowystinger and LFDanLu via c5f5266 September 27, 2021 18:40

LFDanLu previously approved these changes Sep 27, 2021

View reviewed changes

snowystinger reviewed Oct 13, 2021

View reviewed changes

snowystinger added 2 commits October 27, 2021 17:50

Merge branch 'main' into 2340-overlays-cypress-issue

6e8d812

window scroll to behave same as document

c246bed

snowystinger dismissed LFDanLu’s stale review via c246bed October 27, 2021 23:26

remove comment that no longer applies

46d5f12

snowystinger approved these changes Oct 27, 2021

View reviewed changes

Merge branch 'main' into 2340-overlays-cypress-issue

e966664

LFDanLu approved these changes Oct 28, 2021

View reviewed changes

LFDanLu merged commit c8a6c80 into adobe:main Oct 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(@react-aria/overlays): prevent exception on Cypress #2341

fix(@react-aria/overlays): prevent exception on Cypress #2341

alirezamirian commented Sep 16, 2021

LFDanLu Sep 23, 2021

alirezamirian Sep 24, 2021 •

edited

LFDanLu Sep 27, 2021

LFDanLu Sep 27, 2021 •

edited

alirezamirian Sep 27, 2021 •

edited

alirezamirian Sep 27, 2021 •

edited

alirezamirian Sep 27, 2021

LFDanLu Sep 27, 2021

snowystinger left a comment

alirezamirian commented Sep 27, 2021 •

edited

snowystinger left a comment

snowystinger Oct 13, 2021

alirezamirian commented Oct 13, 2021

snowystinger commented Oct 13, 2021

	if (!triggerRef.current \|\| !(target instanceof HTMLElement) \|\| !target.contains(triggerRef.current)) {
	if (!triggerRef.current \|\| ((target instanceof HTMLElement) && !target.contains(triggerRef.current))) {

fix(@react-aria/overlays): prevent exception on Cypress #2341

fix(@react-aria/overlays): prevent exception on Cypress #2341

Conversation

alirezamirian commented Sep 16, 2021

✅ Pull Request Checklist:

📝 Test Instructions:

LFDanLu Sep 23, 2021

Choose a reason for hiding this comment

alirezamirian Sep 24, 2021 • edited

Choose a reason for hiding this comment

LFDanLu Sep 27, 2021

Choose a reason for hiding this comment

LFDanLu Sep 27, 2021 • edited

Choose a reason for hiding this comment

alirezamirian Sep 27, 2021 • edited

Choose a reason for hiding this comment

alirezamirian Sep 27, 2021 • edited

Choose a reason for hiding this comment

alirezamirian Sep 27, 2021

Choose a reason for hiding this comment

LFDanLu Sep 27, 2021

Choose a reason for hiding this comment

snowystinger left a comment

Choose a reason for hiding this comment

alirezamirian commented Sep 27, 2021 • edited

snowystinger left a comment

Choose a reason for hiding this comment

snowystinger Oct 13, 2021

Choose a reason for hiding this comment

alirezamirian commented Oct 13, 2021

snowystinger commented Oct 13, 2021

alirezamirian Sep 24, 2021 •

edited

LFDanLu Sep 27, 2021 •

edited

alirezamirian Sep 27, 2021 •

edited

alirezamirian Sep 27, 2021 •

edited

alirezamirian commented Sep 27, 2021 •

edited