User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.103 Safari/537.36 Steps to reproduce: I have 100 element of a tree, and each page show 5 rows. When scrolled to the last page, and click the scroll down button. Actual results: It's will cause the tree scroll one more row and leave a blank row Expected results: It's should not scroll one more row. The page should be full.
Created attachment 8496496 [details] [diff] [review] 0002-Resolve-the-flipping-problem-of-the-last-row.patch Also fixes the flipping problem, when we scroll to the last row, And push the scroll down arrow without release the mouse, the tree cell area will not be normal
I can reproduce this in the cookie list - when I click-and-hold the scrollbar down button until scrolling stops, the scrollbar overscrolls by one row and when I release the mouse the scrollbar button is still enabled. Clicking on it once resets the view so that there is no empty row and the scrollbar button is disabled. I can reproduce that in Aurora but not Beta so I suspect it's a regression from bug 957445. Yonggang, it would be great if you could attach a testcase for this so we can add that to our regression test suite.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, regressionwindow-wanted
Comment on attachment 8496496 [details] [diff] [review] 0002-Resolve-the-flipping-problem-of-the-last-row.patch Comparing the changed method (nsTreeBodyFrame::ScrollInternal) between Aurora and Beta shows no difference, they are identical. So the suggested fix here might just wallpaper over an error that has occurred earlier. I'd like to understand better exactly where the error occurs, and what exactly the regression is, before I can r+ any patch to fix it.
[Tracking Requested - why for this release]: [Tracking Requested - why for this release]: Steps To Reproduce(for example): 1. Open https://developer.mozilla.org/en-US/Add-ons/Code_snippets 2. Open Page info dialog (click icon on location bar) 3. Select Media pane 4. Click down arrow button of tree view Actual Results: A empty row appears at the bootom of the tree 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
status-firefox33: --- → unaffected
status-firefox34: --- → affected
status-firefox35: --- → affected
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?
tracking-firefox34: ? → +
tracking-firefox35: ? → +
L'd like to say, this is not a regression, firefox 33 is also affected. and some earlier version.
un tracking due to comment#6
No longer blocks: 957445
status-firefox33: unaffected → ---
status-firefox34: affected → ---
status-firefox35: affected → ---
tracking-firefox34: + → ---
tracking-firefox35: + → ---
(In reply to Yonggang Luo from comment #6) > L'd like to say, this is not a regression, > firefox 33 is also affected. and some earlier version. Does this mean that the regression range provided in comment 4 is incorrect? On what earlier versions can you reproduce? I'm setting the tracking flags back to ensure that relman follows up on this one. This doesn't mean that we'll necessarily fix the issue in a given release. Note that even if we decide not to track, the status flags still provide useful information and can be left as set.
status-firefox32: --- → ?
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → affected
tracking-firefox34: --- → +
tracking-firefox35: --- → +
See bug 1074165 comment 10. The situation in this bug is similar: On Beta: (gdb) bt #0 nsTreeBodyFrame::ScrollInternal #1 nsTreeBodyFrame::ScrollToRowInternal #2 nsTreeBodyFrame::ScrollbarButtonPressed #3 nsScrollbarButtonFrame::DoButtonAction #4 nsScrollbarButtonFrame::HandleButtonPress #5 nsScrollbarButtonFrame::HandleEvent ... On Aurora: (gdb) bt #0 nsTreeBodyFrame::ScrollInternal #1 nsTreeBodyFrame::ScrollToRowInternal #2 nsTreeBodyFrame::ScrollToRow #3 nsTreeBodyFrame::ScrollByLines #4 nsTreeBodyFrame::ScrollByLine #5 nsScrollbarButtonFrame::HandleButtonPress #6 nsScrollbarButtonFrame::HandleEvent ... So, before bug 957445 we enjoyed the clamping that nsScrollbarButtonFrame::DoButtonAction did, and now we don't: http://mxr.mozilla.org/mozilla-beta/source/layout/xul/nsScrollbarButtonFrame.cpp?rev=1b5ee8c5491a&mark=226-230,236#203 On Aurora, there isn't any MoveToNewPosition() calls in the XUL tree case so for this bug maybe we should just wallpaper it in ScrollInternal somehow. We do clamp the "mCurPos" on the slider frame later though: (gdb) bt #0 nsSliderFrame::GetMaxPosition #1 nsSliderFrame::CurrentPositionChanged #2 nsSliderFrame::AttributeChanged #3 mozilla::RestyleManager::AttributeChanged #4 PresShell::AttributeChanged #5 nsNodeUtils::AttributeChanged #6 mozilla::dom::Element::SetAttrAndNotify #7 mozilla::dom::Element::SetAttr #8 nsIContent::SetAttr #9 nsXBLPrototypeBinding::AttributeChanged #10 nsXBLBinding::AttributeChanged #11 mozilla::dom::Element::SetAttrAndNotify #12 mozilla::dom::Element::SetAttr #13 nsIContent::SetAttr #14 nsTreeBodyFrame::UpdateScrollbars #15 nsTreeBodyFrame::ScrollToRow #16 nsTreeBodyFrame::ScrollByLines #17 nsTreeBodyFrame::ScrollByLine #18 nsScrollbarButtonFrame::HandleButtonPress #19 nsScrollbarButtonFrame::HandleEvent http://mxr.mozilla.org/mozilla-aurora/source/layout/xul/nsSliderFrame.cpp?rev=249dae95ea83&mark=663-667,701-701#649
I think it's clear that bug 957445 regressed this. At least for the STR in comment 4, which I used for the analysis above.
(In reply to Mats Palmgren (:mats) from comment #11) > I think it's clear that bug 957445 regressed this. At least for the STR in > comment 4, > which I used for the analysis above. Given this, do you have a way to fix this bug?
ScrollByLines/Pages already clamps the new value, so how did mTopRowIndex become larger than mRowCount - mPageLength in the first place? http://mxr.mozilla.org/mozilla-central/source/layout/xul/tree/nsTreeBodyFrame.cpp?rev=297baa9f1a98&mark=4072-4074#4063 This is the stack I get: (gdb) bt nsTreeBodyFrame::ScrollInternal layout/xul/tree/nsTreeBodyFrame.cpp:4119 nsTreeBodyFrame::ScrollToRowInternal layout/xul/tree/nsTreeBodyFrame.cpp:4057 nsTreeBodyFrame::RepeatButtonScroll layout/xul/tree/nsTreeBodyFrame.cpp:4201 nsScrollbarButtonFrame::Notify layout/xul/nsScrollbarButtonFrame.cpp:205 ...
Created attachment 8503584 [details] [diff] [review] fix I think we should just do the clamping in ScrollInternal instead, to cover all paths leading there. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5f89e0f4c460
Severity: normal → minor
status-firefox32: ? → unaffected
status-firefox33: wontfix → unaffected
Lot's of orange b/c "Assertion failure: maxTopRowIndex >= 0". e.g. content/xul/content/crashtests/363791-1.xul
Created attachment 8503595 [details] [diff] [review] fix, v2 So let's clamp that so it's not negative. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f5a904d47243
Comment on attachment 8503595 [details] [diff] [review] fix, v2 Review of attachment 8503595 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, and much cleaner
Attachment #8503595 - Flags: review?(kgilbert) → review+
The failures on Try looks unrelated, so let's land this. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f5a904d47243
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox35: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Hi, I filed Bug 1085050 - Assertion failure: mTopRowIndex == mozilla::clamped(mTopRowIndex, 0, maxTopRowIndex), at .../layout/xul/tree/nsTreeBodyFrame.cpp and I think the issue has something to do with the recent change introduced here. I wonder if someone can take a quick look at the trace of values that lead to the crash and suggest where to look. TIA
mats - Firefox 34 is marked as affected. This has been on m-c for 10 days. There was fallout in bug 1085050, but you fixed that. Can both bugs be uplifted to beta?
Yeah, this seems low-risk to me.
Comment on attachment 8503595 [details] [diff] [review] fix, v2 Approval Request Comment [Feature/regressing bug #]: bug 957445 [User impact if declined]: minor visual glitch when scrolling lists (UI) [Describe test coverage new/current, TBPL]: on m-c since 2014-10-12, but clearly we have no automatic test coverage for this [Risks and why]: low risk [String/UUID change made/needed]: none (minor correction for debug builds in bug 1085050 is also needed)
Attachment #8503595 - Flags: approval-mozilla-beta?
> minor visual glitch when scrolling lists (UI) More accurate: minor visual glitch when scrolling tree widgets, for example the list of cookies in the UI.
Comment on attachment 8503595 [details] [diff] [review] fix, v2 Beta+
Attachment #8503595 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox34: affected → fixed
status-b2g-v2.1: --- → fixed
status-b2g-v2.2: --- → fixed
QA Whiteboard: [good first verify]
Whiteboard: STR in comment #4
You need to log in before you can comment on or make changes to this bug.