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)
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)
348 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
122.08 KB,
image/png
|
Details | |
8.12 KB,
patch
|
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
[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
Reporter | ||
Updated•10 years ago
|
Summary: XUL listbox becomes empty when scroll down by mouse down → XUL listbox becomes empty when scroll down by mouse down on scroll arrow
Reporter | ||
Comment 1•10 years ago
|
||
Updated•10 years ago
|
Comment 3•10 years ago
|
||
The bug #1066459 fix the bug in tree like the issue in this bug.
Reporter | ||
Comment 4•10 years ago
|
||
The offending Bug 957445 should be backed out before uplift 34Beta.
Flags: needinfo?(kgilbert)
Assignee | ||
Comment 5•10 years ago
|
||
(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)
Assignee | ||
Comment 6•10 years ago
|
||
(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 | ||
Updated•10 years ago
|
Assignee: nobody → kgilbert
Assignee | ||
Comment 7•10 years ago
|
||
I have reproduced the issue on Ubuntu 14.04.1 LTS
Assignee | ||
Comment 8•10 years ago
|
||
- 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.
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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-
Comment 11•10 years ago
|
||
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.
Comment 12•10 years ago
|
||
s/It/I/
Assignee | ||
Comment 13•10 years ago
|
||
- 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)
Assignee | ||
Comment 14•10 years ago
|
||
Pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=172ef27b34a7
Assignee | ||
Comment 15•10 years ago
|
||
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?
Comment 16•10 years ago
|
||
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)
Assignee | ||
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
(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.
Comment 19•10 years ago
|
||
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 20•10 years ago
|
||
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+
Comment 21•10 years ago
|
||
> int32_t ComputeNewIndex(nscoord aNewPos);
Also, it should be "const".
Assignee | ||
Comment 22•10 years ago
|
||
- 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
Assignee | ||
Comment 23•10 years ago
|
||
Pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c66cf23ccbf1 Will set checkin-needed if this passes.
Comment 24•10 years ago
|
||
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!
Assignee | ||
Comment 25•10 years ago
|
||
- 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
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 26•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccde95bd3c7e
Keywords: checkin-needed
Comment 27•10 years ago
|
||
sorry had to back this out for bustage like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2880114&repo=mozilla-inbound
Comment 28•10 years ago
|
||
it's likely nsAlgorithm.h that's wrong and should be mozilla/MathAlgorithms.h
Comment 29•10 years ago
|
||
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
Assignee | ||
Comment 30•10 years ago
|
||
Thanks Mats and Mike!
https://hg.mozilla.org/mozilla-central/rev/07f47d9c4c0b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 32•10 years ago
|
||
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)
Assignee | ||
Comment 33•10 years ago
|
||
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 34•10 years ago
|
||
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.
Description
•