Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
293 stars 75 forks source link

UI that cancels `click` events prevents popovers from auto-closing #9013

Closed nwhittaker closed 5 months ago

nwhittaker commented 7 months ago

Check existing issues

Actual Behavior

Given a popover with auto-close enabled, clicking an element that prevents propagation of the click event causes the popover to remain open.

Expected Behavior

Clicking an element that prevents propagation of the click event does not cause the popover to remain open.

Reproduction Sample

https://codepen.io/nwhittaker-esri/pen/MWRvPzQ

Reproduction Steps

  1. Visit the repro sample and click the Layer information button to open the popover.
  2. With the popover still open, click the Button with event cancelation button and observe the popover remains opened.
  3. Optionally click a blank area in the document body and see the popover closes.

Reproduction Version

2.7.0

Relevant Info

Possibly regressed by https://github.com/Esri/calcite-design-system/pull/8983. Is it possible to restore the handler to the capture phase and inspect the event's target there to see if it's disabled or aria-disabled instead?

Other components that exhibit the same regressed behavior: combobox, dropdown, and input-time-zone.

Regression?

2.6.0

Priority impact

p2 - want for current milestone

Impact

This change in behavior is breaking our tests in ways that are not easily solvable and preventing us from upgrading to Calcite 2.7. Going forward, it also prevents us from implementing UI with any sort of click cancelation.

A workaround could be to have our handlers relay a new click event to the window, but that doesn't solve the problem where the cancelation was intended to prevent bubbling up to another handler that is also on the window.

Calcite package

Esri team

ArcGIS Field Apps

timmorey commented 7 months ago

This issue is currently blocking us from adopting 2.7+ in field-maps-designer. Over the last 5+ yrs of development, we have accumulated a number of stopPropagation() calls on click events in various parts of the UI. Each of these is now effectively a "dead spot" in the app where a click interaction will fail to close a popover/dropdown/etc.

Maybe we could find better solutions than doing stopPropagations() in some of these places, but the effort to re-evaluate each would be quite large. I have to presume that a number of other apps are in a similar situation, since stopPropagation() is a common tool to control how events are handled.

jcfranco commented 7 months ago

@nwhittaker @timmorey Apologies for the delayed response. We are exploring alternative solutions and potential workarounds. Worst-case scenario, we may need to revert #8983 for the upcoming release and aim to reinstall in May.

It's important to note that we want all components to use the target/bubbling event phase for consistency and to meet developer expectations. Currently, relying on the capture phase is problematic as it complicates other fixes we need to implement.

Maybe we could find better solutions than doing stopPropagations() in some of these places, but the effort to re-evaluate each would be quite large.

Agreed. It'll take some effort to re-evaluate stopPropagation() usage, but it seems like a worthwhile task to help prevent disruptions and bugs.

cc @geospatialem

nwhittaker commented 7 months ago

It's important to note that we want all components to use the target/bubbling event phase for consistency and to meet developer expectations. Currently, relying on the capture phase is problematic as it complicates other fixes we need to implement.

@jcfranco is it possible to make a distinction: Events targeting elements inside a Calcite shadow-dom vs. events targeting elements outside a Calcite shadow-dom? Where the former events use bubbling and the latter use capturing?

Alternatively, is going back to pointer events^1 for these types of "outside click" handlers an option? It's not perfect, but might be less likely to conflict with developer flows that are listening for mouse events.

jcfranco commented 7 months ago

is it possible to make a distinction: Events targeting elements inside a Calcite shadow-dom vs. events targeting elements outside a Calcite shadow-dom? Where the former events use bubbling and the latter use capturing?

I could be missing something, but this might not help with eventing expectation concern. It'll be simpler to use the same phase. Component composition might also make this tricky as the targeted component could be used within either light or shadow DOM.

Alternatively, is going back to pointer events1 for these types of "outside click" handlers an option?

I'd like to keep click for these contexts.


Workaround-wise, would it be possible to set up something like the following into your environments? The key parts of the workaround are:

  1. hijack Event.prototype.stopPropagation to extend default behavior 👇 (a custom helper to stop propagation could also be used, but went with the simplest setup)
  2. emits custom click event directly on window (should help bypass any event handlers in between and toggles popovers)
    • alternatively, this could emit a KeyboardEvent of type keydown with Escape as the pressed key
nwhittaker commented 7 months ago

I could be missing something, but this might not help with eventing expectation concern. It'll be simpler to use the same phase.

My point was there could be a distinction between events that can be canceled and ones that cannot (e.g. outside clicks). For the ones that cannot, they should be listened for in the capture phase to mitigate cancelation. There's the added complexity of checking for disabled reference elements, but it's with significant benefit to the consumer.

Component composition might also make this tricky as the targeted component could be used within either light or shadow DOM.

Wondering what you're thinking of here because the event's composedPath() function gets everything including elements in shadow DOMs.

Workaround-wise, would it be possible to set up something like the following into your environments?

I did address this in the issue description. As a general workaround, it fails for the case where I have other click handlers on the window that I want to stop the event for. I also suspect monkey patching native classes is something the team wants to avoid.


Ideally the outside-click handling just works without impact to the consumer code. Workarounds aside, I guess I'm left wondering what are the technical blockers that make that not achievable?

wei8123 commented 6 months ago

Hi, this issue effects the click event in ArcGIS Experience Builder Shadow cast tool. 2024-05-15_14-28-31

This is also reproducible with a plain Calcite app. Here's a simple app which reproduces the problem. https://codepen.io/hccampos/pen/WNBvLqK

jcfranco commented 6 months ago

I meant to follow up on this sooner, but we are not considering this a regression and will proceed to close for the following reasons:

  1. The event phases our components use are not publicly documented and are subject to change.
  2. Our components aim to follow developer expectations and therefore use the bubbling/target phase for event handling.
    1. Popover and tooltip were the only outliers using the capture phase due to the initial implementation. We recently refactored popover to use a different phase and will do the same for tooltip.
  3. Stopping event propagation can have unintended side effects and break expectations of how events normally work across components (this is important for a low-level component library).
    1. This will also cause issues with other components currently (e.g., dropdown).
    2. It is very likely to encounter the same issue with other component libraries.

I synced up with @nwhittaker a while back, and their team will pursue an app-level workaround. Not sure if the approach is ready to be shared at this time.

@wei8123 In your example, the stopImmediatePropagation is preventing all click handlers from processing the event (including the popover). Would any of the following workarounds help?

nwhittaker commented 6 months ago

Not sure if the approach is ready to be shared at this time.

Here's our current working workaround. I don't know how comprehensive it is, but it seems to work well so far. The basic approach is to create a click capture listener on the document whenever a target opens and remove it when the target closes. If the listener is invoked, it closes the target if the click occurred outside of the target or any reference-element.

https://gist.github.com/nwhittaker/e07e4e0ec4dfb0ec0a52a1fe98dfff30

EDIT: To clarify, this workaround only takes effect once the target is open. It does not endeavor to guess whether or not stopping the opening event was intended to prevent the target from opening, or to stop something else.

jcfranco commented 5 months ago

Thanks for sharing your workaround @nwhittaker!

Closing per https://github.com/Esri/calcite-design-system/issues/9013#issuecomment-2121454964.

qlqllu commented 5 months ago

ExB has a complex layout system. In the builder, we need to stop the propagation of click events when selecting a widget. And the workaround seems not work in ExB, do we miss something?

wei8123 commented 5 months ago

I meant to follow up on this sooner, but we are not considering this a regression and will proceed to close for the following reasons:

  1. The event phases our components use are not publicly documented and are subject to change.
  2. Our components aim to follow developer expectations and therefore use the bubbling/target phase for event handling.

    1. Popover and tooltip were the only outliers using the capture phase due to the initial implementation. We recently refactored popover to use a different phase and will do the same for tooltip.
  3. Stopping event propagation can have unintended side effects and break expectations of how events normally work across components (this is important for a low-level component library).

    1. This will also cause issues with other components currently (e.g., dropdown).
    2. It is very likely to encounter the same issue with other component libraries.

I synced up with @nwhittaker a while back, and their team will pursue an app-level workaround. Not sure if the approach is ready to be shared at this time.

@wei8123 In your example, the stopImmediatePropagation is preventing all click handlers from processing the event (including the popover). Would any of the following workarounds help?

Hi @jcfranco, as @qlqllu mentioned above, executing this workaround file in EXB's init does not solve the problem: 2024-06-20_15-30-24 Because clicking the problematic button in the 3D-Toolbox widget, does not trigger the events listened in the workaround file (such as 'calciteDropdownBeforeOpen', 'calciteDropdownBeforeClose'). Any idea?

jcfranco commented 4 months ago

@qlqllu @wei8123 Could you please share more details about your issue? I'm afraid I can't help without additional context. We can also schedule a meeting or call if you’d prefer.

qlqllu commented 4 months ago

@jcfranco We call the patchComponentsWithOutsideClickHandlers function in an ExB app's init process; however, we find neither the onBeforeOpen nor onBeforeClose callback gets invoked. Do we miss anything?

Here are more details on why we need to stop event propagation. ExB supports nested layout structure. For example, we can add a Button widget inside a Fix panel widget, and both of these widgets need to be selected. So, when the user clicks the Button widget to select it, we have to stop the click event propagation because if not, the user will select the Fix panel widget.

ruantao1989 commented 4 months ago

Hi, this issue effects the click event in ArcGIS Experience Builder Shadow cast tool. 2024-05-15_14-28-31

Hi @jcfranco, I created a quick demo to show how we use it in EXB: https://codepen.io/ruantao1989/pen/zYVOqwV Could you please take a look?

Thanks!

ruantao1989 commented 4 months ago

I added a demo to show our specific issue: @jcfranco https://codepen.io/ruantao1989/pen/qBzOWvg?editors=1000

https://github.com/user-attachments/assets/2ff4986c-78c1-44dd-b76e-bf70fcc2e3f7

  1. Outside the map and the calcite DOM, we have a container DOM:

    <div id="container" style="border: 2px solid red">
      <div id="mapDiv"> </div>
    </div>
    1. Due to the functionality of EXB, we need to prevent event propagation in the container, However, this causes the ShadowCast calcite component (color-picker) to fail to open
      /******************************************************
      stopping propagation on click on a container DOM:
      The color picker at the bottom of the ShadowCast component cannot be opened
      ******************************************************/
      document.querySelector("#container").addEventListener("click", (e) => {
        e.stopPropagation();
      });
jcfranco commented 4 months ago

@ruantao1989 If you're able to redirect fake clicks to window without any issue, you could apply a modified version of on of the earlier workarounds:

document.querySelector("#container").addEventListener("click", (e) => {
  e.stopPropagation();

  // workaround: redirect a fake click event to window for popover to listen to
  const fakeEvent = new CustomEvent("click", { detail: 1 });
  fakeEvent.composedPath = () => [e.target];
  window.dispatchEvent(fakeEvent);
});

See updated demo.

qlqllu commented 4 months ago

@jcfranco The fake event workaround works. However, I think this solution does not seem very clean, and it may have side effects if an app code listens to the click events on the window. Whenever an app stops the event propagation, it may have this issue and need to find this workaround solution. I still prefer the "capture" phase solution.

Nevertheless, thanks for helping figure out this workaround!

jcfranco commented 4 months ago

@qlqllu Glad to hear the workaround will work for you.

Regarding your other concerns, please refer to point 3 in the above discussion.

ruantao1989 commented 2 months ago
document.querySelector("#container").addEventListener("click", (e) => {
  e.stopPropagation();

  // workaround: redirect a fake click event to window for popover to listen to
  const fakeEvent = new CustomEvent("click", { detail: 1 });
  fakeEvent.composedPath = () => [e.target];
  window.dispatchEvent(fakeEvent);
});

See updated demo.

Hi @jcfranco, the workaround you provided before is no longer usable after the calcite update:

  1. The Timezone popup of Shadow cast will not be opened

    https://github.com/user-attachments/assets/028d8b83-3034-4d0c-9a3a-556d72c17823

  2. If the custom event is delayed, you will find that the Timezone popup will be closed by the custom event

https://github.com/user-attachments/assets/ac88d595-3c81-4dee-a255-5bdef701b7a6

Please see this demo @wei8123

jcfranco commented 2 months ago

@ruantao1989 @wei8123 Apologies for the delayed reply. You'll need to make one small tweak for the workaround to work correctly in your example:

document.querySelector("#container").addEventListener("click", (e) => {
  e.stopPropagation();

  const fakeEvent = new CustomEvent("click", { detail: 1 });
  fakeEvent.composedPath = () => e.composedPath();
  window.dispatchEvent(fakeEvent);
});

We now pass the composed path in the fake event for any other components checking on an event's path to match their expected target (e.g., dropdown).

ruantao1989 commented 1 month ago

Thank you @jcfranco,

But after my test, it seems that the event from the normal DOM (not in the shadow DOM) does not have e.composedPath and e.composedPath();

How can I use the above workaround for this kind of event?