Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
☠☠ backed out by cd807be17132 ☠ ☠
authorDão Gottwald <dao@mozilla.com>
Wed, 31 May 2017 21:34:41 +0200
changeset 409745 4b82f58570e4b0763de9cdf088344fdf5d24e8fd
parent 409744 91a308b8cbbe6487ae10b980bd8d4ccca2dd003f
child 409746 c1a21b5a5175452c8cfc34461e932a05a41c96eb
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmconley
bugs1368208
milestone55.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 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley MozReview-Commit-ID: 1LG3GDBFArv
layout/reftests/bugs/508816-1-ref.xul
layout/reftests/bugs/508816-1.xul
toolkit/content/widgets/scrollbox.xml
--- a/layout/reftests/bugs/508816-1-ref.xul
+++ b/layout/reftests/bugs/508816-1-ref.xul
@@ -39,14 +39,20 @@
       var amountToScroll = 0;
       if (elementStart < containerStart) {
         amountToScroll = elementStart - containerStart;
       } else if (elementEnd > containerEnd) {
         amountToScroll = elementEnd - containerEnd;
       }
       sb.scrollLeft += amountToScroll;
 
-      document.documentElement.removeAttribute("class");
+      requestAnimationFrame(() => {
+        requestAnimationFrame(() => {
+          requestAnimationFrame(() => {
+            document.documentElement.removeAttribute("class");
+          });
+        });
+      });
     }
   ]]>
 </script>
 
 </window>
--- a/layout/reftests/bugs/508816-1.xul
+++ b/layout/reftests/bugs/508816-1.xul
@@ -1,11 +1,11 @@
 <?xml version="1.0"?>
 <!DOCTYPE window>
-<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" title="RTL overflow test" width="640px" height="480px" style="direction: rtl">
+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" title="RTL overflow test" width="640px" height="480px" style="direction: rtl" onload="load()" class="reftest-wait">
 
 <hbox style="max-width: 200px; border: 1px solid red;">
 <scrollbox align="start" pack="start" orient="horizontal" flex="1">
   <button label="test1"/><button label="test2"/><button label="test3"/><button label="test4"/><button label="test5"/>
 </scrollbox>
 </hbox>
 
 <hbox style="max-width: 200px; border: 1px solid blue">
@@ -15,9 +15,23 @@
 </hbox>
 
 <hbox style="max-width: 200px; border: 1px solid black">
 <hbox align="end" pack="end" flex="1">
   <button label="test1"/><button label="test2"/><button label="test3"/><button label="test4"/><button label="test5"/>
 </hbox>
 </hbox>
 
+<script type="text/javascript">
+  <![CDATA[
+    function load() {
+      requestAnimationFrame(() => {
+        requestAnimationFrame(() => {
+          requestAnimationFrame(() => {
+            document.documentElement.removeAttribute("class");
+          });
+        });
+      });
+    }
+  ]]>
+</script>
+
 </window>
--- a/toolkit/content/widgets/scrollbox.xml
+++ b/toolkit/content/widgets/scrollbox.xml
@@ -180,57 +180,85 @@
           var innerRect = {};
           innerRect.left = outerRect.left - this._scrollbox.scrollLeft;
           innerRect.top = outerRect.top - this._scrollbox.scrollTop;
           innerRect.right = innerRect.left + this._scrollbox.scrollWidth;
           innerRect.bottom = innerRect.top + this._scrollbox.scrollHeight;
           return innerRect;
         ]]></getter>
       </property>
-      <property name="scrollboxPaddingStart" readonly="true">
-        <getter><![CDATA[
-          var ltr = (window.getComputedStyle(this).direction == "ltr");
-          var paddingStartName = ltr ? "padding-left" : "padding-right";
-          var scrollboxStyle = window.getComputedStyle(this._scrollbox);
-          return parseFloat(scrollboxStyle.getPropertyValue(paddingStartName));
-        ]]></getter>
-      </property>
+      <field name="scrollboxPaddingStart"><![CDATA[
+        parseFloat(window.getComputedStyle(this._scrollbox)[
+          this._isRTLScrollbox ? "paddingRight" : "paddingLeft"
+        ]);
+      ]]></field>
+      <field name="scrollboxPaddingEnd"><![CDATA[
+        parseFloat(window.getComputedStyle(this._scrollbox)[
+          this._isRTLScrollbox ? "paddingLeft" : "paddingRight"
+        ]);
+      ]]></field>
       <property name="scrollPosition">
         <getter><![CDATA[
           return this.orient == "vertical" ?
                  this._scrollbox.scrollTop :
                  this._scrollbox.scrollLeft;
         ]]></getter>
         <setter><![CDATA[
           if (this.orient == "vertical")
             this._scrollbox.scrollTop = val;
           else
             this._scrollbox.scrollLeft = val;
           return val;
         ]]></setter>
       </property>
 
-      <property name="_startEndProps" readonly="true">
-        <getter><![CDATA[
-          return this.orient == "vertical" ?
-                 ["top", "bottom"] : ["left", "right"];
-        ]]></getter>
-      </property>
+      <field name="_startEndProps"><![CDATA[
+        this.orient == "vertical" ? ["top", "bottom"] : ["left", "right"];
+      ]]></field>
 
       <field name="_isRTLScrollbox"><![CDATA[
         this.orient != "vertical" &&
         document.defaultView.getComputedStyle(this._scrollbox).direction == "rtl";
       ]]></field>
 
       <field name="_scrollTarget">null</field>
 
+      <method name="_boundsWithoutFlushing">
+        <parameter name="element"/>
+        <body><![CDATA[
+          if (!("_DOMWindowUtils" in this)) {
+            try {
+              this._DOMWindowUtils =
+                window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
+                      .getInterface(Components.interfaces.nsIDOMWindowUtils);
+            } catch (e) {
+              // Can't access nsIDOMWindowUtils if we're unprivileged.
+              this._DOMWindowUtils = null;
+            }
+          }
+
+          return this._DOMWindowUtils ?
+                 this._DOMWindowUtils.getBoundsWithoutFlushing(element) :
+                 element.getBoundingClientRect();
+        ]]></body>
+      </method>
+
       <method name="_canScrollToElement">
         <parameter name="element"/>
         <body><![CDATA[
-          return window.getComputedStyle(element).display != "none";
+          if (element.hidden) {
+            return false;
+          }
+
+          // See if the element is hidden via CSS without the hidden attribute.
+          // If we get only zeros for the client rect, this means the element
+          // is hidden. As a performance optimization, we don't flush layout
+          // here which means that on the fly changes aren't fully supported.
+          let rect = this._boundsWithoutFlushing(element);
+          return !!(rect.top || rect.left || rect.width || rect.height);
         ]]></body>
       </method>
 
       <method name="ensureElementIsVisible">
         <parameter name="element"/>
         <parameter name="aSmoothScroll"/>
         <body><![CDATA[
           if (!this._canScrollToElement(element))
@@ -606,50 +634,78 @@
           if (this._isScrolling) {
             this._scrollAnim.stop();
             this._isScrolling = 0;
             this._scrollTarget = null;
           }
         ]]></body>
       </method>
 
+      <field name="_scrollButtonUpdatePending">false</field>
       <method name="_updateScrollButtonsDisabledState">
-        <parameter name="aScrolling"/>
         <body><![CDATA[
-          let scrolledToStart;
-          let scrolledToEnd;
-
-          // Avoid flushing layout when not overflowing or when scrolling.
           if (this.hasAttribute("notoverflowing")) {
-            scrolledToStart = true;
-            scrolledToEnd = true;
-          } else if (aScrolling) {
-            scrolledToStart = false;
-            scrolledToEnd = false;
-          } else if (this.scrollPosition == 0) {
-            // In the RTL case, this means the _last_ element in the
-            // scrollbox is visible
-            scrolledToEnd = this._isRTLScrollbox;
-            scrolledToStart = !this._isRTLScrollbox;
-          } else if (this.scrollClientSize + this.scrollPosition == this.scrollSize) {
-            // In the RTL case, this means the _first_ element in the
-            // scrollbox is visible
-            scrolledToStart = this._isRTLScrollbox;
-            scrolledToEnd = !this._isRTLScrollbox;
+            this.setAttribute("scrolledtoend", "true");
+            this.setAttribute("scrolledtostart", "true");
+            return;
           }
 
-          if (scrolledToEnd)
-            this.setAttribute("scrolledtoend", "true");
-          else
-            this.removeAttribute("scrolledtoend");
+          if (this._scrollButtonUpdatePending) {
+            return;
+          }
+          this._scrollButtonUpdatePending = true;
+
+          // Wait until after the next paint to get current layout data from
+          // getBoundsWithoutFlushing.
+          window.requestAnimationFrame(() => {
+            window.requestAnimationFrame(() => {
+              this._scrollButtonUpdatePending = false;
+
+              let scrolledToStart = false;
+              let scrolledToEnd = false;
+
+              if (this.hasAttribute("notoverflowing")) {
+                scrolledToStart = true;
+                scrolledToEnd = true;
+              } else {
+                let scrollboxPaddingStart = Math.round(this.scrollboxPaddingStart);
+                let scrollboxPaddingEnd = Math.round(this.scrollboxPaddingEnd);
+                let [start, end] = this._startEndProps;
+                if (this._isRTLScrollbox) {
+                  [start, end] = [end, start];
+                  scrollboxPaddingStart = -scrollboxPaddingStart;
+                  scrollboxPaddingEnd = -scrollboxPaddingEnd;
+                }
 
-          if (scrolledToStart)
-            this.setAttribute("scrolledtostart", "true");
-          else
-            this.removeAttribute("scrolledtostart");
+                let scrollboxRect = this._boundsWithoutFlushing(this._scrollbox);
+                let elements = this._getScrollableElements();
+                let [firstElement, lastElement] = [elements[0], elements[elements.length - 1]];
+
+                if (firstElement &&
+                    this._boundsWithoutFlushing(firstElement)[start] - scrollboxPaddingStart == scrollboxRect[start]) {
+                  scrolledToStart = true;
+                } else if (lastElement &&
+                           this._boundsWithoutFlushing(lastElement)[end] + scrollboxPaddingEnd == scrollboxRect[end]) {
+                  scrolledToEnd = true;
+                }
+              }
+
+              if (scrolledToEnd) {
+                this.setAttribute("scrolledtoend", "true");
+              } else {
+                this.removeAttribute("scrolledtoend");
+              }
+
+              if (scrolledToStart) {
+                this.setAttribute("scrolledtostart", "true");
+              } else {
+                this.removeAttribute("scrolledtostart");
+              }
+            });
+          });
         ]]></body>
       </method>
     </implementation>
 
     <handlers>
       <handler event="wheel"><![CDATA[
         let doScroll = false;
         let useSmoothScroll = event.deltaMode != event.DOM_DELTA_PIXEL && this.smoothScroll;
@@ -792,37 +848,17 @@
           // as the whole overflow event should not be happening in that case.
           this._updateScrollButtonsDisabledState();
         } catch (e) {
           this.setAttribute("notoverflowing", "true");
         }
       ]]></handler>
 
       <handler event="scroll"><![CDATA[
-        if (!this._delayedUpdateScrollButtonsTimer) {
-          // This is the beginning of a scrolling animation. We need to update
-          // scroll buttons now in case we were scrolled to the start or to the
-          // end before we started scrolling.
-          this._updateScrollButtonsDisabledState(true);
-        } else {
-          // We're in the middle of the scrolling animation. We'll restart the
-          // delayed update request so that we only update the scroll buttons
-          // a second time once we're done scrolling.
-          window.clearTimeout(this._delayedUpdateScrollButtonsTimer);
-        }
-
-        // Try to detect the end of the scrolling animation to update the
-        // scroll buttons again. To avoid false positives, this timeout needs
-        // to be big enough to account for intermediate frames that don't move
-        // the scroll position in case we're scrolling slowly.
-        this._delayedUpdateScrollButtonsTimer = setTimeout(() => {
-          // Scrolling animation has finished.
-          this._delayedUpdateScrollButtonsTimer = 0;
-          this._updateScrollButtonsDisabledState();
-        }, 200);
+        this._updateScrollButtonsDisabledState();
       ]]></handler>
     </handlers>
   </binding>
 
   <binding id="autorepeatbutton" extends="chrome://global/content/bindings/scrollbox.xml#scrollbox-base">
     <content repeat="hover">
       <xul:image class="autorepeatbutton-icon"/>
     </content>