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

[Discover] can't enter ES|QL editor when column menu open #177756

Closed
drewdaemon opened this issue Feb 23, 2024 · 11 comments · Fixed by #178622
Closed

[Discover] can't enter ES|QL editor when column menu open #177756

drewdaemon opened this issue Feb 23, 2024 · 11 comments · Fixed by #178622
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:ES|QL ES|QL related features in Kibana Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)

Comments

@drewdaemon
Copy link
Contributor

Kibana version:
8.13.0

Screen.Recording.2024-02-23.at.12.48.07.PM.mov
@drewdaemon drewdaemon added bug Fixes for quality problems that affect the customer experience Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. Feature:ES|QL ES|QL related features in Kibana labels Feb 23, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@shayfeld
Copy link

Same problem in version 8.12 with ES|QL reference.

@kertal
Copy link
Member

kertal commented Mar 4, 2024

Confirmed but I think this is just surfacing in Discover, might be a problem how EuiDataGrid deals with the focus state and I think it's because the ESQL editor is having it's own React render tree, because else the column menu would close when clicking somewhere else? Can you confirm @elastic/eui-team ? Is there something we can do here?

@kertal kertal added the EUI label Mar 4, 2024
@kertal kertal self-assigned this Mar 4, 2024
@cee-chen
Copy link
Contributor

cee-chen commented Mar 4, 2024

might be a problem how EuiDataGrid deals with the focus state

To clarify, if it is an issue with EUI's underlying focus behavior, it wouldn't be specific to the EuiDataGrid component - this would be general to either EuiPopover or EuiFocusTrap.

I think it's because the ESQL editor is having it's own React render tree

The focus trap library we use (react-focus-on) shouldn't care about separate react render trees; it uses vanilla JS window event listeners if I recall correctly. The bigger concern would be if:

  1. The esql editor is in an iframe
  2. If the esql editor has its own click event listener that prevents events from bubbling or propagating upwards

2 is my suspicion here personally.

@davismcphee
Copy link
Contributor

I also suspect @cee-chen is right and the editor is probably swallowing the clicks. At least that's where I'd start investigating personally.

@kertal
Copy link
Member

kertal commented Mar 5, 2024

I think it's because of Monaco, because I've been testing with out push flyout / JSON editor, and it's the same behavior there, it's also Monaco in user there .Is there something we can do with "@kbn/code-editor", @elastic/appex-sharedux , thx!

@kertal kertal added Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) and removed EUI labels Mar 5, 2024
@Dosant
Copy link
Contributor

Dosant commented Mar 5, 2024

it's also Monaco in user there .Is there something we can do with "@kbn/code-editor", @elastic/appex-sharedux , thx!

No ideas. Looks like mousedown handler is deep inside the Monaco editor to set the focus in: http://localhost:5601/exz/XXXXXXXXXXXX/bundles/kbn-ui-shared-deps-src/kbn-ui-shared-deps-src/node_modules/monaco-editor/esm/vs/editor/browser/controller/mouseHandler.js

And there is the preventDefault() on that mouse down event.

Then when it bubbles up to the react-focus-on inside EUI, clickOutside isn't fired because the event has default prevented: https://github.com/theKashey/react-focus-on/blob/bf292697ef522aaa5b25d663bd39941e69f9b930/src/Effect.tsx#L48

And then, I think, the focus trap reverts the focus from the editor.

@cee-chen
Copy link
Contributor

cee-chen commented Mar 6, 2024

If editing monaco's mouse handler directly isn't possible, one not-super-great workaround option could be overlaying an invisible div over the monaco editor whenever an EUI focus trap is added on the page, which then would block monaco's mouse handler and trigger the focus trap being removed. 😬

@tsullivan
Copy link
Member

I’m wondering if we can add our own mouse handler (on top of monaco's) that will re-broadcast the click event to the window... That would hopefully force the column menu popup to close

@petrklapka
Copy link
Member

From Triage on March 11th: Eyo will take a peek to see what can be done and will estimate LOE for a non hacky solution. We'll re-triage based on his input.

@kertal kertal removed the Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. label Mar 12, 2024
@kertal kertal removed their assignment Mar 12, 2024
@eokoneyo
Copy link
Contributor

eokoneyo commented Mar 14, 2024

I looked into this, and opened a PR here that pretty much implements @tsullivan's suggestion to rebroadcast the event, the approach I took makes it such that every mousedown event that happens within the editor with it's default prevented gets rebroadcasted without the default prevented with the caveat that the target is the wrapper placed around the code editor. This way external listeners are satisfied and the default behaviour for the event propagated by monaco is preserved.

eokoneyo added a commit that referenced this issue Mar 19, 2024
## Summary


Closes #177756

Introduces a wrapper that will ~trap~ listen on `mousedown` events from
the monaco editor and rebroadcast this event type so that when it's
integrated, any component that needs interaction events still gets
events related to the monaco but with the wrapper as the event target.

The advantage of this approach is we keep the default behaviour for
events propagated for monaco editor as is, also applying style of
[`display:contents`](https://www.w3.org/TR/css-display-3/#box-generation)
ensures this extra wrapper element does not modify the existing layout.

<!-- 
### Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to
identify risks that should be tested prior to the change/feature
release.

When forming the risk matrix, consider some of the following examples
and how they may potentially impact the change:

| Risk | Probability | Severity | Mitigation/Notes |

|---------------------------|-------------|----------|-------------------------|
| Multiple Spaces&mdash;unexpected behavior in non-default Kibana Space.
| Low | High | Integration tests will verify that all features are still
supported in non-default Kibana Space and when user switches between
spaces. |
| Multiple nodes&mdash;Elasticsearch polling might have race conditions
when multiple Kibana nodes are polling for the same tasks. | High | Low
| Tasks are idempotent, so executing them multiple times will not result
in logical error, but will degrade performance. To test for this case we
add plenty of unit tests around this logic and document manual testing
procedure. |
| Code should gracefully handle cases when feature X or plugin Y are
disabled. | Medium | High | Unit tests will verify that any feature flag
or plugin combination still results in our service operational. |
| [See more potential risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:ES|QL ES|QL related features in Kibana Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants