Closed Bug 1074165 Opened 10 years ago Closed 10 years ago

XUL listbox becomes empty when scroll down by mouse down on scroll arrow

Categories

(Core :: XUL, defect)

34 Branch
x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 + fixed
firefox35 + fixed

People

(Reporter: alice0775, Assigned: kip)

References

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

Attached file listbox.xul
[Tracking Requested - why for this release]:Regression

Steps To Reproduce:
1. Open attached XUL
2. Click and hold mouse down on scroll arrow

Actual Results:
XUL listbox becomes empty


Regression window(m-i)
Good(but enexpected scrollbar apperas):
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec4622e6e1fa
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140822111353
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef03db1385b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140822113357
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ec4622e6e1fa&tochange=4ef03db1385b

Regressed by: Bug 957445
Summary: XUL listbox becomes empty when scroll down by mouse down → XUL listbox becomes empty when scroll down by mouse down on scroll arrow
Attached image screenshot
Reproduced on Windows7 and Ubuntu12.04
OS: Windows 7 → All
See Also: → 491788
The bug #1066459 fix the bug in tree like the issue in this bug.
The offending Bug 957445 should be backed out before uplift 34Beta.
Flags: needinfo?(kgilbert)
(In reply to Alice0775 White from comment #4)
> The offending Bug 957445 should be backed out before uplift 34Beta.
I am reviewing the issue now.  I hope that there will be a quick fix, as backing out Bug 957445 will block a significant amount of work.
Flags: needinfo?(kgilbert)
(In reply to :kip (Kearwood Gilbert) from comment #5)
> (In reply to Alice0775 White from comment #4)
> > The offending Bug 957445 should be backed out before uplift 34Beta.
> I am reviewing the issue now.  I hope that there will be a quick fix, as
> backing out Bug 957445 will block a significant amount of work.

Bug 1010538 (Implement CSSOM-View scroll-behavior CSS property) is scheduled for 34Beta and will be unable to land if Bug 957445 is backed out.
Assignee: nobody → kgilbert
I have reproduced the issue on Ubuntu 14.04.1 LTS
- Added an out-of-range check to nsListBoxBodyFrame::UpdateIndex to prevent
  scrollbar activity, such as scroll down button repeating, from scrolling
  beyond the end of the list.
Comment on attachment 8499212 [details] [diff] [review]
Bug 1074165 - Prevent out of range scrolling in nsListboxBodyFrame

Hi Mats,

Would you have some extra cycles to review this?  Also, please advise if a test would be required.

- Kearwood "Kip" Gilbert
Attachment #8499212 - Flags: review?(mats)
Comment on attachment 8499212 [details] [diff] [review]
Bug 1074165 - Prevent out of range scrolling in nsListboxBodyFrame

On Aurora (after bug 957445), we have:
nsListBoxBodyFrame::UpdateIndex(int32_t aDirection)
{
  if (aDirection == 0)
    return;
  if (aDirection < 0)
    mCurrentIndex--;
  else mCurrentIndex++;
  if (mCurrentIndex < 0) {
    mCurrentIndex = 0;
    return;
  }
  InternalPositionChanged(aDirection < 0, 1);
}

which corresponds to ScrollbarButtonPressed, on Beta (before bug 957445):
nsListBoxBodyFrame::ScrollbarButtonPressed(
  nsScrollbarFrame* aScrollbar, int32_t aOldIndex, int32_t aNewIndex)
{
  if (aOldIndex == aNewIndex)
    return NS_OK;
  if (aNewIndex < aOldIndex)
    mCurrentIndex--;
  else mCurrentIndex++;
  if (mCurrentIndex < 0) {
    mCurrentIndex = 0;
    return NS_OK;
  }
  InternalPositionChanged(aNewIndex < aOldIndex, 1);

  return NS_OK;
}

Those are pretty much the same thing, and I looked at all the code
related to InternalPositionChanged on both branches and they are
identical... so why is this fix needed?  It seems to me the error
must be higher up in the call chain. (btw, XUL trees has the same
problem, see bug 1066459).

So, holding down the scroll button, on Beta, the stack is:
(gdb) bt
#0  nsListBoxBodyFrame::ScrollbarButtonPressed
#1  nsScrollbarButtonFrame::DoButtonAction
#2  nsScrollbarButtonFrame::HandleButtonPress
#3  nsScrollbarButtonFrame::HandleEvent
...

Adding some printfs:
nsScrollbarButtonFrame::DoButtonAction oldpos=0 curpos=45 maxpos=0 mIncrement=45=> 
 clamped curpos=0
nsListBoxBodyFrame::ScrollbarButtonPressed 0 0
nsScrollbarButtonFrame::DoButtonAction oldpos=0 curpos=45 maxpos=0 mIncrement=45=> 
 clamped curpos=0
...

On Aurora we have:
(gdb) bt
#0  nsListBoxBodyFrame::UpdateIndex
#1  nsListBoxBodyFrame::ScrollByLine
#2  nsScrollbarButtonFrame::HandleButtonPress
#3  nsScrollbarButtonFrame::HandleEvent

With a printf:
nsListBoxBodyFrame::UpdateIndex 1
nsListBoxBodyFrame::UpdateIndex 1
nsListBoxBodyFrame::UpdateIndex 1
...

So on Beta, the clamping we have there is effective:
http://mxr.mozilla.org/mozilla-beta/source/layout/xul/nsScrollbarButtonFrame.cpp?rev=1b5ee8c5491a&mark=226-230,236#203
because it occurs *before* the ScrollbarButtonPressed call.

On Aurora that code now lives in nsScrollbarFrame::MoveToNewPosition():
http://mxr.mozilla.org/mozilla-aurora/source/layout/xul/nsScrollbarFrame.cpp?rev=249dae95ea83&mark=251-256#235
but that occurs *after* the UpdateIndex call:
http://mxr.mozilla.org/mozilla-aurora/source/layout/xul/nsListBoxBodyFrame.cpp?rev=9d4e083655b9&mark=349-351#328

So I think we should instead move the UpdateIndex call last in those
methods, and use the clamped result from MoveToNewPosition.
Something like this:
  int32_t oldPos = mCurrentIndex;
  int32_t newPos = aScrollbar->MoveToNewPosition();
  UpdateIndex(oldPos, newPos);

and change UpdateIndex() back to what ScrollbarButtonPressed used to do.

(perhaps just UpdateIndex(newPos) and let it use mCurrentIndex directly?)

Or is there a better way?
Attachment #8499212 - Flags: review?(mats) → review-
BTW, it's a bit odd to have a scrollbar here in the first place and then
have it disappear when you click the scrollbar down button.
(This isn't a regression, we've always done that.)

It think the scrollbar should either be static (not disappear),
or it shouldn't be there at all in this testcase.
s/It/I/
- 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 udpated to to ComputeNewIndex.


V2 Patch:

- Please advise if this is a better approach, as per Comment 10
Attachment #8499212 - Attachment is obsolete: true
Attachment #8500782 - Flags: review?(mats)
On OSX, there is a side effect in scrollbar animation while dragging.  Prior to this patch, the scroll bar would track the mouse pixel-by-pixel and flicker when the list box scrolls from one line to another.

With the patch applied, the scroll bar will no longer flicker; however, it will "step" through each line of the list box.  This is most noticeable when there are a small number of items in the list box.

Which behavior is the desired one?  Should it be kept smooth with the flickering?  Should we take effort to keep the scroll bar handle smoothly animated while dragging without the flickers or is the stepped scroll preferable?
I can't seem to reproduce that problem; how many total items do you have in the list
and how many are visible? are you using an externally attached mouse device?
(in case it matters for some weird reason)

It sounds like the behavior without the flicker would be best, i.e. with your patch.

The only problem I see on OSX is that click-and-hold on the scrollbar background
flickers horribly bad when it scrolls.  But this problem occurs with or without
your patch.   It doesn't occur in 32.0.3 though, so it might be another regression
from bug 957445.  (this problem doesn't occur on Linux though)
(In reply to Mats Palmgren (:mats) from comment #16)
> I can't seem to reproduce that problem; how many total items do you have in
> the list
> and how many are visible? are you using an externally attached mouse device?
> (in case it matters for some weird reason)
> 
> It sounds like the behavior without the flicker would be best, i.e. with
> your patch.
> 
> The only problem I see on OSX is that click-and-hold on the scrollbar
> background
> flickers horribly bad when it scrolls.  But this problem occurs with or
> without
> your patch.   It doesn't occur in 32.0.3 though, so it might be another
> regression
> from bug 957445.  (this problem doesn't occur on Linux though)
I modified the listbox.xul attached to this bug to have 5 visible rows and 10 items.

I was using an external, Apple "Magic Mouse", while dragging on the scroll handle.
(In reply to :kip (Kearwood Gilbert) from comment #17)
> (In reply to Mats Palmgren (:mats) from comment #16)
> > I can't seem to reproduce that problem; how many total items do you have in
> > the list
> > and how many are visible? are you using an externally attached mouse device?
> > (in case it matters for some weird reason)
> > 
> > It sounds like the behavior without the flicker would be best, i.e. with
> > your patch.
> > 
> > The only problem I see on OSX is that click-and-hold on the scrollbar
> > background
> > flickers horribly bad when it scrolls.  But this problem occurs with or
> > without
> > your patch.   It doesn't occur in 32.0.3 though, so it might be another
> > regression
> > from bug 957445.  (this problem doesn't occur on Linux though)
> I modified the listbox.xul attached to this bug to have 5 visible rows and
> 10 items.
> 
> I was using an external, Apple "Magic Mouse", while dragging on the scroll
> handle.
Upon closer review, it appears that the "stepped" effect was not introduced with this patch, but rather something else that has already landed.  FX33 still exhibits the flickering behavior, while Nightly has the stepped effect.
OK, I was testing the patch with Nightly.  So, let's not worry about the
behavior change on the 33 branch.

BTW, I think I found what causes the horrible flicker when I click-n-hold
on the scrollbar background:
In UpdateIndex(int32_t aNewPos):

+  bool up = newIndex < mCurrentIndex;
+  if (up) {
     mCurrentIndex--;
-  else mCurrentIndex++;
+  } else {
+    mCurrentIndex++;
+  }
   ...
+  InternalPositionChanged(up, 1);

If I change that so that it use the new index as is rather than just
a ±1 delta then it works perfectly.  Something like this:
   int32_t delta = newIndex - mCurrentIndex;
   mCurrentIndex = newIndex;
   ...
   InternalPositionChanged(delta < 0, Abs(delta));

Is there a reason we must just step ±1 here?
(it seems odd to me)
Comment on attachment 8500782 [details] [diff] [review]
Bug 1074165 - Prevent out of range scrolling in nsListboxBodyFrame (V2 Patch)

>- nsListBoxBodyFrame::ThumbMoved has been udpated to to ComputeNewIndex.

"to to" (in the commit message)

>layout/xul/nsListBoxBodyFrame.cpp
>+nsListBoxBodyFrame::ComputeNewIndex(nscoord aNewPos)
>+{
>+  nscoord 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++;
>   }
>-  aScrollbar->MoveToNewPosition();
>+
>+  // update the position to be row based.
>+  int32_t newIndex = aNewPos > oldTwipIndex ? mCurrentIndex + rowDelta : mCurrentIndex - rowDelta;
>+
>+  return newIndex;
> }

I see that you're just moving the code, but the above seems unnecessarily
complicated... can we just round aNewPos to the nearest row, like so:

  return NS_roundf(float(aNewPos) / mRowHeight);

Also, this seems to be just a conversion method, so I don't think "New"
adds any value in the names for this method.  How about renaming it:
nsListBoxBodyFrame::ToRowIndex(nscoord aPos)

Also, can we assert that aNewPos is >= 0 in this method,
or clamp it otherwise?

> nsListBoxBodyFrame::ThumbMoved(nsScrollbarFrame* aScrollbar,
>-  if (smoother->IsRunning() || rowDelta*mTimePerRow > USER_TIME_THRESHOLD) {
>+  if (smoother->IsRunning() || abs(rowDelta)*mTimePerRow > USER_TIME_THRESHOLD) {

We shouldn't use abs() - use mozilla::Abs instead (or just Abs in case
we're "using mozilla" here), and #include "nsAlgorithm.h".

>+  InternalPositionChanged(rowDelta < 0, abs(rowDelta));

same here

>-nsListBoxBodyFrame::UpdateIndex(int32_t aDirection)
>+nsListBoxBodyFrame::UpdateIndex(int32_t aNewPos)

This method looks fine so far but I think we should consider
fixing the flickering I discovered above.  Can you look into
that and see if it's doable in this bug?

>layout/xul/nsListBoxBodyFrame.h
>-  void UpdateIndex(int32_t aDirection);
>+  void UpdateIndex(int32_t aNewPos);

Please document that aNewPos is in CSS pixels.

And the same for the return value of MoveToNewPosition in
layout/xul/nsScrollbarFrame.h just to be clear.
Attachment #8500782 - Flags: review?(mats) → review+
> int32_t ComputeNewIndex(nscoord aNewPos);

Also, it should be "const".
- 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.

V3 Patch:
- Updated based on review in Comment 19 to Comment 21.
- Includes fix for flickering described in Comment 19
Attachment #8500782 - Attachment is obsolete: true
Pushed to try:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c66cf23ccbf1

Will set checkin-needed if this passes.
Comment on attachment 8502074 [details] [diff] [review]
8500782: Bug 1074165 - Prevent out of range scrolling in nsListboxBodyFrame (V3 Patch), r=mats

Sorry, I just realized that by making MoveToNewPosition() NOT be the
last thing in these methods we made it insecure - 'this' might be
destroyed by it.  So I think all these methods needs a WeakFrame
check and early return, like so:

  nsWeakFrame weakFrame(this);
  int32_t newPos = aScrollbar->MoveToNewPosition();
  if (!weakFrame.IsAlive()) {
    return;
  }

Otherwise, this looks great, thanks for fixing this!
- 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.

V4 Patch:
- Added nsWeakFrame::IsAlive checks after MoveToPosition calls
Attachment #8502074 - Attachment is obsolete: true
Keywords: checkin-needed
it's likely nsAlgorithm.h that's wrong and should be mozilla/MathAlgorithms.h
Fixed the #include (my bad advice, sorry).  I also added a "using namespace mozilla"
since it was missing - I'm guessing we got that through unified sources.
https://hg.mozilla.org/integration/mozilla-inbound/rev/07f47d9c4c0b
Thanks Mats and Mike!
https://hg.mozilla.org/mozilla-central/rev/07f47d9c4c0b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
This fix landed on m-c 13 days ago and is now on Aurora. 34 (Beta) is marked as affected. Can you submit an approval request for beta?
Flags: needinfo?(kgilbert)
Comment on attachment 8502152 [details] [diff] [review]
Bug 1074165 - Prevent out of range scrolling in nsListboxBodyFrame (V4 Patch), r=mats

Approval Request Comment
[Feature/regressing bug #]: Bug 957445 changed the order of scroll index clamping, resulting in out of range indexes in XUL listboxes 
[User impact if declined]: XUL listboxes may become empty when scrolling
[Describe test coverage new/current, TBPL]: Manual testing of test attached to Bug 1074165, manual testing of various sizes and list item counts of XUL listboxes, tested with try push.
[Risks and why]: low, change was localized to XUL element related code
[String/UUID change made/needed]: none
Flags: needinfo?(kgilbert)
Attachment #8502152 - Flags: approval-mozilla-beta?
Comment on attachment 8502152 [details] [diff] [review]
Bug 1074165 - Prevent out of range scrolling in nsListboxBodyFrame (V4 Patch), r=mats

Beta+
Attachment #8502152 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.