Merge branch 'develop' into tim/HotkeyTests

* develop:
  ReportContext constructs its own widget_ids_this_run (streamlit#2669)
  Add types to report_thread.py (streamlit#2665)
  Take scrollbars into account when computing Dataframe dimensions (streamlit#2622)
  Revert "Revert "Revert "Add anchors to Markdown headers (streamlit#2513)""" (streamlit#2664)
  ♻️ Remove "_proto" from "data_frame_proto.py" (streamlit#2640)

e2e/scripts/st_header.py

@@ -15,4 +15,3 @@
 import streamlit as st
 
 st.header("This header is awesome!")
-st.header("This header is awesome too!", anchor="awesome-header")

e2e/scripts/st_markdown.py

@@ -35,7 +35,3 @@
 $$
 """
 )
-
-st.markdown("# Some header 1")
-st.markdown("## Some header 2")
-st.markdown("### Some header 3")

e2e/scripts/st_subheader.py

@@ -15,4 +15,3 @@
 import streamlit as st
 
 st.subheader("This subheader is awesome!")
-st.subheader("This subheader is awesome too!", anchor="awesome-subheader")

e2e/scripts/st_title.py

@@ -15,4 +15,3 @@
 import streamlit as st
 
 st.title("This title is awesome!")
-st.title("This title is awesome too!", anchor="awesome-title")

e2e/specs/st_header.spec.js

@@ -20,21 +20,10 @@ describe("st.header", () => {
     cy.visit("http://localhost:3000/");
   });
 
-  it("displays correct number of elements", () => {
-    cy.get(".element-container .stMarkdown h2").should("have.length", 2);
-  });
-
   it("displays a header", () => {
-    cy.get(".element-container .stMarkdown h2").then(els => {
-      expect(els[0].textContent).to.eq("This header is awesome!");
-      expect(els[1].textContent).to.eq("This header is awesome too!");
-    });
-  });
-
-  it("displays headers with anchors", () => {
-    cy.get(".element-container .stMarkdown h2").then(els => {
-      cy.wrap(els[0]).should("have.attr", "id", "this-header-is-awesome");
-      cy.wrap(els[1]).should("have.attr", "id", "awesome-header");
-    });
+    cy.get(".element-container .stMarkdown h2").should(
+      "contain",
+      "This header is awesome!"
+    );
   });
 });

e2e/specs/st_markdown.spec.js

@@ -20,11 +20,8 @@ describe("st.markdown", () => {
     cy.visit("http://localhost:3000/");
   });
 
-  it("displays correct number of elements", () => {
-    cy.get(".element-container .stMarkdown").should("have.length", 11);
-  });
-
   it("displays markdown", () => {
+    cy.get(".element-container .stMarkdown").should("have.length", 8);
     cy.get(".element-container .stMarkdown").then(els => {
       expect(els[0].textContent).to.eq("This markdown is awesome! 😎");
       expect(els[1].textContent).to.eq("This <b>HTML tag</b> is escaped!");
@@ -36,30 +33,14 @@ describe("st.markdown", () => {
       expect(els[7].textContent).to.eq(
         "ax2+bx+c=0ax^2 + bx + c = 0ax2+bx+c=0"
       );
-      expect(els[8].textContent).to.eq("Some header 1");
-      expect(els[9].textContent).to.eq("Some header 2");
-      expect(els[10].textContent).to.eq("Some header 3");
 
       cy.wrap(els[3])
         .find("a")
         .should("not.exist");
+
       cy.wrap(els[4])
         .find("a")
         .should("have.attr", "href");
     });
   });
-
-  it("displays headers with anchors", () => {
-    cy.get(".element-container .stMarkdown").then(els => {
-      cy.wrap(els[8])
-        .find("h1")
-        .should("have.attr", "id", "some-header-1");
-      cy.wrap(els[9])
-        .find("h2")
-        .should("have.attr", "id", "some-header-2");
-      cy.wrap(els[10])
-        .find("h3")
-        .should("have.attr", "id", "some-header-3");
-    });
-  });
 });

e2e/specs/st_subheader.spec.js

@@ -20,21 +20,10 @@ describe("st.subheader", () => {
     cy.visit("http://localhost:3000/");
   });
 
-  it("displays correct number of elements", () => {
-    cy.get(".element-container .stMarkdown h3").should("have.length", 2);
-  });
-
   it("displays a subheader", () => {
-    cy.get(".element-container .stMarkdown h3").then(els => {
-      expect(els[0].textContent).to.eq("This subheader is awesome!");
-      expect(els[1].textContent).to.eq("This subheader is awesome too!");
-    });
-  });
-
-  it("displays subheaders with anchors", () => {
-    cy.get(".element-container .stMarkdown h3").then(els => {
-      cy.wrap(els[0]).should("have.attr", "id", "this-subheader-is-awesome");
-      cy.wrap(els[1]).should("have.attr", "id", "awesome-subheader");
-    });
+    cy.get(".element-container .stMarkdown h3").should(
+      "contain",
+      "This subheader is awesome!"
+    );
   });
 });

e2e/specs/st_title.spec.js

@@ -20,21 +20,10 @@ describe("st.title", () => {
     cy.visit("http://localhost:3000/");
   });
 
-  it("displays correct number of elements", () => {
-    cy.get(".element-container .stMarkdown h1").should("have.length", 2);
-  });
-
   it("displays a title", () => {
-    cy.get(".element-container .stMarkdown h1").then(els => {
-      expect(els[0].textContent).to.eq("This title is awesome!");
-      expect(els[1].textContent).to.eq("This title is awesome too!");
-    });
-  });
-
-  it("displays title with anchors", () => {
-    cy.get(".element-container .stMarkdown h1").then(els => {
-      cy.wrap(els[0]).should("have.attr", "id", "this-title-is-awesome");
-      cy.wrap(els[1]).should("have.attr", "id", "awesome-title");
-    });
+    cy.get(".element-container .stMarkdown h1").should(
+      "contain",
+      "This title is awesome!"
+    );
   });
 });

frontend/src/App.tsx

@@ -58,7 +58,6 @@ import {
   SessionState,
   Config,
 } from "autogen/proto"
-import { without, concat } from "lodash"
 
 import { RERUN_PROMPT_MODAL_DIALOG } from "lib/baseconsts"
 import { SessionInfo } from "lib/SessionInfo"
@@ -102,7 +101,6 @@ interface State {
   layout: PageConfig.Layout
   initialSidebarState: PageConfig.SidebarState
   allowRunOnSave: boolean
-  reportFinishedHandlers: (() => void)[]
   deployParams?: IDeployParams | null
 }
 
@@ -160,7 +158,6 @@ export class App extends PureComponent<Props, State> {
       layout: PageConfig.Layout.CENTERED,
       initialSidebarState: PageConfig.SidebarState.AUTO,
       allowRunOnSave: true,
-      reportFinishedHandlers: [],
       deployParams: null,
     }
 
@@ -557,12 +554,6 @@ export class App extends PureComponent<Props, State> {
    */
   handleReportFinished(status: ForwardMsg.ReportFinishedStatus): void {
     if (status === ForwardMsg.ReportFinishedStatus.FINISHED_SUCCESSFULLY) {
-      // Notify any subscribers of this event (and do it on the next cycle of
-      // the event loop)
-      window.setTimeout(() => {
-        this.state.reportFinishedHandlers.map(handler => handler())
-      }, 0)
-
       // Clear any stale elements left over from the previous run.
       // (We don't do this if our script had a compilation error and didn't
       // finish successfully.)
@@ -904,18 +895,6 @@ export class App extends PureComponent<Props, State> {
     this.setState({ isFullScreen })
   }
 
-  addReportFinishedHandler = (func: () => void): void => {
-    this.setState({
-      reportFinishedHandlers: concat(this.state.reportFinishedHandlers, func),
-    })
-  }
-
-  removeReportFinishedHandler = (func: () => void): void => {
-    this.setState({
-      reportFinishedHandlers: without(this.state.reportFinishedHandlers, func),
-    })
-  }
-
   render(): JSX.Element {
     const {
       allowRunOnSave,
@@ -956,8 +935,6 @@ export class App extends PureComponent<Props, State> {
           embedded: isEmbeddedInIFrame(),
           isFullScreen,
           setFullScreen: this.handleFullScreen,
-          addReportFinishedHandler: this.addReportFinishedHandler,
-          removeReportFinishedHandler: this.removeReportFinishedHandler,
         }}
       >
         <HotKeys

frontend/src/components/core/PageLayoutContext/index.tsx

@@ -25,6 +25,4 @@ export default React.createContext({
   embedded: false,
   isFullScreen: false,
   setFullScreen: (value: boolean) => {},
-  addReportFinishedHandler: (func: () => void) => {},
-  removeReportFinishedHandler: (func: () => void) => {},
 })

frontend/src/components/elements/DataFrame/DataFrame.test.tsx

@@ -18,11 +18,17 @@
 import React from "react"
 import { shallow } from "enzyme"
 import { fromJS } from "immutable"
+import { random, times } from "lodash"
 
 import mockDataFrame from "./mock"
 import { DataFrame, DataFrameProps } from "./DataFrame"
 import { MIN_CELL_WIDTH_PX } from "./DataFrameUtil"
 
+const SCROLLBAR_SIZE = 10
+jest.mock("vendor/dom-helpers", () => ({
+  scrollbarSize: () => SCROLLBAR_SIZE,
+}))
+
 const getProps = (
   elementProps: Record<string, unknown> = {}
 ): DataFrameProps => ({
@@ -34,6 +40,20 @@ const getProps = (
   height: 400,
 })
 
+const fakeData = (
+  numRows: number,
+  numCols: number
+): Partial<typeof mockDataFrame> => ({
+  data: {
+    cols: times(numCols, () => ({
+      int64s: { data: times(numRows, () => random(0, 9)) },
+      type: "int64s",
+    })),
+  },
+  index: { rangeIndex: { start: 0, stop: numRows }, type: "rangeIndex" },
+  columns: { rangeIndex: { start: 0, stop: numCols }, type: "rangeIndex" },
+})
+
 describe("DataFrame Element", () => {
   const props = getProps()
   const wrapper = shallow(<DataFrame {...props} />)
@@ -56,10 +76,12 @@ describe("DataFrame Element", () => {
     expect(multiGridProps.columnCount).toBe(11)
     expect(multiGridProps).toHaveProperty("enableFixedColumnScroll")
     expect(multiGridProps).toHaveProperty("enableFixedRowScroll")
-    expect(multiGridProps.height).toBe(275)
+    // 275px for the dataframe itself + 10px for the horizontal scrollbar
+    expect(multiGridProps.height).toBe(285)
     expect(multiGridProps.rowHeight).toBe(25)
     expect(multiGridProps.rowCount).toBe(11)
-    expect(multiGridProps.width).toBe(398)
+    // 400px full container width - 12px for border and vertical scrollbar
+    expect(multiGridProps.width).toBe(388)
   })
 
   it("should render as empty if there's no data", () => {
@@ -81,4 +103,30 @@ describe("DataFrame Element", () => {
     expect(multiGridProps.rowCount).toBe(1)
     expect(multiGridProps.width).toBe(60)
   })
+
+  it("adds extra height for horizontal scrollbar when wide but not tall", () => {
+    let props = getProps({ ...fakeData(1, 1) })
+    let wrapper = shallow(<DataFrame {...props} />)
+    const normalHeight = wrapper.find("MultiGrid").props().height
+
+    props = getProps({ ...fakeData(1, 20) })
+    wrapper = shallow(<DataFrame {...props} />)
+    const heightWithScrollbar = wrapper.find("MultiGrid").props().height
+
+    expect(heightWithScrollbar).toBe(normalHeight + SCROLLBAR_SIZE)
+  })
+
+  it("adds extra width for vertical scrollbar when tall but not wide", () => {
+    // Be careful to ensure that the number of digits needed to display the
+    // largest row number is the same for the two DataFrames.
+    let props = getProps({ ...fakeData(11, 1) })
+    let wrapper = shallow(<DataFrame {...props} />)
+    const normalWidth = wrapper.find("MultiGrid").props().width
+
+    props = getProps({ ...fakeData(99, 1) })
+    wrapper = shallow(<DataFrame {...props} />)
+    const widthWithScrollbar = wrapper.find("MultiGrid").props().width
+
+    expect(widthWithScrollbar).toBe(normalWidth + SCROLLBAR_SIZE)
+  })
 })

frontend/src/components/elements/DataFrame/DataFrameUtil.tsx

@@ -22,6 +22,7 @@ import {
 } from "lib/dataFrameProto"
 import { toFormattedString } from "lib/format"
 import { logWarning } from "lib/log"
+import { scrollbarSize } from "vendor/dom-helpers"
 import React, { ReactElement, ComponentType } from "react"
 import { Map as ImmutableMap } from "immutable"
 import {
@@ -90,6 +91,7 @@ interface ComputedWidths {
   elementWidth: number
   columnWidth: ({ index }: { index: number }) => number
   headerWidth: number
+  needsHorizontalScrollbar: boolean
 }
 
 const DEFAULT_HEIGHT = 300
@@ -116,15 +118,21 @@ export const getDimensions = (
   const headerHeight = rowHeight * headerRows
   const border = 2
 
-  let { elementWidth, columnWidth, headerWidth } = getWidths(
+  // Reserve enough space to render the dataframe border as well as a vertical
+  // scrollbar if necessary.
+  const availableWidth = width - border - scrollbarSize()
+  const widths = getWidths(
     cols,
     rows,
     headerCols,
     headerRows,
-    width - border,
+    availableWidth,
     cellContentsGetter
   )
 
+  let { elementWidth, columnWidth, headerWidth } = widths
+  const { needsHorizontalScrollbar } = widths
+
   // Add space for the "empty" text when the table is empty.
   const EMPTY_WIDTH = 60 // px
   if (dataRows === 0 && elementWidth < EMPTY_WIDTH) {
@@ -139,14 +147,24 @@ export const getDimensions = (
     }
   }
 
+  // Allocate extra space for horizontal and vertical scrollbars, if needed.
+  const totalHeight = rows * rowHeight
+  const maxHeight = height || DEFAULT_HEIGHT
+
+  const horizScrollbarHeight = needsHorizontalScrollbar ? scrollbarSize() : 0
+  height = Math.min(totalHeight + horizScrollbarHeight, maxHeight)
+
+  const needsVerticalScrollbar = totalHeight > maxHeight
+  elementWidth += needsVerticalScrollbar ? scrollbarSize() : 0
+
   return {
     rowHeight,
     headerHeight,
     border,
-    height: Math.min(rows * rowHeight, height || DEFAULT_HEIGHT),
-    elementWidth,
     columnWidth,
     headerWidth,
+    elementWidth,
+    height,
   }
 }
 
@@ -305,6 +323,7 @@ export function getWidths(
   }
 
   const elementWidth = Math.min(distributedTableTotal, containerWidth)
+  const needsHorizontalScrollbar = distributedTableTotal > containerWidth
   const columnWidth = ({ index }: { index: number }): number =>
     distributedTable[index]
 
@@ -316,5 +335,6 @@ export function getWidths(
     elementWidth,
     columnWidth,
     headerWidth,
+    needsHorizontalScrollbar,
   }
 }