[Vega] Fix element sizing issues in fullscreen mode #194330

nickofthyme · 2024-09-27T17:05:21Z

Summary

Fixes #194011 where a Vega visualization does not respect the enclosing element dimensions.

Fixes #194861 where resizing vega vis panel fails to update to the latest panel dimensions causing scroll bars.

Details

This issue stems from changes in #181543 to remove jQuery, specifically these changes. The issue is that $(element).width() was replaced with element.getBoundingClientRect().width. In most cases these 2 values should be the same, see Determining the dimensions of elements, but here for some reason they return different values.

Zight.Recording.2024-09-27.at.12.03.43.PM.mp4

Notice the size of the chart is the same in both the normal and fullscreen modes, without scrollbars.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2024-09-27T17:05:23Z

Pinging @elastic/kibana-visualizations (Team:Visualizations)

- update tests to use client dimensions - covert test file to ts - update types for test file

markov00 · 2024-10-03T07:11:16Z

src/plugins/vis_types/vega/public/vega_view/vega_base_view.js

+    const width = Math.floor(Math.max(0, dimensions?.width ?? this._container.clientWidth));
+    const height = Math.floor(Math.max(0, dimensions?.height ?? this._container.clientHeight));


I played a bit with the PR but I see the same issue (scrollbars visible) if you just resize the panel. The PR fixes when switching to full screen, but it doesn't work for the panel resize.
I'm not sure if that was also the previous behaviour

Yeah, I've seen that before too where we are throttling the size update. I'll check with older versions and see if it's the same there.

Ah, that's the reason? throttling? if so we just need to just to use a debounce strategy instead of throttling.

Ok I switched the resize to debounce in 0b2d976.

Throttle

Debounce

Removing debounce fixes this...

src/plugins/vis_types/vega/public/vega_view/vega_base_view.js

nickofthyme · 2024-10-03T17:15:48Z

src/plugins/vis_types/vega/public/components/vega_vis_component.tsx

Another issue I've seen with both throttle and debounce, is when the panel is sized very quickly. It basically lock in the dimensions before the latest update.

It's not a huge issue but it appears most visualizations are no longer debouncing resize or very little, including Lens. Should we remove the throttle/debounce altogether in vega?

Also the debounce prevents re-rendering on long slow resizing of the panel.

It's not a huge issue but it appears most visualizations are no longer debouncing resize or very little, including Lens. Should we remove the throttle/debounce altogether in vega?

I believe we can remove it, resizing is not an operation that a user does continuously so it is safe even if it's triggered quickly

Removed in 6965c17

kibana-ci · 2024-10-03T18:37:23Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] Jest Tests #1 / VegaVisualizations VegaVisualization - basics should show vegalite graph and update on resize (may fail in dev env)
[job] [logs] Jest Tests #1 / VegaVisualizations VegaVisualization - basics should show vegalite graph and update on resize (may fail in dev env)

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
`visTypeVega`	1.8MB	1.8MB	-54.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`visTypeVega`	6	7	+1

Total ESLint disabled count

id	before	after	diff
`visTypeVega`	6	7	+1

History

💛 Build #238108 was flaky 440f6b6
💚 Build #237845 succeeded 3267e42
💔 Build #237827 failed 02e283d
💔 Build #237765 failed 6c81de7

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

elasticmachine · 2024-10-07T20:03:05Z

💚 Build Succeeded

Buildkite Build
Commit: 440dcd9

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
`visTypeVega`	1.8MB	1.8MB	-119.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`visTypeVega`	6	7	+1

Total ESLint disabled count

id	before	after	diff
`visTypeVega`	6	7	+1

kibanamachine · 2024-10-07T22:20:44Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11224911060

- Fixes elastic#194011 where a Vega visualization does not respect the enclosing element dimensions. - Fixes elastic#194861 where resizing Vega visualization panel fails to update to the latest panel dimensions causing scroll bars. (cherry picked from commit 55c2fd7)

kibanamachine · 2024-10-07T22:25:14Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…195328) # Backport This will backport the following commits from `main` to `8.x`: - [[Vega] Fix element sizing issues in fullscreen mode (#194330)](#194330)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Nick Partridge <[email protected]>

Fix vega sizing for fullscreen mode

6c81de7

nickofthyme added release_note:fix Feature:Vega Vega visualizations Team:Visualizations Visualization editors, elastic-charts and infrastructure backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Sep 27, 2024

nickofthyme requested review from dej611 and markov00 September 27, 2024 17:05

nickofthyme requested a review from a team as a code owner September 27, 2024 17:05

nickofthyme mentioned this pull request Sep 27, 2024

[Vega] Visualization parent dimensions are incorrect #194011

Closed

nickofthyme added 3 commits September 27, 2024 22:02

test: update broken tests and types

02e283d

- update tests to use client dimensions - covert test file to ts - update types for test file

fix: type errors

3267e42

Merge branch 'main' into fix-vega-fullscreen-sizing

440f6b6

markov00 reviewed Oct 3, 2024

View reviewed changes

src/plugins/vis_types/vega/public/vega_view/vega_base_view.js Show resolved Hide resolved

nickofthyme added 4 commits October 3, 2024 11:15

Merge branch 'main' into fix-vega-fullscreen-sizing

fabb566

fix resizing issue elastic#194861

81a7703

refactor vis resizing to use debounce instead of throttle

0b2d976

fix initial render dimensions

38aa19f

nickofthyme commented Oct 3, 2024

View reviewed changes

nickofthyme requested a review from markov00 October 3, 2024 17:22

markov00 approved these changes Oct 4, 2024

View reviewed changes

nickofthyme added 3 commits October 7, 2024 12:41

Merge branch 'main' into fix-vega-fullscreen-sizing

ef0b6af

remove debounce logic

6965c17

update jest snapshots due to 1px diff

440dcd9

nickofthyme merged commit 55c2fd7 into elastic:main Oct 7, 2024
30 checks passed

nickofthyme deleted the fix-vega-fullscreen-sizing branch October 7, 2024 22:20

nickofthyme changed the title ~~[Vega] Fix element sizing for fullscreen mode~~ [Vega] Fix element sizing issues in fullscreen mode Oct 7, 2024

kibanamachine added the v9.0.0 label Oct 7, 2024

kibanamachine mentioned this pull request Oct 7, 2024

[8.x] [Vega] Fix element sizing issues in fullscreen mode (#194330) #195328

Merged

nickofthyme added the v8.16.0 label Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vega] Fix element sizing issues in fullscreen mode #194330

[Vega] Fix element sizing issues in fullscreen mode #194330

nickofthyme commented Sep 27, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Sep 27, 2024

markov00 Oct 3, 2024

nickofthyme Oct 3, 2024

markov00 Oct 3, 2024

nickofthyme Oct 3, 2024 •

edited

Loading

nickofthyme Oct 7, 2024

nickofthyme Oct 3, 2024

nickofthyme Oct 3, 2024

nickofthyme Oct 3, 2024

markov00 Oct 4, 2024 •

edited

Loading

nickofthyme Oct 7, 2024

kibana-ci commented Oct 3, 2024 •

edited

Loading

ESLint disabled line counts

Total ESLint disabled count

elasticmachine commented Oct 7, 2024

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Oct 7, 2024

kibanamachine commented Oct 7, 2024

		const width = Math.floor(Math.max(0, dimensions?.width ?? this._container.clientWidth));
		const height = Math.floor(Math.max(0, dimensions?.height ?? this._container.clientHeight));

[Vega] Fix element sizing issues in fullscreen mode #194330

[Vega] Fix element sizing issues in fullscreen mode #194330

Conversation

nickofthyme commented Sep 27, 2024 • edited by kibanamachine Loading

Summary

Details

Checklist

elasticmachine commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickofthyme Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Throttle

Debounce

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Oct 3, 2024 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

elasticmachine commented Oct 7, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Oct 7, 2024

kibanamachine commented Oct 7, 2024

💚 All backports created successfully

Questions ?

nickofthyme commented Sep 27, 2024 •

edited by kibanamachine

Loading

nickofthyme Oct 3, 2024 •

edited

Loading

markov00 Oct 4, 2024 •

edited

Loading

kibana-ci commented Oct 3, 2024 •

edited

Loading