Repository: streamlit/streamlit
Pull request: Fix tabs switching couse scrolling up
<!--
Before contributing (PLEASE READ!)

⚠️ If your contribution is more than a few lines of code, then prior to starting to code on it please post in the issue saying you want to volunteer, then wait for a positive response. And if there is no issue for it yet, create it first.

This helps make sure:

  1. Two people aren't working on the same thing
  2. This is something Streamlit's maintainers believe should be implemented/fixed
  3. Any API, UI, or deeper architectural changes that need to be implemented have been fully thought through by Streamlit's maintainers
  4. Your time is well spent!

More information in our wiki: https://github.com/streamlit/streamlit/wiki/Contributing
-->

## 📚 Context

_Please describe the project or issue background here_

- What kind of change does this PR introduce?

  - [x] Bugfix
  - [ ] Feature
  - [ ] Refactoring
  - [ ] Other, please describe:

## 🧠 Description of Changes

It's a bug fix for [Issue-#5069](https://github.com/streamlit/streamlit/issues/5069). When using an HTML Video element with tabs it's initial height it's set to 0 and stays like this, which couses unexpected scrolling up on tab change.

This PR fixes it by setting up HTML Video element height on loadedmetadata event inside `useEffect` method

- _Add bullet points summarizing your changes here_

  - [ ] This is a breaking API change
  - [x] This is a visible (user-facing) change

**Revised:**

_Insert screenshot of your updated UI/code here_

**Current:**

_Insert screenshot of existing UI/code here_

## 🧪 Testing Done

- [ ] Screenshots included
- [ ] Added/Updated unit tests
- [ ] Added/Updated e2e tests

## 🌐 References

_Does this depend on other work, documents, or tickets?_

- **Issue**: It's fixes part of [Issue-#5069](https://github.com/streamlit/streamlit/issues/5069). It is expected that other elements like `st.image` will couse similar issue, when their initial height is set to 0

---

**Contribution License Agreement**

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.


Problem statement:
tabs switching cause scrolling up
### Summary

app scrolls up when I have tabs after some widgets and switch them


### Steps to reproduce

Code snippet:

```
import streamlit as st

for i in range(20):
    st.markdown("dummy text")


tab1, tab2 = st.tabs(["tab1", "tab2"])

with tab1:
    st.video(video_path)

with tab2:
    st.video(video_path)

```

steps :
1. scroll down to the videos
2. switching tabs

**Expected behavior:**

tabs should be switched and focus stay on video

**Actual behavior:**

it scrolling up and I have to scroll down again

### Debug info

- Streamlit version:  1.11.0
- Python version: 3.8.11

Hints from issue discussion:
@RRaphaell Thanks for reporting this issue! I was able to reproduce it and have deployed a demo app [here](https://lukasmasuch-st-playground-issuesgh-5069app-tmwuwl.streamlitapp.com/). The underlying issue is that when changing tabs, the height of the actual tab content is not correct for a split second. This influences the full height of the app -> which in turn influences the maximum possible scroll position -> and this causes the unexpected up scroll. This does not happen if there is enough content underneath the tab (try out in the demo app), since in that case the scroll position will not be larger than the maximum possible. 

To find a solution here requires a bit more investigation. I tried out the `renderAll` property of the baseWeb component, but this only fixes it for a second tab change. 
thanks @LukasMasuch for your time. you are right but when I tested it by adding more content below it still happens if you scroll down to the new content and then switch tabs
+1 .I have the same issue.
@LukasMasuch is correct about the source of the issue, when I've setup fixed height of the `<video>` React component it fixed the issue, however I don't think we want to have the fixed `height` for every video. I'd like to set fixed `height` on first `componentDidMount`, what do you think about such proposition?
@sfc-gh-tszerszen Thanks for investigating this! I think there are also a few other elements without a fixed height. For example, our built-in chart commands:

```python
import streamlit as st
import pandas as pd
import numpy as np

chart_data = pd.DataFrame(np.random.randn(20, 3), columns=["a", "b", "c"])

for i in range(20):
    st.markdown("dummy text")


tab1, tab2 = st.tabs(["tab1", "tab2"])

with tab1:
    st.line_chart(chart_data)

with tab2:
    st.line_chart(chart_data)
```

So, we might need to figure out which elements are affected and apply the same fix. 

>  I'd like to set fixed height on first componentDidMount, what do you think about such proposition?


Would be great if we can fix it this way, but this needs some testing if there are any negative side effects. E.g. in some situations, components are reused for different content, so just fixing the height on `componentDidMount` might lead to wrong height in some situations.

Commit message: update E2E Cypress st_tabs.spec.js to march renderAll property

Patch to review:
diff --git a/e2e/specs/st_tabs.spec.js b/e2e/specs/st_tabs.spec.js
index f747a79513fa..24a444e9804a 100644
--- a/e2e/specs/st_tabs.spec.js
+++ b/e2e/specs/st_tabs.spec.js
@@ -36,11 +36,18 @@ describe("st.tabs", () => {
       2
     );
 
-    cy.get("[data-testid='stSidebar'] .stTabs")
+    cy.get("[data-testid='stSidebar'] .stTabs .st-cx")
       .first()
       .within(() => {
         cy.get(".stMarkdown").should("have.text", "I am in the sidebar");
       });
+
+    // text from every tab should be here because renderAll property is set to true
+    cy.get("[data-testid='stSidebar'] .stTabs")
+      .first()
+      .within(() => {
+        cy.get(".stMarkdown").should("have.text", "I am in the sidebarI'm also in the sidebar");
+      });
   });
 
   it("changes rendered content on tab selection", () => {
diff --git a/frontend/src/components/elements/Tabs/Tabs.tsx b/frontend/src/components/elements/Tabs/Tabs.tsx
index 7439316f7b58..9f69d74d5bc5 100644
--- a/frontend/src/components/elements/Tabs/Tabs.tsx
+++ b/frontend/src/components/elements/Tabs/Tabs.tsx
@@ -62,6 +62,10 @@ function Tabs(props: Props): ReactElement {
         onChange={({ activeKey }) => {
           setActiveKey(activeKey)
         }}
+        /* renderAll on UITabs should always be set to true to avoid scrolling issue
+           https://github.com/streamlit/streamlit/issues/5069
+         */
+        renderAll={true}
         disabled={widgetsDisabled}
         overrides={{
           TabHighlight: {
diff --git a/frontend/src/components/elements/Video/Video.tsx b/frontend/src/components/elements/Video/Video.tsx
index 5b3813d41e52..e5ece8c10f77 100644
--- a/frontend/src/components/elements/Video/Video.tsx
+++ b/frontend/src/components/elements/Video/Video.tsx
@@ -36,19 +36,26 @@ export default function Video({ element, width }: VideoProps): ReactElement {
   useEffect(() => {
     const videoNode = videoRef.current
 
-    const setStartTime: () => void = () => {
+    const onLoadedMetadata: () => void = () => {
       if (videoNode) {
+        // setStartTime
         videoNode.currentTime = element.startTime
+
+        /* height of HTML video element needs to be set to avoid scrolling issue: https://github.com/streamlit/streamlit/issues/5069
+           initially HTML video has height 0 and when tabs are switched page auto scrolls up
+           because of "lack of content" on the page, setting height of HTML video element fixes it
+         */
+        videoNode.height = videoNode.videoHeight
       }
     }
 
     if (videoNode) {
-      videoNode.addEventListener("loadedmetadata", setStartTime)
+      videoNode.addEventListener("loadedmetadata", onLoadedMetadata)
     }
 
     return () => {
       if (videoNode) {
-        videoNode.removeEventListener("loadedmetadata", setStartTime)
+        videoNode.removeEventListener("loadedmetadata", onLoadedMetadata)
       }
     }
   }, [element])


Code review comments:
diff_hunk: @@ -36,11 +36,18 @@ describe("st.tabs", () => {
       2
     );
 
-    cy.get("[data-testid='stSidebar'] .stTabs")
+    cy.get("[data-testid='stSidebar'] .stTabs .st-cx")
path: e2e/specs/st_tabs.spec.js
text: @user1:
I'm not sure if we can or should use `.st-cx` since this is probably an internally generated class. I think it is fine to just select the first `.stMarkdown` within the `.stTabs` component: 

```javascript
cy.get("[data-testid='stSidebar'] .stTabs")
.first()
      .within(() => {
        cy.get(".stMarkdown").first().should("have.text", "I am in the sidebar");
      });
```