From f50f3deb8070aba0de72b2977dd69bc2576ce4d7 Mon Sep 17 00:00:00 2001 From: Alexandre Poirot Date: Fri, 1 Mar 2019 10:08:52 +0000 Subject: [PATCH] Bug 1529018 - Simplify netmonitor DOM/CSS around table elements. r=Honza * Use the right hiearchy of table elements: (display: table-header-group) (display: table-cell) (display: table-row-group) (display: table-row) ...
...
...
* Prevent using position:absolute within the Instead set the overflow: auto on the parent of
in order to create an scrollable table. * Remove any wrapper / intermediate elements that would be intermediate childs between
and or between and , ... * Remove manual size definition in JS from RequestListContent. Differential Revision: https://phabricator.services.mozilla.com/D20383 --HG-- extra : moz-landing-system : lando --- .../src/assets/styles/RequestList.css | 33 ++++--------- .../src/components/RequestListContent.js | 46 ++++++++++--------- .../src/components/RequestListHeader.js | 2 +- .../test/browser_net_accessibility-02.js | 2 +- .../netmonitor/test/browser_net_autoscroll.js | 8 ++-- .../test/browser_net_columns_showhide.js | 4 +- .../test/browser_net_headers-alignment.js | 11 +++-- .../test/browser_net_image-tooltip.js | 2 +- devtools/client/netmonitor/test/head.js | 7 ++- 9 files changed, 52 insertions(+), 63 deletions(-) diff --git a/devtools/client/netmonitor/src/assets/styles/RequestList.css b/devtools/client/netmonitor/src/assets/styles/RequestList.css index 95f48453fe4f..26e6e493fc6c 100644 --- a/devtools/client/netmonitor/src/assets/styles/RequestList.css +++ b/devtools/client/netmonitor/src/assets/styles/RequestList.css @@ -51,25 +51,17 @@ color: var(--table-text-color); } -.requests-list-wrapper { - width: 100%; - height: 100%; +.requests-list-scroll { + overflow-x: hidden; + overflow-y: auto; } .requests-list-table { display: table; - position: relative; - width: 100%; - height: 100%; } -.requests-list-contents { +.requests-list-row-group { display: table-row-group; - position: absolute; - overflow-x: hidden; - overflow-y: auto; - --timings-scale: 1; - --timings-rev-scale: 1; } .requests-list-column { @@ -89,19 +81,20 @@ /* Requests list headers */ -.requests-list-headers-wrapper { +.requests-list-headers-group { + display: table-header-group; + position: sticky; top: 0; - z-index: 1; + left: 0; width: 100%; - padding: 0; + z-index: 1; } .requests-list-headers { - display: table-header-group; + display: table-row; height: 24px; padding: 0; - width: 100%; } .requests-list-headers .requests-list-column:first-child .requests-list-header-button { @@ -574,13 +567,7 @@ /* Request list item */ .request-list-item { - position: relative; -/* display: table-row; - Bug 1431132: Disabling this rule is surprising as we no longer have "rows". - But this helps preventing reflowing the whole requests list anytime we append - a new request/row. -*/ height: 24px; line-height: 24px; } diff --git a/devtools/client/netmonitor/src/components/RequestListContent.js b/devtools/client/netmonitor/src/components/RequestListContent.js index 42b255050b08..2d207cb23a8c 100644 --- a/devtools/client/netmonitor/src/components/RequestListContent.js +++ b/devtools/client/netmonitor/src/components/RequestListContent.js @@ -86,24 +86,22 @@ class RequestListContent extends Component { componentDidMount() { // Install event handler for displaying a tooltip - this.tooltip.startTogglingOnHover(this.refs.contentEl, this.onHover, { + this.tooltip.startTogglingOnHover(this.refs.scrollEl, this.onHover, { toggleDelay: REQUESTS_TOOLTIP_TOGGLE_DELAY, interactive: true, }); // Install event handler to hide the tooltip on scroll - this.refs.contentEl.addEventListener("scroll", this.onScroll, true); + this.refs.scrollEl.addEventListener("scroll", this.onScroll, true); this.onResize(); } componentWillUpdate(nextProps) { // Check if the list is scrolled to bottom before the UI update. - // The scroll is ever needed only if new rows are added to the list. - const delta = nextProps.displayedRequests.size - this.props.displayedRequests.size; - this.shouldScrollBottom = delta > 0 && this.isScrolledToBottom(); + this.shouldScrollBottom = this.isScrolledToBottom(); } componentDidUpdate(prevProps) { - const node = this.refs.contentEl; + const node = this.refs.scrollEl; // Keep the list scrolled to bottom if a new row was added if (this.shouldScrollBottom && node.scrollTop !== MAX_SCROLL_HEIGHT) { // Using maximum scroll height rather than node.scrollHeight to avoid sync reflow. @@ -118,7 +116,7 @@ class RequestListContent extends Component { } componentWillUnmount() { - this.refs.contentEl.removeEventListener("scroll", this.onScroll, true); + this.refs.scrollEl.removeEventListener("scroll", this.onScroll, true); // Uninstall the tooltip event handler this.tooltip.stopTogglingOnHover(); @@ -126,23 +124,22 @@ class RequestListContent extends Component { } onResize() { - const parent = this.refs.contentEl.parentNode; - this.refs.contentEl.style.width = parent.offsetWidth + "px"; - this.refs.contentEl.style.height = parent.offsetHeight + "px"; + const parent = this.refs.scrollEl.parentNode; + this.refs.scrollEl.style.width = parent.offsetWidth + "px"; + this.refs.scrollEl.style.height = parent.offsetHeight + "px"; } isScrolledToBottom() { - const { contentEl } = this.refs; - const lastChildEl = contentEl.lastElementChild; + const { scrollEl, rowGroupEl } = this.refs; + const lastChildEl = rowGroupEl.lastElementChild; if (!lastChildEl) { return false; } - const lastChildRect = lastChildEl.getBoundingClientRect(); - const contentRect = contentEl.getBoundingClientRect(); - - return (lastChildRect.height + lastChildRect.top) <= contentRect.bottom; + const lastNodeHeight = lastChildEl.clientHeight; + return scrollEl.scrollTop + scrollEl.clientHeight >= + scrollEl.scrollHeight - lastNodeHeight / 2; } /** @@ -275,16 +272,21 @@ class RequestListContent extends Component { } = this.props; return ( - div({ className: "requests-list-wrapper" }, - div({ className: "requests-list-table" }, + div({ + ref: "scrollEl", + className: "requests-list-scroll", + }, + div({ + className: "requests-list-table", + }, + RequestListHeader(), div({ - ref: "contentEl", - className: "requests-list-contents", + ref: "rowGroupEl", + className: "requests-list-row-group", tabIndex: 0, onKeyDown: this.onKeyDown, style: { "--timings-scale": scale, "--timings-rev-scale": 1 / scale }, }, - RequestListHeader(), displayedRequests.map((item, index) => RequestListItem({ firstRequestStartedMillis, fromCache: item.status === "304" || item.fromCache, @@ -302,7 +304,7 @@ class RequestListContent extends Component { onWaterfallMouseDown: () => onWaterfallMouseDown(), requestFilterTypes, })) - ) + ) // end of requests-list-row-group"> ) ) ); diff --git a/devtools/client/netmonitor/src/components/RequestListHeader.js b/devtools/client/netmonitor/src/components/RequestListHeader.js index e7786b029376..5429ea458ffd 100644 --- a/devtools/client/netmonitor/src/components/RequestListHeader.js +++ b/devtools/client/netmonitor/src/components/RequestListHeader.js @@ -167,7 +167,7 @@ class RequestListHeader extends Component { const { columns, scale, sort, sortBy, waterfallWidth } = this.props; return ( - div({ className: "devtools-toolbar requests-list-headers-wrapper" }, + div({ className: "devtools-toolbar requests-list-headers-group" }, div({ className: "devtools-toolbar requests-list-headers", onContextMenu: this.onContextMenu, diff --git a/devtools/client/netmonitor/test/browser_net_accessibility-02.js b/devtools/client/netmonitor/test/browser_net_accessibility-02.js index 86337b014282..17b7747dee47 100644 --- a/devtools/client/netmonitor/test/browser_net_accessibility-02.js +++ b/devtools/client/netmonitor/test/browser_net_accessibility-02.js @@ -33,7 +33,7 @@ add_task(async function() { // Execute requests. await performRequests(monitor, tab, 2); - document.querySelector(".requests-list-contents").focus(); + document.querySelector(".requests-list-row-group").focus(); check(-1, false); diff --git a/devtools/client/netmonitor/test/browser_net_autoscroll.js b/devtools/client/netmonitor/test/browser_net_autoscroll.js index 2eefedc8d71e..02ab452b7481 100644 --- a/devtools/client/netmonitor/test/browser_net_autoscroll.js +++ b/devtools/client/netmonitor/test/browser_net_autoscroll.js @@ -19,7 +19,7 @@ add_task(async function() { // Wait until the first request makes the empty notice disappear await waitForRequestListToAppear(); - const requestsContainer = document.querySelector(".requests-list-contents"); + const requestsContainer = document.querySelector(".requests-list-scroll"); ok(requestsContainer, "Container element exists as expected."); // (1) Check that the scroll position is maintained at the bottom @@ -53,7 +53,7 @@ add_task(async function() { store.dispatch(Actions.selectRequestByIndex(0)); await waitForNetworkEvents(monitor, 8); await waitSomeTime(); - const requestsContainerHeaders = requestsContainer.firstChild; + const requestsContainerHeaders = document.querySelector(".requests-list-headers"); const headersHeight = requestsContainerHeaders.offsetHeight; is(requestsContainer.scrollTop, headersHeight, "Did not scroll."); @@ -67,7 +67,7 @@ add_task(async function() { function waitForRequestListToAppear() { info("Waiting until the empty notice disappears and is replaced with the list"); - return waitUntil(() => !!document.querySelector(".requests-list-contents")); + return waitUntil(() => !!document.querySelector(".requests-list-row-group")); } async function waitForRequestsToOverflowContainer() { @@ -75,7 +75,7 @@ add_task(async function() { while (true) { info("Waiting for one network request"); await waitForNetworkEvents(monitor, 1); - if (requestsContainer.scrollHeight > requestsContainer.clientHeight) { + if (requestsContainer.scrollHeight > requestsContainer.clientHeight + 50) { info("The list is long enough, returning"); return; } diff --git a/devtools/client/netmonitor/test/browser_net_columns_showhide.js b/devtools/client/netmonitor/test/browser_net_columns_showhide.js index 9b72d9d513e7..61fa4407d563 100644 --- a/devtools/client/netmonitor/test/browser_net_columns_showhide.js +++ b/devtools/client/netmonitor/test/browser_net_columns_showhide.js @@ -27,7 +27,7 @@ add_task(async function() { await requestData(item.id, "responseHeaders"); } - const requestsContainer = document.querySelector(".requests-list-contents"); + const requestsContainer = document.querySelector(".requests-list-row-group"); ok(requestsContainer, "Container element exists as expected."); const headers = document.querySelector(".requests-list-headers"); @@ -61,7 +61,7 @@ async function testWhiteSpaceContextMenuItem(column, document, parent) { info(`Right clicking on white-space in the header to get the context menu`); EventUtils.sendMouseEvent({ type: "contextmenu" }, - document.querySelector(".devtools-toolbar.requests-list-headers")); + document.querySelector(".requests-list-headers")); // Wait for next tick to do stuff async and force repaint. await waitForTick(); diff --git a/devtools/client/netmonitor/test/browser_net_headers-alignment.js b/devtools/client/netmonitor/test/browser_net_headers-alignment.js index 1af56dae42a7..ed9a56b0e749 100644 --- a/devtools/client/netmonitor/test/browser_net_headers-alignment.js +++ b/devtools/client/netmonitor/test/browser_net_headers-alignment.js @@ -19,12 +19,13 @@ add_task(async function() { // Wait until the first request makes the empty notice disappear await waitForRequestListToAppear(); - const requestsContainer = document.querySelector(".requests-list-contents"); - ok(requestsContainer, "Container element exists as expected."); + const requestsContainerScroll = document.querySelector(".requests-list-scroll"); + ok(requestsContainerScroll, "Container element exists as expected."); + const requestsContainer = document.querySelector(".requests-list-row-group"); const headers = document.querySelector(".requests-list-headers"); ok(headers, "Headers element exists as expected."); - await waitForRequestsToOverflowContainer(monitor, requestsContainer); + await waitForRequestsToOverflowContainer(monitor, requestsContainerScroll); testColumnsAlignment(headers, requestsContainer); @@ -38,7 +39,7 @@ add_task(async function() { function waitForRequestListToAppear() { info("Waiting until the empty notice disappears and is replaced with the list"); - return waitUntil(() => !!document.querySelector(".requests-list-contents")); + return waitUntil(() => !!document.querySelector(".requests-list-row-group")); } }); @@ -47,7 +48,7 @@ async function waitForRequestsToOverflowContainer(monitor, requestList) { while (true) { info("Waiting for one network request"); await waitForNetworkEvents(monitor, 1); - if (requestList.scrollHeight > requestList.clientHeight) { + if (requestList.scrollHeight > requestList.clientHeight + 50) { info("The list is long enough, returning"); return; } diff --git a/devtools/client/netmonitor/test/browser_net_image-tooltip.js b/devtools/client/netmonitor/test/browser_net_image-tooltip.js index 2df9c9f2ecbb..acab179bba26 100644 --- a/devtools/client/netmonitor/test/browser_net_image-tooltip.js +++ b/devtools/client/netmonitor/test/browser_net_image-tooltip.js @@ -46,7 +46,7 @@ add_task(async function test() { await showTooltipAndVerify(document.querySelectorAll(".request-list-item")[1]); info("Checking if the image thumbnail is hidden when mouse leaves the menu widget"); - const requestsListContents = document.querySelector(".requests-list-contents"); + const requestsListContents = document.querySelector(".requests-list-row-group"); EventUtils.synthesizeMouse(requestsListContents, 0, 0, { type: "mousemove" }, monitor.panelWin); await waitUntil(() => !toolboxDoc.querySelector(".tooltip-container.tooltip-visible")); diff --git a/devtools/client/netmonitor/test/head.js b/devtools/client/netmonitor/test/head.js index 5b84015519cb..79dbee068069 100644 --- a/devtools/client/netmonitor/test/head.js +++ b/devtools/client/netmonitor/test/head.js @@ -688,8 +688,7 @@ function waitForContentMessage(name) { } function testColumnsAlignment(headers, requestList) { - // Get first request line, not child 0 as this is the headers - const firstRequestLine = requestList.childNodes[1]; + const firstRequestLine = requestList.childNodes[0]; // Find number of columns const numberOfColumns = headers.childElementCount; @@ -708,7 +707,7 @@ async function hideColumn(monitor, column) { info(`Clicking context-menu item for ${column}`); EventUtils.sendMouseEvent({ type: "contextmenu" }, - document.querySelector(".devtools-toolbar.requests-list-headers")); + document.querySelector(".requests-list-headers")); const onHeaderRemoved = waitForDOM(document, `#requests-list-${column}-button`, 0); parent.document.querySelector(`#request-list-header-${column}-toggle`).click(); @@ -723,7 +722,7 @@ async function showColumn(monitor, column) { info(`Clicking context-menu item for ${column}`); EventUtils.sendMouseEvent({ type: "contextmenu" }, - document.querySelector(".devtools-toolbar.requests-list-headers")); + document.querySelector(".requests-list-headers")); const onHeaderAdded = waitForDOM(document, `#requests-list-${column}-button`, 1); parent.document.querySelector(`#request-list-header-${column}-toggle`).click();