Bug 1416714 - Remove request list scroll to bottom r?honza draft
authorRicky Chien <ricky060709@gmail.com>
Wed, 15 Nov 2017 14:02:01 +0800
changeset 698132 cd350dceed9fa50af3056ef33fddf3cf2a747263
parent 698131 79bc20e22f7b9b92e01e085babc05fe30d196157
child 740315 1672908b3eee634ad3e94293ee43e40efbdbea8b
push id89222
push userbmo:rchien@mozilla.com
push dateWed, 15 Nov 2017 10:15:47 +0000
reviewershonza
bugs1416714
milestone59.0a1
Bug 1416714 - Remove request list scroll to bottom r?honza MozReview-Commit-ID: 6yD63GIVwjQ
devtools/client/netmonitor/src/components/RequestListContent.js
devtools/client/netmonitor/src/components/RequestListItem.js
devtools/client/netmonitor/test/browser.ini
devtools/client/netmonitor/test/browser_net_autoscroll.js
--- a/devtools/client/netmonitor/src/components/RequestListContent.js
+++ b/devtools/client/netmonitor/src/components/RequestListContent.js
@@ -23,18 +23,16 @@ const {
 const RequestListHeader = createFactory(require("./RequestListHeader"));
 const RequestListItem = createFactory(require("./RequestListItem"));
 const RequestListContextMenu = require("../request-list-context-menu");
 
 const { div } = DOM;
 
 // Tooltip show / hide delay in ms
 const REQUESTS_TOOLTIP_TOGGLE_DELAY = 500;
-// Gecko's scrollTop is int32_t, so the maximum value is 2^31 - 1 = 2147483647
-const MAX_SCROLL_HEIGHT = 2147483647;
 
 /**
  * Renders the actual contents of the request list.
  */
 class RequestListContent extends Component {
   static get propTypes() {
     return {
       connector: PropTypes.object.isRequired,
@@ -48,22 +46,20 @@ class RequestListContent extends Compone
       onWaterfallMouseDown: PropTypes.func.isRequired,
       scale: PropTypes.number,
       selectedRequestId: PropTypes.string,
     };
   }
 
   constructor(props) {
     super(props);
-    this.isScrolledToBottom = this.isScrolledToBottom.bind(this);
     this.onHover = this.onHover.bind(this);
     this.onScroll = this.onScroll.bind(this);
     this.onKeyDown = this.onKeyDown.bind(this);
     this.onContextMenu = this.onContextMenu.bind(this);
-    this.onFocusedNodeChange = this.onFocusedNodeChange.bind(this);
   }
 
   componentWillMount() {
     const { dispatch, connector } = this.props;
     this.contextMenu = new RequestListContextMenu({
       cloneSelectedRequest: () => dispatch(Actions.cloneSelectedRequest()),
       getTabTarget: connector.getTabTarget,
       getLongString: connector.getLongString,
@@ -79,53 +75,24 @@ class RequestListContent extends Compone
       toggleDelay: REQUESTS_TOOLTIP_TOGGLE_DELAY,
       interactive: true
     });
 
     // Install event handler to hide the tooltip on scroll
     this.refs.contentEl.addEventListener("scroll", this.onScroll, true);
   }
 
-  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();
-  }
-
-  componentDidUpdate(prevProps) {
-    let node = this.refs.contentEl;
-    // 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.
-      node.scrollTop = MAX_SCROLL_HEIGHT;
-    }
-  }
-
   componentWillUnmount() {
+    // Uninstall scroll event handler
     this.refs.contentEl.removeEventListener("scroll", this.onScroll, true);
 
     // Uninstall the tooltip event handler
     this.tooltip.stopTogglingOnHover();
   }
 
-  isScrolledToBottom() {
-    const { contentEl } = this.refs;
-    const lastChildEl = contentEl.lastElementChild;
-
-    if (!lastChildEl) {
-      return false;
-    }
-
-    let lastChildRect = lastChildEl.getBoundingClientRect();
-    let contentRect = contentEl.getBoundingClientRect();
-
-    return (lastChildRect.height + lastChildRect.top) <= contentRect.bottom;
-  }
-
   /**
    * The predicate used when deciding whether a popup should be shown
    * over a request item or not.
    *
    * @param nsIDOMNode target
    *        The element node currently being hovered.
    * @param object tooltip
    *        The current tooltip instance.
@@ -198,56 +165,47 @@ class RequestListContent extends Compone
     }
   }
 
   onContextMenu(evt) {
     evt.preventDefault();
     this.contextMenu.open(evt);
   }
 
-  /**
-   * If selection has just changed (by keyboard navigation), don't keep the list
-   * scrolled to bottom, but allow scrolling up with the selection.
-   */
-  onFocusedNodeChange() {
-    this.shouldScrollBottom = false;
-  }
-
   render() {
     const {
       columns,
       displayedRequests,
       firstRequestStartedMillis,
       onItemMouseDown,
       onWaterfallMouseDown,
       scale,
       selectedRequestId,
     } = this.props;
 
     return (
-      div({ className: "requests-list-wrapper"},
-        div({ className: "requests-list-table"},
+      div({ className: "requests-list-wrapper" },
+        div({ className: "requests-list-table" },
           div({
             ref: "contentEl",
             className: "requests-list-contents",
             tabIndex: 0,
             onKeyDown: this.onKeyDown,
-            style: {"--timings-scale": scale, "--timings-rev-scale": 1 / scale}
+            style: { "--timings-scale": scale, "--timings-rev-scale": 1 / scale }
           },
             RequestListHeader(),
             displayedRequests.map((item, index) => RequestListItem({
               firstRequestStartedMillis,
               fromCache: item.status === "304" || item.fromCache,
               columns,
               item,
               index,
               isSelected: item.id === selectedRequestId,
               key: item.id,
               onContextMenu: this.onContextMenu,
-              onFocusedNodeChange: this.onFocusedNodeChange,
               onMouseDown: () => onItemMouseDown(item.id),
               onWaterfallMouseDown: () => onWaterfallMouseDown(),
             }))
           )
         )
       )
     );
   }
--- a/devtools/client/netmonitor/src/components/RequestListItem.js
+++ b/devtools/client/netmonitor/src/components/RequestListItem.js
@@ -77,17 +77,16 @@ class RequestListItem extends Component 
     return {
       columns: PropTypes.object.isRequired,
       item: PropTypes.object.isRequired,
       index: PropTypes.number.isRequired,
       isSelected: PropTypes.bool.isRequired,
       firstRequestStartedMillis: PropTypes.number.isRequired,
       fromCache: PropTypes.bool,
       onContextMenu: PropTypes.func.isRequired,
-      onFocusedNodeChange: PropTypes.func,
       onMouseDown: PropTypes.func.isRequired,
       onWaterfallMouseDown: PropTypes.func.isRequired,
       waterfallWidth: PropTypes.number,
     };
   }
 
   componentDidMount() {
     if (this.props.isSelected) {
@@ -99,19 +98,16 @@ class RequestListItem extends Component 
     return !propertiesEqual(UPDATED_REQ_ITEM_PROPS, this.props.item, nextProps.item) ||
       !propertiesEqual(UPDATED_REQ_PROPS, this.props, nextProps) ||
       !I.is(this.props.columns, nextProps.columns);
   }
 
   componentDidUpdate(prevProps) {
     if (!prevProps.isSelected && this.props.isSelected) {
       this.refs.listItem.focus();
-      if (this.props.onFocusedNodeChange) {
-        this.props.onFocusedNodeChange();
-      }
     }
   }
 
   render() {
     let {
       columns,
       item,
       index,
--- a/devtools/client/netmonitor/test/browser.ini
+++ b/devtools/client/netmonitor/test/browser.ini
@@ -56,17 +56,16 @@ support-files =
   !/devtools/client/framework/test/shared-head.js
   xhr_bundle.js
   xhr_bundle.js.map
   xhr_original.js
 
 [browser_net_accessibility-01.js]
 [browser_net_accessibility-02.js]
 [browser_net_api-calls.js]
-[browser_net_autoscroll.js]
 [browser_net_cached-status.js]
 [browser_net_cause.js]
 [browser_net_cause_redirect.js]
 [browser_net_cause_source_map.js]
 [browser_net_service-worker-status.js]
 [browser_net_charts-01.js]
 [browser_net_charts-02.js]
 [browser_net_charts-03.js]
deleted file mode 100644
--- a/devtools/client/netmonitor/test/browser_net_autoscroll.js
+++ /dev/null
@@ -1,100 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-"use strict";
-
-/**
- * Bug 863102 - Automatically scroll down upon new network requests.
- * edited to account for changes made to fix Bug 1360457
- */
-add_task(function* () {
-  requestLongerTimeout(4);
-
-  let { tab, monitor } = yield initNetMonitor(INFINITE_GET_URL, true);
-  let { document, windowRequire, store } = monitor.panelWin;
-  let Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
-
-  store.dispatch(Actions.batchEnable(false));
-
-  // Wait until the first request makes the empty notice disappear
-  yield waitForRequestListToAppear();
-
-  let requestsContainer = document.querySelector(".requests-list-contents");
-  ok(requestsContainer, "Container element exists as expected.");
-
-  // (1) Check that the scroll position is maintained at the bottom
-  // when the requests overflow the vertical size of the container.
-  yield waitForRequestsToOverflowContainer();
-  yield waitForScroll();
-  ok(true, "Scrolled to bottom on overflow.");
-
-  // (2) Now scroll to the top and check that additional requests
-  // do not change the scroll position.
-  requestsContainer.scrollTop = 0;
-  yield waitSomeTime();
-  ok(!scrolledToBottom(requestsContainer), "Not scrolled to bottom.");
-  // save for comparison later
-  let scrollTop = requestsContainer.scrollTop;
-  yield waitForNetworkEvents(monitor, 8);
-  yield waitSomeTime();
-  is(requestsContainer.scrollTop, scrollTop, "Did not scroll.");
-
-  // (3) Now set the scroll position back at the bottom and check that
-  // additional requests *do* cause the container to scroll down.
-  requestsContainer.scrollTop = requestsContainer.scrollHeight;
-  ok(scrolledToBottom(requestsContainer), "Set scroll position to bottom.");
-  yield waitForNetworkEvents(monitor, 8);
-  yield waitForScroll();
-  ok(true, "Still scrolled to bottom.");
-
-  // (4) Now select the first item in the list
-  // and check that additional requests do not change the scroll position
-  // from just below the headers.
-  store.dispatch(Actions.selectRequestByIndex(0));
-  yield waitForNetworkEvents(monitor, 8);
-  yield waitSomeTime();
-  let requestsContainerHeaders = requestsContainer.firstChild;
-  let headersHeight = requestsContainerHeaders.offsetHeight;
-  is(requestsContainer.scrollTop, headersHeight, "Did not scroll.");
-
-  // Stop doing requests.
-  yield ContentTask.spawn(tab.linkedBrowser, {}, function () {
-    content.wrappedJSObject.stopRequests();
-  });
-
-  // Done: clean up.
-  return teardown(monitor);
-
-  function waitForRequestListToAppear() {
-    info("Waiting until the empty notice disappears and is replaced with the list");
-    return waitUntil(() => !!document.querySelector(".requests-list-contents"));
-  }
-
-  function* waitForRequestsToOverflowContainer() {
-    info("Waiting for enough requests to overflow the container");
-    while (true) {
-      info("Waiting for one network request");
-      yield waitForNetworkEvents(monitor, 1);
-      console.log(requestsContainer.scrollHeight);
-      console.log(requestsContainer.clientHeight);
-      if (requestsContainer.scrollHeight > requestsContainer.clientHeight) {
-        info("The list is long enough, returning");
-        return;
-      }
-    }
-  }
-
-  function scrolledToBottom(element) {
-    return element.scrollTop + element.clientHeight >= element.scrollHeight;
-  }
-
-  function waitSomeTime() {
-    // Wait to make sure no scrolls happen
-    return wait(50);
-  }
-
-  function waitForScroll() {
-    info("Waiting for the list to scroll to bottom");
-    return waitUntil(() => scrolledToBottom(requestsContainer));
-  }
-});