Bug 1127423 - Don't scroll horizontally when selecting an element in markup view;r=jryans
☠☠ backed out by aa4eac3a4336 ☠ ☠
authorBrian Grinstead <bgrinstead@mozilla.com>
Fri, 08 May 2015 20:39:23 -0700
changeset 274536 f407ab8f71292ca63ef6dc644cd6a73b1dd04640
parent 274535 44cfa341a8d3eaf76c622eb474f8269174e6c83e
child 274537 368ba4f292bc0ece70e3afc3f17fde1fd258f1f8
push id863
push userraliiev@mozilla.com
push dateMon, 03 Aug 2015 13:22:43 +0000
treeherdermozilla-release@f6321b14228d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjryans
bugs1127423
milestone40.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 1127423 - Don't scroll horizontally when selecting an element in markup view;r=jryans
browser/devtools/markupview/markup-view.js
browser/devtools/shared/test/browser.ini
browser/devtools/shared/test/browser_layoutHelpers.html
browser/devtools/shared/test/browser_layoutHelpers.js
browser/devtools/shared/test/browser_layoutHelpers_iframe.html
toolkit/devtools/LayoutHelpers.jsm
--- a/browser/devtools/markupview/markup-view.js
+++ b/browser/devtools/markupview/markup-view.js
@@ -145,17 +145,17 @@ MarkupView.prototype = {
     this._onMouseMove = this._onMouseMove.bind(this);
     this._elt.addEventListener("mousemove", this._onMouseMove, false);
     this._onMouseLeave = this._onMouseLeave.bind(this);
     this._elt.addEventListener("mouseleave", this._onMouseLeave, false);
 
     // Show markup-containers as hovered on toolbox "picker-node-hovered" event
     // which happens when the "pick" button is pressed
     this._onToolboxPickerHover = (event, nodeFront) => {
-      this.showNode(nodeFront, true).then(() => {
+      this.showNode(nodeFront).then(() => {
         this._showContainerAsHovered(nodeFront);
       });
     };
     this._inspector.toolbox.on("picker-node-hovered", this._onToolboxPickerHover);
   },
 
   _makeTooltipPersistent: function(state) {
     if (state) {
@@ -431,17 +431,17 @@ MarkupView.prototype = {
     }
 
     let done = this._inspector.updating("markup-view");
     if (selection.isNode()) {
       if (this._shouldNewSelectionBeHighlighted()) {
         this._brieflyShowBoxModel(selection.nodeFront);
       }
 
-      this.showNode(selection.nodeFront, true).then(() => {
+      this.showNode(selection.nodeFront).then(() => {
         if (this._destroyer) {
           return promise.reject("markupview destroyed");
         }
         if (selection.reason !== "treepanel") {
           this.markNodeAsSelected(selection.nodeFront);
         }
         done();
       }).then(null, e => {
@@ -831,33 +831,32 @@ MarkupView.prototype = {
       container.flashMutation();
     }
   },
 
   /**
    * Make sure the given node's parents are expanded and the
    * node is scrolled on to screen.
    */
-  showNode: function(aNode, centered) {
+  showNode: function(aNode, centered=true) {
     let parent = aNode;
 
     this.importNode(aNode);
 
     while ((parent = parent.parentNode())) {
       this.importNode(parent);
       this.expandNode(parent);
     }
 
     return this._waitForChildren().then(() => {
       if (this._destroyer) {
         return promise.reject("markupview destroyed");
       }
       return this._ensureVisible(aNode);
     }).then(() => {
-      // Why is this not working?
       this.layoutHelpers.scrollIntoViewIfNeeded(this.getContainer(aNode).editor.elt, centered);
     }, e => {
       // Only report this rejection as an error if the panel hasn't been
       // destroyed in the meantime.
       if (!this._destroyer) {
         console.error(e);
       } else {
         console.warn("Could not show the node, the markup-view was destroyed " +
--- a/browser/devtools/shared/test/browser.ini
+++ b/browser/devtools/shared/test/browser.ini
@@ -1,15 +1,14 @@
 [DEFAULT]
 tags = devtools
 subsuite = devtools
 support-files =
   browser_layoutHelpers.html
   browser_layoutHelpers-getBoxQuads.html
-  browser_layoutHelpers_iframe.html
   browser_templater_basic.html
   browser_toolbar_basic.html
   browser_toolbar_webconsole_errors_count.html
   browser_devices.json
   doc_options-view.xul
   head.js
   html-mdn-css-basic-testing.html
   html-mdn-css-no-summary.html
--- a/browser/devtools/shared/test/browser_layoutHelpers.html
+++ b/browser/devtools/shared/test/browser_layoutHelpers.html
@@ -17,9 +17,8 @@
     position: absolute;
     width: 40px;
     height: 40px;
     border: 0;
   }
 </style>
 
 <div id=some></div>
-<iframe id=frame src='./browser_layoutHelpers_iframe.html'></iframe>
--- a/browser/devtools/shared/test/browser_layoutHelpers.js
+++ b/browser/devtools/shared/test/browser_layoutHelpers.js
@@ -1,101 +1,86 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 // Tests that scrollIntoViewIfNeeded works properly.
+let {LayoutHelpers} = Cu.import("resource://gre/modules/devtools/LayoutHelpers.jsm", {});
 
-let imported = {};
-Components.utils.import("resource://gre/modules/devtools/LayoutHelpers.jsm",
-    imported);
-registerCleanupFunction(function () {
-  imported = {};
-});
-
-let LayoutHelpers = imported.LayoutHelpers;
 
 const TEST_URI = TEST_URI_ROOT + "browser_layoutHelpers.html";
 
-function test() {
-  addTab(TEST_URI, function(browser, tab) {
-    info("Starting browser_layoutHelpers.js");
-    let doc = browser.contentDocument;
-    runTest(doc.defaultView, doc.getElementById('some'));
-    gBrowser.removeCurrentTab();
-    finish();
-  });
-}
+add_task(function*() {
+  let browser = yield promiseTab(TEST_URI);
+  let doc = browser.contentDocument;
+  yield runTest(doc.defaultView);
+  gBrowser.removeCurrentTab();
+});
 
-function runTest(win, some) {
+function runTest(win) {
   let lh = new LayoutHelpers(win);
+  let some = win.document.getElementById('some');
 
   some.style.top = win.innerHeight + 'px';
   some.style.left = win.innerWidth + 'px';
   // The tests start with a black 2x2 pixels square below bottom right.
   // Do not resize the window during the tests.
 
-  win.scroll(win.innerWidth / 2, win.innerHeight + 2);  // Above the viewport.
+  let xPos = Math.floor(win.innerWidth / 2);
+  win.scroll(xPos, win.innerHeight + 2);  // Above the viewport.
   lh.scrollIntoViewIfNeeded(some);
   is(win.scrollY, Math.floor(win.innerHeight / 2) + 1,
      'Element completely hidden above should appear centered.');
+  is(win.scrollX, xPos,
+     'scrollX position has not changed.');
 
   win.scroll(win.innerWidth / 2, win.innerHeight + 1);  // On the top edge.
   lh.scrollIntoViewIfNeeded(some);
   is(win.scrollY, win.innerHeight,
      'Element partially visible above should appear above.');
+  is(win.scrollX, xPos,
+     'scrollX position has not changed.');
 
   win.scroll(win.innerWidth / 2, 0);  // Just below the viewport.
   lh.scrollIntoViewIfNeeded(some);
   is(win.scrollY, Math.floor(win.innerHeight / 2) + 1,
      'Element completely hidden below should appear centered.');
+  is(win.scrollX, xPos,
+     'scrollX position has not changed.');
 
   win.scroll(win.innerWidth / 2, 1);  // On the bottom edge.
   lh.scrollIntoViewIfNeeded(some);
   is(win.scrollY, 2,
      'Element partially visible below should appear below.');
-
+  is(win.scrollX, xPos,
+     'scrollX position has not changed.');
 
   win.scroll(win.innerWidth / 2, win.innerHeight + 2);  // Above the viewport.
   lh.scrollIntoViewIfNeeded(some, false);
   is(win.scrollY, win.innerHeight,
      'Element completely hidden above should appear above ' +
      'if parameter is false.');
+  is(win.scrollX, xPos,
+     'scrollX position has not changed.');
 
   win.scroll(win.innerWidth / 2, win.innerHeight + 1);  // On the top edge.
   lh.scrollIntoViewIfNeeded(some, false);
   is(win.scrollY, win.innerHeight,
      'Element partially visible above should appear above ' +
      'if parameter is false.');
+  is(win.scrollX, xPos,
+     'scrollX position has not changed.');
 
   win.scroll(win.innerWidth / 2, 0);  // Below the viewport.
   lh.scrollIntoViewIfNeeded(some, false);
   is(win.scrollY, 2,
      'Element completely hidden below should appear below ' +
      'if parameter is false.');
+  is(win.scrollX, xPos,
+     'scrollX position has not changed.');
 
   win.scroll(win.innerWidth / 2, 1);  // On the bottom edge.
   lh.scrollIntoViewIfNeeded(some, false);
   is(win.scrollY, 2,
      'Element partially visible below should appear below ' +
      'if parameter is false.');
-
-  // The case of iframes.
-  win.scroll(0, 0);
-
-  let frame = win.document.getElementById('frame');
-  let fwin = frame.contentWindow;
-
-  frame.style.top = win.innerHeight + 'px';
-  frame.style.left = win.innerWidth + 'px';
-
-  fwin.addEventListener('load', function frameLoad() {
-    let some = fwin.document.getElementById('some');
-    lh.scrollIntoViewIfNeeded(some);
-    is(win.scrollX, Math.floor(win.innerWidth / 2) + 20,
-       'Scrolling from an iframe should center the iframe vertically.');
-    is(win.scrollY, Math.floor(win.innerHeight / 2) + 20,
-       'Scrolling from an iframe should center the iframe horizontally.');
-    is(fwin.scrollX, Math.floor(fwin.innerWidth / 2) + 1,
-       'Scrolling from an iframe should center the element vertically.');
-    is(fwin.scrollY, Math.floor(fwin.innerHeight / 2) + 1,
-       'Scrolling from an iframe should center the element horizontally.');
-  }, false);
+  is(win.scrollX, xPos,
+     'scrollX position has not changed.');
 }
deleted file mode 100644
--- a/browser/devtools/shared/test/browser_layoutHelpers_iframe.html
+++ /dev/null
@@ -1,19 +0,0 @@
-<!doctype html>
-<meta charset=utf-8>
-<title> Layout Helpers </title>
-
-<style>
-  html {
-    height: 300%;
-    width: 300%;
-  }
-  div#some {
-    position: absolute;
-    background: black;
-    width: 2px;
-    height: 2px;
-  }
-</style>
-
-<div id=some></div>
-
--- a/toolkit/devtools/LayoutHelpers.jsm
+++ b/toolkit/devtools/LayoutHelpers.jsm
@@ -211,92 +211,62 @@ LayoutHelpers.prototype = {
           node = subnode;
         }
       }
     }
     return node;
   },
 
   /**
-   * Scroll the document so that the element "elem" appears in the viewport.
+   * Scroll the document so that the element "elem" appears vertically in
+   * the viewport.
    *
    * @param {DOMNode} elem
    *        The element that needs to appear in the viewport.
    * @param {Boolean} centered
    *        true if you want it centered, false if you want it to appear on the
-   *        top of the viewport. It is true by default, and that is usually what
+   *        top of the viewport. True by default, and that is usually what
    *        you want.
    */
   scrollIntoViewIfNeeded: function(elem, centered) {
     // We want to default to centering the element in the page,
     // so as to keep the context of the element.
-    centered = centered === undefined? true: !!centered;
+    centered = centered === undefined ? true: !!centered;
 
     let win = elem.ownerDocument.defaultView;
     let clientRect = elem.getBoundingClientRect();
 
-    // The following are always from the {top, bottom, left, right}
+    // The following are always from the {top, bottom}
     // of the viewport, to the {top, …} of the box.
     // Think of them as geometrical vectors, it helps.
     // The origin is at the top left.
 
     let topToBottom = clientRect.bottom;
     let bottomToTop = clientRect.top - win.innerHeight;
-    let leftToRight = clientRect.right;
-    let rightToLeft = clientRect.left - win.innerWidth;
-    let xAllowed = true;  // We allow one translation on the x axis,
-    let yAllowed = true;  // and one on the y axis.
+    let yAllowed = true;  // We allow one translation on the y axis.
 
     // Whatever `centered` is, the behavior is the same if the box is
     // (even partially) visible.
-
     if ((topToBottom > 0 || !centered) && topToBottom <= elem.offsetHeight) {
       win.scrollBy(0, topToBottom - elem.offsetHeight);
       yAllowed = false;
     } else
     if ((bottomToTop < 0 || !centered) && bottomToTop >= -elem.offsetHeight) {
       win.scrollBy(0, bottomToTop + elem.offsetHeight);
       yAllowed = false;
     }
 
-    if ((leftToRight > 0 || !centered) && leftToRight <= elem.offsetWidth) {
-      if (xAllowed) {
-        win.scrollBy(leftToRight - elem.offsetWidth, 0);
-        xAllowed = false;
-      }
-    } else
-    if ((rightToLeft < 0 || !centered) && rightToLeft >= -elem.offsetWidth) {
-      if (xAllowed) {
-        win.scrollBy(rightToLeft + elem.offsetWidth, 0);
-        xAllowed = false;
-      }
-    }
-
     // If we want it centered, and the box is completely hidden,
     // then we center it explicitly.
-
     if (centered) {
-
       if (yAllowed && (topToBottom <= 0 || bottomToTop >= 0)) {
         win.scroll(win.scrollX,
                    win.scrollY + clientRect.top
                    - (win.innerHeight - elem.offsetHeight) / 2);
       }
-
-      if (xAllowed && (leftToRight <= 0 || rightToLeft <= 0)) {
-        win.scroll(win.scrollX + clientRect.left
-                   - (win.innerWidth - elem.offsetWidth) / 2,
-                   win.scrollY);
-      }
-    }
-
-    if (!this.isTopLevelWindow(win)) {
-      // We are inside an iframe.
-      let frameElement = this.getFrameElement(win);
-      this.scrollIntoViewIfNeeded(frameElement, centered);
     }
   },
 
   /**
    * Check if a node and its document are still alive
    * and attached to the window.
    *
    * @param {DOMNode} aNode