Bug 1074165 - Prevent out of range scrolling in nsListboxBodyFrame. r=mats, a=lmandel
authorKearwood (Kip) Gilbert <kgilbert@mozilla.com>
Thu, 09 Oct 2014 12:14:26 +0000
changeset 225767 9d9abce3b2f2
parent 225766 4ff961ace0d0
child 225768 25b64ba60455
push id4011
push userryanvm@gmail.com
push date2014-10-22 22:03 +0000
treeherdermozilla-beta@65f5bf99d815 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats, lmandel
bugs1074165
milestone34.0
Bug 1074165 - Prevent out of range scrolling in nsListboxBodyFrame. r=mats, a=lmandel - 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, ToRowIndex. - nsListBoxBodyFrame::ThumbMoved has been updated to use ToRowIndex.
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 "mozilla/MathAlgorithms.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"
@@ -35,16 +36,17 @@
 #include "nsRenderingContext.h"
 #include "prtime.h"
 #include <algorithm>
 
 #ifdef ACCESSIBILITY
 #include "nsAccessibilityService.h"
 #endif
 
+using namespace mozilla;
 using namespace mozilla::dom;
 
 /////////////// nsListScrollSmoother //////////////////
 
 /* A mediator used to smooth out scrolling. It works by seeing if 
  * we have time to scroll the amount of rows requested. This is determined
  * by measuring how long it takes to scroll a row. If we can scroll the 
  * rows in time we do so. If not we start a timer and skip the request. We
@@ -323,101 +325,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 +442,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;