Bug 1074165 - Prevent out of range scrolling in nsListboxBodyFrame. r=mats
☠☠ backed out by 3da679acadb8 ☠ ☠
authorKearwood (Kip) Gilbert <kgilbert@mozilla.com>
Wed, 08 Oct 2014 17:14:00 +0200
changeset 234073 ccde95bd3c7e76a10ce5f192bbead2646463a847
parent 234072 c39759a6771e50912d89837adf6837152db0dced
child 234074 a24b22bc3fe5672634b841e7f0f08800d7d7a4f4
push id611
push userraliiev@mozilla.com
push dateMon, 05 Jan 2015 23:23:16 +0000
treeherdermozilla-release@345cd3b9c445 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1074165
milestone35.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 1074165 - Prevent out of range scrolling in nsListboxBodyFrame. r=mats - nsListBoxBodyFrame::UpdateIndex now uses the position returned by nsScrollBarFrame::MoveToNewPosition to calculate the new row position. - Code used to calculate the row position from the scroll position has been moved to a new function, ComputeNewIndex. - nsListBoxBodyFrame::ThumbMoved has been updated to use ComputeNewIndex.
layout/xul/nsListBoxBodyFrame.cpp
layout/xul/nsListBoxBodyFrame.h
layout/xul/nsScrollbarFrame.h
--- a/layout/xul/nsListBoxBodyFrame.cpp
+++ b/layout/xul/nsListBoxBodyFrame.cpp
@@ -2,16 +2,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsListBoxBodyFrame.h"
 
 #include "nsListBoxLayout.h"
 
+#include "nsAlgorithm.h"
 #include "nsCOMPtr.h"
 #include "nsGridRowGroupLayout.h"
 #include "nsIServiceManager.h"
 #include "nsGkAtoms.h"
 #include "nsIContent.h"
 #include "nsNameSpaceManager.h"
 #include "nsIDocument.h"
 #include "nsIDOMMouseEvent.h"
@@ -323,101 +324,107 @@ nsListBoxBodyFrame::GetPrefSize(nsBoxLay
 }
 
 ///////////// nsIScrollbarMediator ///////////////
 
 void
 nsListBoxBodyFrame::ScrollByPage(nsScrollbarFrame* aScrollbar, int32_t aDirection)
 {
   MOZ_ASSERT(aScrollbar != nullptr);
-  UpdateIndex(aDirection);
   aScrollbar->SetIncrementToPage(aDirection);
-  aScrollbar->MoveToNewPosition();
+  nsWeakFrame weakFrame(this);
+  int32_t newPos = aScrollbar->MoveToNewPosition();
+  if (!weakFrame.IsAlive()) {
+    return;
+  }
+  UpdateIndex(newPos);
 }
 
 void
 nsListBoxBodyFrame::ScrollByWhole(nsScrollbarFrame* aScrollbar, int32_t aDirection)
 {
-  MOZ_ASSERT(aScrollbar != nullptr); 
-  UpdateIndex(aDirection);
+  MOZ_ASSERT(aScrollbar != nullptr);
   aScrollbar->SetIncrementToWhole(aDirection);
-  aScrollbar->MoveToNewPosition();
+  nsWeakFrame weakFrame(this);
+  int32_t newPos = aScrollbar->MoveToNewPosition();
+  if (!weakFrame.IsAlive()) {
+    return;
+  }
+  UpdateIndex(newPos);
 }
 
 void
 nsListBoxBodyFrame::ScrollByLine(nsScrollbarFrame* aScrollbar, int32_t aDirection)
 {
-  MOZ_ASSERT(aScrollbar != nullptr); 
-  UpdateIndex(aDirection);
+  MOZ_ASSERT(aScrollbar != nullptr);
   aScrollbar->SetIncrementToLine(aDirection);
-  aScrollbar->MoveToNewPosition();
+  nsWeakFrame weakFrame(this);
+  int32_t newPos = aScrollbar->MoveToNewPosition();
+  if (!weakFrame.IsAlive()) {
+    return;
+  }
+  UpdateIndex(newPos);
 }
 
 void
 nsListBoxBodyFrame::RepeatButtonScroll(nsScrollbarFrame* aScrollbar)
 {
-  int32_t increment = aScrollbar->GetIncrement();
-  if (increment < 0) {
-    UpdateIndex(-1);
-  } else if (increment > 0) {
-    UpdateIndex(1);
+  nsWeakFrame weakFrame(this);
+  int32_t newPos = aScrollbar->MoveToNewPosition();
+  if (!weakFrame.IsAlive()) {
+    return;
   }
-  aScrollbar->MoveToNewPosition();
+  UpdateIndex(newPos);
+}
+
+int32_t
+nsListBoxBodyFrame::ToRowIndex(nscoord aPos) const
+{
+  return NS_roundf(float(std::max(aPos, 0)) / mRowHeight);
 }
 
 void
 nsListBoxBodyFrame::ThumbMoved(nsScrollbarFrame* aScrollbar,
                                nscoord aOldPos,
                                nscoord aNewPos)
 { 
   if (mScrolling || mRowHeight == 0)
     return;
 
-  nscoord oldTwipIndex;
-  oldTwipIndex = mCurrentIndex*mRowHeight;
-  int32_t twipDelta = aNewPos > oldTwipIndex ? aNewPos - oldTwipIndex : oldTwipIndex - aNewPos;
-
-  int32_t rowDelta = twipDelta / mRowHeight;
-  int32_t remainder = twipDelta % mRowHeight;
-  if (remainder > (mRowHeight/2))
-    rowDelta++;
-
-  if (rowDelta == 0)
+  int32_t newIndex = ToRowIndex(aNewPos);
+  if (newIndex == mCurrentIndex) {
     return;
-
-  // update the position to be row based.
-
-  int32_t newIndex = aNewPos > oldTwipIndex ? mCurrentIndex + rowDelta : mCurrentIndex - rowDelta;
-  //aNewIndex = newIndex*mRowHeight/mOnePixel;
+  }
+  int32_t rowDelta = newIndex - mCurrentIndex;
 
   nsListScrollSmoother* smoother = GetSmoother();
 
   // if we can't scroll the rows in time then start a timer. We will eat
   // events until the user stops moving and the timer stops.
-  if (smoother->IsRunning() || rowDelta*mTimePerRow > USER_TIME_THRESHOLD) {
+  if (smoother->IsRunning() || Abs(rowDelta)*mTimePerRow > USER_TIME_THRESHOLD) {
 
      smoother->Stop();
 
-     smoother->mDelta = aNewPos > oldTwipIndex ? rowDelta : -rowDelta;
+     smoother->mDelta = rowDelta;
 
      smoother->Start();
 
      return;
   }
 
   smoother->Stop();
 
   mCurrentIndex = newIndex;
   smoother->mDelta = 0;
   
   if (mCurrentIndex < 0) {
     mCurrentIndex = 0;
     return;
   }
-  InternalPositionChanged(aNewPos < oldTwipIndex, rowDelta);
+  InternalPositionChanged(rowDelta < 0, Abs(rowDelta));
 }
 
 void
 nsListBoxBodyFrame::VisibilityChanged(bool aVisible)
 {
   if (mRowHeight == 0)
     return;
 
@@ -434,28 +441,26 @@ nsListBoxBodyFrame::VisibilityChanged(bo
 nsIFrame*
 nsListBoxBodyFrame::GetScrollbarBox(bool aVertical)
 {
   nsIScrollableFrame* scrollFrame = nsLayoutUtils::GetScrollableFrameFor(this);
   return scrollFrame ? scrollFrame->GetScrollbarBox(true) : nullptr;
 }
 
 void
-nsListBoxBodyFrame::UpdateIndex(int32_t aDirection)
+nsListBoxBodyFrame::UpdateIndex(int32_t aNewPos)
 {
-  if (aDirection == 0)
-    return;
-  if (aDirection < 0)
-    mCurrentIndex--;
-  else mCurrentIndex++;
-  if (mCurrentIndex < 0) {
-    mCurrentIndex = 0;
+  int32_t newIndex = ToRowIndex(nsPresContext::CSSPixelsToAppUnits(aNewPos));
+  if (newIndex == mCurrentIndex) {
     return;
   }
-  InternalPositionChanged(aDirection < 0, 1);
+  bool up = newIndex < mCurrentIndex;
+  int32_t indexDelta = Abs(newIndex - mCurrentIndex);
+  mCurrentIndex = newIndex;
+  InternalPositionChanged(up, indexDelta);
 }
  
 ///////////// nsIReflowCallback ///////////////
 
 bool
 nsListBoxBodyFrame::ReflowFinished()
 {
   nsAutoScriptBlocker scriptBlocker;
--- a/layout/xul/nsListBoxBodyFrame.h
+++ b/layout/xul/nsListBoxBodyFrame.h
@@ -93,17 +93,18 @@ public:
   nsresult InternalPositionChanged(bool aUp, int32_t aDelta);
   // Process pending position changed events, then do the position change.
   // This can wipe out the frametree.
   nsresult DoInternalPositionChangedSync(bool aUp, int32_t aDelta);
   // Actually do the internal position change.  This can wipe out the frametree
   nsresult DoInternalPositionChanged(bool aUp, int32_t aDelta);
   nsListScrollSmoother* GetSmoother();
   void VerticalScroll(int32_t aDelta);
-  void UpdateIndex(int32_t aDirection);
+  // Update the scroll index given a position, in CSS pixels
+  void UpdateIndex(int32_t aNewPos);
 
   // frames
   nsIFrame* GetFirstFrame();
   nsIFrame* GetLastFrame();
 
   // lazy row creation and destruction
   void CreateRows();
   void DestroyRows(int32_t& aRowsToLose);
@@ -161,16 +162,17 @@ protected:
     }
 
     nsListBoxBodyFrame* mFrame;
     bool mUp;
     int32_t mDelta;
   };
 
   void ComputeTotalRowCount();
+  int32_t ToRowIndex(nscoord aPos) const;
   void RemoveChildFrame(nsBoxLayoutState &aState, nsIFrame *aChild);
 
   nsTArray< nsRefPtr<nsPositionChangedEvent> > mPendingPositionChangeEvents;
   nsCOMPtr<nsPIBoxObject> mBoxObject;
 
   // frame markers
   nsWeakFrame mTopFrame;
   nsIFrame* mBottomFrame;
--- a/layout/xul/nsScrollbarFrame.h
+++ b/layout/xul/nsScrollbarFrame.h
@@ -88,16 +88,17 @@ public:
    * scrollbar button is pressed.
    */
   void SetIncrementToLine(int32_t aDirection);
   void SetIncrementToPage(int32_t aDirection);
   void SetIncrementToWhole(int32_t aDirection);
   /**
    * MoveToNewPosition() adds mIncrement to the current position and
    * updates the curpos attribute.
+   * @returns The new position after clamping, in CSS Pixels
    * @note This method might destroy the frame, pres shell, and other objects.
    */
   int32_t MoveToNewPosition();
   int32_t GetIncrement() { return mIncrement; }
 
 protected:
   int32_t mIncrement; // Amount to scroll, in CSSPixels
   bool mSmoothScroll;