Bug 1521026 - Flex highlighter shouldn't scroll for position:fixed elements r=gl
authorMichael Ratcliffe <mratcliffe@mozilla.com>
Mon, 21 Jan 2019 16:49:24 +0000
changeset 454708 dc8bd111db7000fdc8c3c8a448771e8a5900339c
parent 454707 3ee7a4b57a9a955db8ba8d8f961754c4db638ebc
child 454709 2717febc588c1e7145af3a68cb0f932fa0cb6f81
push id35411
push usercsabou@mozilla.com
push dateTue, 22 Jan 2019 03:53:40 +0000
treeherdermozilla-central@ada22b635f8d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgl
bugs1521026, 1509071
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1521026 - Flex highlighter shouldn't scroll for position:fixed elements r=gl This fixes the issue but because of our virtual canvas implementation and the fact that reflow events are batched there is quite a bit of flicker and some drag (see attached video). Unfortunately, until bug 1509071 is implemented (full screen canvas using `position:fixed`) we can't really do anything about the flicker... I suppose we could stop batching reflow events but that would make all of our highlighters unusably slow. Differential Revision: https://phabricator.services.mozilla.com/D16988
devtools/server/actors/highlighters/flexbox.js
devtools/shared/layout/utils.js
--- a/devtools/server/actors/highlighters/flexbox.js
+++ b/devtools/server/actors/highlighters/flexbox.js
@@ -17,16 +17,17 @@ const {
   updateCanvasPosition,
 } = require("./utils/canvas");
 const {
   CanvasFrameAnonymousContentHelper,
   createNode,
   getComputedStyle,
 } = require("./utils/markup");
 const {
+  getAbsoluteScrollOffsetsForNode,
   getCurrentZoom,
   getDisplayPixelRatio,
   getUntransformedQuad,
   getWindowDimensions,
   setIgnoreLayoutChanges,
 } = require("devtools/shared/layout/utils");
 
 const FLEXBOX_LINES_PROPERTIES = {
@@ -568,17 +569,17 @@ class FlexboxHighlighter extends AutoRef
     }
 
     const lineWidth = getDisplayPixelRatio(this.win);
     const options = { matrix: this.currentMatrix };
     const { width: containerWidth, height: containerHeight } =
       getUntransformedQuad(this.currentNode, "content").getBounds();
 
     this.setupCanvas({
-      useScrollOffsets: true,
+      useContainerScrollOffsets: true,
     });
 
     for (const flexLine of this.flexData.lines) {
       const { crossStart, crossSize } = flexLine;
 
       switch (this.axes) {
         case "horizontal-lr vertical-tb":
         case "horizontal-lr vertical-bt":
@@ -653,17 +654,17 @@ class FlexboxHighlighter extends AutoRef
 
     const { width: containerWidth, height: containerHeight } =
       getUntransformedQuad(this.currentNode, "content").getBounds();
 
     this.setupCanvas({
       lineDash: FLEXBOX_LINES_PROPERTIES.alignItems.lineDash,
       offset: (getDisplayPixelRatio(this.win) / 2) % 1,
       skipLineAndStroke: true,
-      useScrollOffsets: true,
+      useContainerScrollOffsets: true,
     });
 
     for (const flexLine of this.flexData.lines) {
       const { crossStart, crossSize } = flexLine;
       let mainStart = 0;
 
       // In these two situations mainStart goes from right to left so set it's
       // value as appropriate.
@@ -743,39 +744,49 @@ class FlexboxHighlighter extends AutoRef
    *          FLEXBOX_LINES_PROPERTIES.item.lineDash
    *          FLEXBOX_LINES_PROPERTIES.alignItems.lineDash
    * @param {Number} [options.lineWidthMultiplier = 1]
    *        The width of the line.
    * @param {Number} [options.offset = `(displayPixelRatio / 2) % 1`]
    *        The single line width used to obtain a crisp line.
    * @param {Boolean} [options.skipLineAndStroke = false]
    *        Skip the setting of lineWidth and strokeStyle.
-   * @param {Boolean} [options.useScrollOffsets = false]
-   *        Take scroll and zoom offsets into account. This is often needed
-   *        when working purely with mainStart, mainSize, crossStart and
-   *        crossSize because they do not take the scroll position into account.
+   * @param {Boolean} [options.useContainerScrollOffsets = false]
+   *        Take the flexbox container's scroll and zoom offsets into account.
+   *        This is needed for drawing flex lines and justify content when the
+   *        flexbox container itself is display:scroll.
    */
   setupCanvas({
       lineDash = null,
       lineWidthMultiplier = 1,
       offset = (getDisplayPixelRatio(this.win) / 2) % 1,
       skipLineAndStroke = false,
-      useScrollOffsets = false }) {
+      useContainerScrollOffsets = false }) {
     const { devicePixelRatio } = this.win;
     const lineWidth = getDisplayPixelRatio(this.win);
     const zoom = getCurrentZoom(this.win);
-
+    const style = getComputedStyle(this.currentNode);
+    const position = style.position;
     let offsetX = this._canvasPosition.x;
     let offsetY = this._canvasPosition.y;
 
-    if (useScrollOffsets) {
+    if (useContainerScrollOffsets) {
       offsetX += this.currentNode.scrollLeft / zoom;
       offsetY += this.currentNode.scrollTop / zoom;
     }
 
+    // If the flexbox container is position:fixed we need to subtract the scroll
+    // positions of all ancestral elements.
+    if (position === "fixed") {
+      const { scrollLeft, scrollTop } =
+        getAbsoluteScrollOffsetsForNode(this.currentNode);
+      offsetX -= scrollLeft / zoom;
+      offsetY -= scrollTop / zoom;
+    }
+
     const canvasX = Math.round(offsetX * devicePixelRatio * zoom);
     const canvasY = Math.round(offsetY * devicePixelRatio * zoom);
 
     this.ctx.save();
     this.ctx.translate(offset - canvasX, offset - canvasY);
 
     if (lineDash) {
       this.ctx.setLineDash(lineDash);
--- a/devtools/shared/layout/utils.js
+++ b/devtools/shared/layout/utils.js
@@ -982,8 +982,53 @@ function isNodeAFlexItemInContainer(supp
         }
       }
     }
   }
 
   return false;
 }
 exports.isNodeAFlexItemInContainer = isNodeAFlexItemInContainer;
+
+/**
+ * Calculate the total of the node and all of its ancestor's scrollTop and
+ * scrollLeft values.
+ *
+ * @param  {DOMNode} node
+ *         The node for which the absolute scroll offsets should be calculated.
+ * @return {Object} object
+ *         An object containing scrollTop and scrollLeft values.
+ * @return {Number} object.scrollLeft
+ *         The total scrollLeft values of the node and all of its ancestors.
+ * @return {Number} object.scrollTop
+ *         The total scrollTop values of the node and all of its ancestors.
+ */
+function getAbsoluteScrollOffsetsForNode(node) {
+  const doc = node.ownerDocument;
+
+  // Our walker will only iterate up to document.body so we start by saving the
+  // scroll values for `document.documentElement`.
+  let scrollTop = doc.documentElement.scrollTop;
+  let scrollLeft = doc.documentElement.scrollLeft;
+  const walker = doc.createTreeWalker(doc.body, NodeFilter.SHOW_ELEMENT);
+  walker.currentNode = node;
+  let currentNode = walker.currentNode;
+
+  // Iterate from `node` up the tree to `document.body` adding scroll offsets
+  // as we go.
+  while (currentNode) {
+    const nodeScrollTop = currentNode.scrollTop;
+    const nodeScrollLeft = currentNode.scrollLeft;
+
+    if (nodeScrollTop || nodeScrollLeft) {
+      scrollTop += nodeScrollTop;
+      scrollLeft += nodeScrollLeft;
+    }
+
+    currentNode = walker.parentNode();
+  }
+
+  return {
+    scrollLeft,
+    scrollTop,
+  };
+}
+exports.getAbsoluteScrollOffsetsForNode = getAbsoluteScrollOffsetsForNode;