Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled draft
authorDão Gottwald <dao@mozilla.com>
Wed, 21 Jun 2017 15:08:30 +0200
changeset 598171 c7e5a765ac10899f39b31cb19943a3a685e93662
parent 598062 e49151136658c0d0f79b522727b807fd6cd8a79e
child 634425 906a6ac307ff178775c46ed51916929beb2bb429
push id65150
push userdgottwald@mozilla.com
push dateWed, 21 Jun 2017 13:08:55 +0000
bugs1368208
milestone56.0a1
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled MozReview-Commit-ID: HcOQ4W0Z4YP
layout/reftests/bugs/508816-1-ref.xul
layout/reftests/bugs/508816-1.xul
layout/reftests/bugs/reftest.list
toolkit/content/widgets/scrollbox.xml
--- a/layout/reftests/bugs/508816-1-ref.xul
+++ b/layout/reftests/bugs/508816-1-ref.xul
@@ -1,52 +1,40 @@
 <?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" onload="load()" class="reftest-wait">
 
+<style type="text/css" xmlns="http://www.w3.org/1999/xhtml"><![CDATA[
+button {
+  margin: 0;
+}
+]]></style>
+
 <hbox style="max-width: 200px; border: 1px solid red;">
-<scrollbox id="sb" align="start" pack="start" orient="horizontal" flex="1">
+<scrollbox align="start" pack="start" orient="horizontal" flex="1">
   <button id="sb1" 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">
-<arrowscrollbox id="asb" align="start" pack="start" orient="horizontal" flex="1">
+<arrowscrollbox align="start" pack="start" orient="horizontal" flex="1">
   <button id="asb1" label="test1"/><button label="test2"/><button label="test3"/><button label="test4"/><button label="test5"/>
 </arrowscrollbox>
 </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() {
-      // arrowscrollbox provides a helper ensureElementIsVisible method
-      document.getElementById("asb").ensureElementIsVisible(document.getElementById("asb1"), false);
-
-      // scrollbox doesn't provide that, so we have to do the heavy lifting directly
-      var sb = document.getElementById("sb");
-      var sb1 = document.getElementById("sb1");
-      var rect = sb.getBoundingClientRect();
-      var containerStart = rect.left;
-      var containerEnd = rect.right;
-      rect = sb1.getBoundingClientRect();
-      var elementStart = rect.left;
-      var elementEnd = rect.right;
-
-      var amountToScroll = 0;
-      if (elementStart < containerStart) {
-        amountToScroll = elementStart - containerStart;
-      } else if (elementEnd > containerEnd) {
-        amountToScroll = elementEnd - containerEnd;
-      }
-      sb.scrollLeft += amountToScroll;
+      document.getElementById("sb1").scrollIntoView({ behavior: "instant" });
+      document.getElementById("asb1").scrollIntoView({ behavior: "instant" });
 
       document.documentElement.removeAttribute("class");
     }
   ]]>
 </script>
 
 </window>
--- a/layout/reftests/bugs/508816-1.xul
+++ b/layout/reftests/bugs/508816-1.xul
@@ -1,23 +1,29 @@
-<?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">
-
-<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">
-<arrowscrollbox align="start" pack="start" orient="horizontal" flex="1">
-  <button label="test1"/><button label="test2"/><button label="test3"/><button label="test4"/><button label="test5"/>
-</arrowscrollbox>
-</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>
-
-</window>
+<?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">
+
+<style type="text/css" xmlns="http://www.w3.org/1999/xhtml"><![CDATA[
+button {
+  margin: 0;
+}
+]]></style>
+
+<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">
+<arrowscrollbox align="start" pack="start" orient="horizontal" flex="1">
+  <button label="test1"/><button label="test2"/><button label="test3"/><button label="test4"/><button label="test5"/>
+</arrowscrollbox>
+</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>
+
+</window>
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -1406,17 +1406,17 @@ needs-focus fails-if(!styloVsGecko) == 5
 fuzzy-if(Android,5,2800) == 506481-1.html 506481-1-ref.html
 == 507187-1.html 507187-1-ref.html
 == 507487-1.html 507487-1-ref.html
 == 507487-2.xhtml 507487-2-ref.xhtml
 == 507762-1.html 507762-1-ref.html
 == 507762-2.html 507762-2-ref.html
 == 507762-3.html 507762-1-ref.html
 == 507762-4.html 507762-2-ref.html
-random-if(cocoaWidget) == 508816-1.xul 508816-1-ref.xul # Bug 631982
+random-if(cocoaWidget||(gtkWidget&&webrender)||winWidget) == 508816-1.xul 508816-1-ref.xul # Bug 631982 and bug 1375012
 == 508816-2.html 508816-2-ref.html
 == 508908-1.xul 508908-1-ref.xul
 == 508919-1.xhtml 508919-1-ref.xhtml
 == 509155-1.xhtml 509155-1-ref.xhtml
 fuzzy-if(Android,5,1656) fuzzy-if(skiaContent,1,1200) == 512410.html 512410-ref.html
 == 512631-1.html 512631-1-ref.html
 == 513153-1a.html 513153-1-ref.html
 == 513153-1b.html 513153-1-ref.html
--- a/toolkit/content/widgets/scrollbox.xml
+++ b/toolkit/content/widgets/scrollbox.xml
@@ -185,16 +185,21 @@
           return innerRect;
         ]]></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")
@@ -629,50 +634,80 @@
           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(() => {
+            setTimeout(() => {
+              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 &&
+                    Math.round(this._boundsWithoutFlushing(firstElement)[start]) - scrollboxPaddingStart ==
+                      Math.round(scrollboxRect[start])) {
+                  scrolledToStart = true;
+                } else if (lastElement &&
+                           Math.round(this._boundsWithoutFlushing(lastElement)[end]) + scrollboxPaddingEnd ==
+                             Math.round(scrollboxRect[end])) {
+                  scrolledToEnd = true;
+                }
+              }
+
+              if (scrolledToEnd) {
+                this.setAttribute("scrolledtoend", "true");
+              } else {
+                this.removeAttribute("scrolledtoend");
+              }
+
+              if (scrolledToStart) {
+                this.setAttribute("scrolledtostart", "true");
+              } else {
+                this.removeAttribute("scrolledtostart");
+              }
+            }, 0);
+          });
         ]]></body>
       </method>
     </implementation>
 
     <handlers>
       <handler event="wheel"><![CDATA[
         let doScroll = false;
         let useSmoothScroll = event.deltaMode != event.DOM_DELTA_PIXEL && this.smoothScroll;
@@ -815,37 +850,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>