[Lens] Make the dimension flyout panel stay close on outside click #83059

dej611 · 2020-11-10T14:45:57Z

Summary

This PR addresses a bug identified in #82455 : if a popover inside the flyout was open, then clicking outside the panel would close first both popover and flyout, then quickly reopen the latter.

Why that happen? That's because on flyout panel close multiple updates happen at the same time (well, sequentially), and in particular multiple calls to the setActiveDimension state update were triggered: in particular one calls would refresh the state adding a tiny partial isNew: false property, while re-using the previous state.
Due to the multiple updates and React didn't have time to redraw the component, the activeDimension variable used was stale, causing the reopening: using the functional version of the setActiveDimension hook made it use the "last" version of the state in order to update.

Why that happened (2)? The main issue here was the double mechanism to toggle the popover, together with some state updates happening.
Removed the state update on close, the popover were still visible for a fraction of a second on the top left of the screen (easy to check with the Filter operation popover). While this seemed a minor issue it was only the effect of a bigger problem: the way the internal popover was closed was not correct, as it had to make sure to notify the parent component first, then its own state. On top of this beautiful dnd did not have the time to unmount in some scenarios showing a pretty big warning + error in development.

The fix has addressed in this PR only the popover close commands order, leaving the mount/unmount problem. A new issue on the EUI repo has been created to address this problem upstream.

The solution was to have the popover correctly check its state and close only via the correct call, and delay a bit the flyout panel closing in order to give time to the popover and beautiful dnd to unmount correctly. The 150 delay has been empirically measured on my machine and it was the smallest value by which the visual glitch was not visible.

Not really sure on how to test this (maybe functional?). Any suggestion is welcome.

Fixes: #82455

elasticmachine · 2020-11-11T14:20:59Z

Pinging @elastic/kibana-app (Team:KibanaApp)

flash1293 · 2020-11-11T16:22:50Z

I tested this PR and it fixes the issue, but why is it happening in the first place? Where are these setState calls coming from? They don't seem required and I would like to fix this problem at the root if possible.

I debugged a little and it seems like they are triggered by the useEffects hook like the problem with double fetch, maybe they are related?

dej611 · 2020-11-11T17:20:17Z

I suspected they are related at some level.
Managed to isolate the issue at the operation level. ~~Some dnd warnings are still shown in some scenarios, but the update on close should be gone.~~

dej611 · 2020-11-11T17:49:36Z

Note the latest change introduce a small delay on panel closing (150ms). While the number does not seem too big, the delay (even if small) is noticeable.

dej611 · 2020-11-12T10:23:03Z

@elasticmachine merge upstream

flash1293 · 2020-11-12T16:43:07Z

...k/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.tsx

+    setTimeout(() => {
+      handleClose();
+      setFocusTrapIsEnabled(false);
+    }, 150); // <= 150 has been chosen from empirical testing


I'm really concerned about this approach. As you said it will add a delay as well as not even work in all cases depending on the timing of things.

I think I wasn't really clear in my original review, I think it's fine to use the updater function setState approach to make sure the flyout stays closed if the additional setStates triggered on closing the popover are really necessary, but I was under the impression the state update happening in this scenario doesn't even make sense and we can just avoid it all together.

So, in summary

Let's please not introduce constant waiting times in the UI if avoidable

See if it's possible to not update the state on unmount like this

If we have to, use the updater function to make sure the flyout stays closed, but limit the cases where this is happening

I'll move the issue upstream and have EUI fix it at component level.

Created issue elastic/eui#4265 on EUI repo and will remove the code here.

dej611 · 2020-11-16T10:24:41Z

@elasticmachine merge upstream

flash1293

Tested in Chrome and Firefox, works as expected.

kibanamachine · 2020-11-16T15:23:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c79adaf

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	918.4KB	918.5KB	+91.0B

History

💚 Build #87939 succeeded afa67bc
💚 Build #87572 succeeded 42b0bc5
💚 Build #87557 succeeded c4cc2bf
💔 Build #87398 failed 2dae5e8
💚 Build #87086 succeeded bf5ac0f

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2020-11-18T16:24:40Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 83059 or prevent reminders by adding the backport:skip label.

…lastic#83059) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…ick (#83059) (#83669) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…ode-details * 'master' of github.com:elastic/kibana: Remove dependency of tests on strict SyntaxKind values (elastic#83440) [SecuritySolution] override timerange for prebuilt templates (elastic#82468) [Enterprise Search] Added a shouldShowActiveForSubroutes option (elastic#83338) [Lens] Make the dimension flyout panel stay close on outside click (elastic#83059) [Security Solution] Gracefully handle errors in detection rules install (elastic#83306) Fix advanced settings category sorting (elastic#83394)

🐛 Update the active dimension on close with last value in the state

bf5ac0f

dej611 added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.11.0 labels Nov 10, 2020

dej611 requested a review from mbondyra November 10, 2020 14:45

dej611 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Nov 11, 2020

dej611 marked this pull request as ready for review November 11, 2020 14:20

dej611 added 2 commits November 11, 2020 18:21

🐛 Rework the fix at the operation level

59ecc9c

🐛 Fixed the dnd warning issue

2dae5e8

♻️ Refactor to reuse shared function

c4cc2bf

dej611 requested a review from a team November 12, 2020 09:01

Merge branch 'master' into fix/82455

42b0bc5

flash1293 requested changes Nov 12, 2020

View reviewed changes

dej611 added 2 commits November 16, 2020 11:22

♻️ Revert flyout cose logic

33e776a

Merge branch 'fix/82455' of github.com:dej611/kibana into fix/82455

95fc727

Merge branch 'master' into fix/82455

afa67bc

flash1293 approved these changes Nov 16, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix/82455

c79adaf

dej611 merged commit e1500bf into elastic:master Nov 16, 2020

dej611 deleted the fix/82455 branch November 16, 2020 16:22

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 18, 2020

dej611 mentioned this pull request Nov 18, 2020

[7.x] [Lens] Make the dimension flyout panel stay close on outside click (#83059) #83669

Merged

cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Nov 18, 2020

[Lens] Make the dimension flyout panel stay close on outside click (e…

57f0664

…lastic#83059) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 19, 2020

mbondyra mentioned this pull request Dec 7, 2020

[Lens] Filters popover stays open when it should be closed #85134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Make the dimension flyout panel stay close on outside click #83059

[Lens] Make the dimension flyout panel stay close on outside click #83059

dej611 commented Nov 10, 2020 •

edited

Loading

elasticmachine commented Nov 11, 2020

flash1293 commented Nov 11, 2020 •

edited

Loading

dej611 commented Nov 11, 2020 •

edited

Loading

dej611 commented Nov 11, 2020

dej611 commented Nov 12, 2020

flash1293 Nov 12, 2020

dej611 Nov 12, 2020

dej611 Nov 16, 2020

dej611 commented Nov 16, 2020

flash1293 left a comment

kibanamachine commented Nov 16, 2020

kibanamachine commented Nov 18, 2020

[Lens] Make the dimension flyout panel stay close on outside click #83059

[Lens] Make the dimension flyout panel stay close on outside click #83059

Conversation

dej611 commented Nov 10, 2020 • edited Loading

Summary

elasticmachine commented Nov 11, 2020

flash1293 commented Nov 11, 2020 • edited Loading

dej611 commented Nov 11, 2020 • edited Loading

dej611 commented Nov 11, 2020

dej611 commented Nov 12, 2020

flash1293 Nov 12, 2020

Choose a reason for hiding this comment

dej611 Nov 12, 2020

Choose a reason for hiding this comment

dej611 Nov 16, 2020

Choose a reason for hiding this comment

dej611 commented Nov 16, 2020

flash1293 left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 16, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Nov 18, 2020

dej611 commented Nov 10, 2020 •

edited

Loading

flash1293 commented Nov 11, 2020 •

edited

Loading

dej611 commented Nov 11, 2020 •

edited

Loading