[Core Rendering Styles] Fix bug with kibanaFullBodyHeight calculation #193798

tsullivan · 2024-09-23T21:04:58Z

A variable for the page height was not being calculated correctly when the Kibana chrome is invisible. This PR fixes the issue by adding px as a unit to a variable used as input in the calculation.

Unblocks: #193647

Testing

It will help to test within the #193647 PR branch. The following screenshots show the before and after effects of having this fix:

before

after

tsullivan · 2024-09-23T23:17:58Z

src/core/public/styles/rendering/_base.scss

@@ -67,7 +67,7 @@
 }

 .kbnBody--chromeHidden {
-  --euiFixedHeadersOffset: 0;
+  --euiFixedHeadersOffset: 0px;


The problem was: this calculation, which is used in Discover, was coming up wrong:

@media only screen and (min-width: 1200px) { .dscPage { // --kbnAppHeadersOffset is undefined // --euiFixedHeadersOffset is `0` height: calc(100vh - var(--kbnAppHeadersOffset, var(--euiFixedHeadersOffset, 0))); } }

@cee-chen found the reason why in a spec: https://drafts.csswg.org/css-values-3/#calc-type-checking

Note: Because s are always interpreted as s or s, "unitless 0" s aren’t supported in calc(). That is, width: calc(0 + 5px); is invalid, even though both width: 0; and width: 5px; are valid.

that's very strange, it seems that this change now causes a functional test error here:

kibana/test/functional/services/visualizations/elastic_chart.ts

Lines 43 to 50 in 2b344d2

public async waitForRenderComplete(dataTestSubj?: string, timeout?: number) {

const chart = await this.getChart(dataTestSubj, timeout);

const rendered = await chart.findAllByCssSelector(

'.echChartStatus[data-ech-render-complete=true]',

timeout

);

expect(rendered.length).to.equal(1);

}

I've checked the html of the failure

And so it failes because it seems data-ech-render-complete=false doesn't turn to true

dear @elastic/kibana-visualizations , do you have an idea what's happening here?

Interesting. Does the test fail with the attribute still set to false, but the chart rendered? Or the panel is still in a loading phase?

For me it looks like it was rendered?

Interesting, looks like the charts now starts a redraw due to the height change.
Is not the first time I see this, where the discover heatmap is rendered 2 times because of some jumps in the layout rendering of the table.

that's is a possible workaround. I believe there is still something that needs to be anyway solved as it doesn't seems to work correctly.
I've checked locally and I can see the following:
before this PR, when switching to full screen the chart height goes to zero, and back to its height when toggling the fullscreen off. With this PR the chart remain the same size but is hidden behind the position:fixed table. This, for some reason I don't understand cause a trigger in the ResizeObserver we are using to compute the chart size causing it to turn the rendered off, but doesn't trigger it again. I can investigate a bit more if you want

ok, the way I see it, yes, it's related to the data grid going to full screen, and with the changes of this PR, as you said, the chart remains same size. For me it seems like we could try to remove the await this.testSubjects.click('dataGridFullScreenButton'); , check for flakiness, and if there's no quick fix for the ResizeObserver not being triggered again (which shold be investigated I think), open an issue to track this

@kertal @tsullivan not sure if this is a bug in Charts or in ResizeObserver (that triggers a size change even if there is no size change in the chart), but I've created a PR to fix this in charts.
I will merge the fix in charts, and push an upgraded charts version in Kibana today if possible

elastic/elastic-charts#2534

Awesome 🎉 thx!

Sorry for the late work on that, we finally manage to fix the bug in charts, and we are now waiting for a CI to pass before merging the charts dependency update in Kibana #194794
Please do a bootstrap locally if you want to see and test the changes

cee-chen

Thanks so much for identifying the fix Tim!

Dosant

Nice!

Dosant · 2024-09-24T07:09:32Z

@elasticmachine merge upstream

tsullivan · 2024-09-24T17:19:37Z

https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7007

kibanamachine · 2024-09-24T17:55:25Z

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7007

[❌] test/functional/apps/discover/group1/config.ts: 0/50 tests passed.

see run history

kibana-ci · 2024-10-04T14:32:13Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 3311c08

Failed CI Steps

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	455.7KB	455.7KB	+4.0B

History

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

tsullivan · 2024-10-04T17:25:19Z

https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7092

kibanamachine · 2024-10-04T18:30:48Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7092

[✅] test/functional/apps/discover/group1/config.ts: 50/50 tests passed.

see run history

kibanamachine · 2024-10-04T21:01:15Z

Starting backport for target branches: 8.x

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

tsullivan · 2024-10-04T21:01:35Z

Thanks so much @markov00 for handling the chart issue!

…elastic#193798) A variable for the page height was not being calculated correctly when the Kibana chrome is invisible. This PR fixes the issue by adding `px` as a unit to a variable used as input in the calculation. Unblocks: elastic#193647 ## Testing It will help to test within the elastic#193647 PR branch. The following screenshots show the before and after effects of having this fix: **before** ![ghjkgjghgkhjgjgkjg-1](https://github.com/user-attachments/assets/d684bc77-dcc0-40ef-9271-8ae198582eea) **after** ![ghjkgjghgkhjgjgkjg-2](https://github.com/user-attachments/assets/5198cc43-b721-4ce8-92f0-0b2bac9f5eb5) Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Marco Vettorello <[email protected]> (cherry picked from commit 1df458d)

kibanamachine · 2024-10-04T21:05:42Z

💚 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

…lation (#193798) (#195146) # Backport This will backport the following commits from `main` to `8.x`: - [[Core Rendering Styles] Fix bug with kibanaFullBodyHeight calculation (#193798)](#193798)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Tim Sullivan <[email protected]>

…elastic#193798) A variable for the page height was not being calculated correctly when the Kibana chrome is invisible. This PR fixes the issue by adding `px` as a unit to a variable used as input in the calculation. Unblocks: elastic#193647 ## Testing It will help to test within the elastic#193647 PR branch. The following screenshots show the before and after effects of having this fix: **before** ![ghjkgjghgkhjgjgkjg-1](https://github.com/user-attachments/assets/d684bc77-dcc0-40ef-9271-8ae198582eea) **after** ![ghjkgjghgkhjgjgkjg-2](https://github.com/user-attachments/assets/5198cc43-b721-4ce8-92f0-0b2bac9f5eb5) Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Marco Vettorello <[email protected]>

tsullivan requested a review from a team as a code owner September 23, 2024 21:04

tsullivan requested review from Dosant and cee-chen September 23, 2024 21:05

tsullivan added release_note:skip Skip the PR/issue when compiling release notes v8.16.0 backport:version Backport to applied version labels labels Sep 23, 2024

tsullivan marked this pull request as draft September 23, 2024 22:09

tsullivan force-pushed the core-styles/full-height-calculation branch from 0bf86aa to 08ee079 Compare September 23, 2024 22:38

tsullivan commented Sep 23, 2024

View reviewed changes

Fix bug with kibanaFullBodyHeight calculation

1c56a35

tsullivan force-pushed the core-styles/full-height-calculation branch from 08ee079 to 1c56a35 Compare September 23, 2024 23:23

tsullivan marked this pull request as ready for review September 23, 2024 23:25

elastic deleted a comment from kibana-ci Sep 23, 2024

cee-chen approved these changes Sep 23, 2024

View reviewed changes

Dosant approved these changes Sep 24, 2024

View reviewed changes

elasticmachine and others added 2 commits September 24, 2024 09:09

Merge branch 'main' into core-styles/full-height-calculation

70b9529

Merge branch 'main' into core-styles/full-height-calculation

156cb15

MichaelMarcialis approved these changes Sep 24, 2024

View reviewed changes

tsullivan added 3 commits September 24, 2024 13:45

Merge branch 'main' into core-styles/full-height-calculation

2cfd4de

Merge branch 'main' into core-styles/full-height-calculation

b744b54

Merge branch 'main' into core-styles/full-height-calculation

1ef07b4

markov00 mentioned this pull request Sep 27, 2024

fix(ChartStatus): render complete if same parent size is dispatched elastic/elastic-charts#2534

Merged

tsullivan and others added 2 commits October 1, 2024 12:22

Merge branch 'main' into core-styles/full-height-calculation

53c9b5c

Merge branch 'main' into core-styles/full-height-calculation

3311c08

tsullivan merged commit 1df458d into elastic:main Oct 4, 2024
24 checks passed

tsullivan deleted the core-styles/full-height-calculation branch October 4, 2024 21:01

kibanamachine added the v9.0.0 label Oct 4, 2024

kibanamachine mentioned this pull request Oct 4, 2024

[8.x] [Core Rendering Styles] Fix bug with kibanaFullBodyHeight calculation (#193798) #195146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core Rendering Styles] Fix bug with kibanaFullBodyHeight calculation #193798

[Core Rendering Styles] Fix bug with kibanaFullBodyHeight calculation #193798

tsullivan commented Sep 23, 2024 •

edited by kibanamachine

Loading

tsullivan Sep 23, 2024 •

edited

Loading

kertal Sep 26, 2024

dej611 Sep 26, 2024

kertal Sep 26, 2024

markov00 Sep 26, 2024

markov00 Sep 27, 2024

kertal Sep 27, 2024

markov00 Sep 27, 2024

kertal Sep 27, 2024

markov00 Oct 3, 2024

cee-chen left a comment

Dosant left a comment

Dosant commented Sep 24, 2024

tsullivan commented Sep 24, 2024

kibanamachine commented Sep 24, 2024

kibana-ci commented Oct 4, 2024

tsullivan commented Oct 4, 2024

kibanamachine commented Oct 4, 2024

kibanamachine commented Oct 4, 2024

tsullivan commented Oct 4, 2024

kibanamachine commented Oct 4, 2024

	public async waitForRenderComplete(dataTestSubj?: string, timeout?: number) {
	const chart = await this.getChart(dataTestSubj, timeout);
	const rendered = await chart.findAllByCssSelector(
	'.echChartStatus[data-ech-render-complete=true]',
	timeout
	);
	expect(rendered.length).to.equal(1);
	}

[Core Rendering Styles] Fix bug with kibanaFullBodyHeight calculation #193798

[Core Rendering Styles] Fix bug with kibanaFullBodyHeight calculation #193798

Conversation

tsullivan commented Sep 23, 2024 • edited by kibanamachine Loading

Testing

tsullivan Sep 23, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

Dosant left a comment

Choose a reason for hiding this comment

Dosant commented Sep 24, 2024

tsullivan commented Sep 24, 2024

kibanamachine commented Sep 24, 2024

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7007

kibana-ci commented Oct 4, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Page load bundle

History

tsullivan commented Oct 4, 2024

kibanamachine commented Oct 4, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7092

kibanamachine commented Oct 4, 2024

tsullivan commented Oct 4, 2024

kibanamachine commented Oct 4, 2024

💚 All backports created successfully

Questions ?

tsullivan commented Sep 23, 2024 •

edited by kibanamachine

Loading

tsullivan Sep 23, 2024 •

edited

Loading