Closed
Bug 1066459
Opened 10 years ago
Closed 10 years ago
When the tree scrolled to the last page and last row, It's showed a blank row
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla35
Tracking | Status | |
---|---|---|
firefox32 | --- | unaffected |
firefox33 | --- | unaffected |
firefox34 | + | fixed |
firefox35 | + | fixed |
b2g-v2.1 | --- | fixed |
b2g-v2.2 | --- | fixed |
People
(Reporter: luoyonggang, Assigned: MatsPalmgren_bugz)
References
(Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: STR in comment #4)
Attachments
(1 file, 3 obsolete files)
3.02 KB,
patch
|
kip
:
review+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•10 years ago
|
OS: Windows 7 → All
Hardware: x86_64 → All
Reporter | ||
Comment 1•10 years ago
|
||
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
Attachment #8496496 -
Flags: superreview?(mats)
Attachment #8496496 -
Flags: review?(dbaron)
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Component: General → XUL
Assignee | ||
Comment 3•10 years ago
|
||
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.
Attachment #8496496 -
Flags: superreview?(mats)
Comment 4•10 years ago
|
||
[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
Blocks: 957445
status-firefox33:
--- → unaffected
status-firefox34:
--- → affected
status-firefox35:
--- → affected
tracking-firefox34:
--- → ?
tracking-firefox35:
--- → ?
Keywords: regressionwindow-wanted
Comment 5•10 years ago
|
||
Updated•10 years ago
|
Reporter | ||
Comment 6•10 years ago
|
||
L'd like to say, this is not a regression, firefox 33 is also affected. and some earlier version.
Comment 7•10 years ago
|
||
un tracking due to comment#6
No longer blocks: 957445
status-firefox33:
unaffected → ---
status-firefox34:
affected → ---
status-firefox35:
affected → ---
tracking-firefox34:
+ → ---
tracking-firefox35:
+ → ---
Keywords: regressionwindow-wanted
Updated•10 years ago
|
Attachment #8496857 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
(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:
--- → +
Assignee | ||
Comment 10•10 years ago
|
||
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
Assignee | ||
Comment 11•10 years ago
|
||
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.
Blocks: 957445
Keywords: regressionwindow-wanted
Comment 12•10 years ago
|
||
(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?
Flags: needinfo?(mats)
Assignee | ||
Comment 13•10 years ago
|
||
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 ...
Flags: needinfo?(mats)
Assignee | ||
Comment 14•10 years ago
|
||
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
Assignee: nobody → mats
Attachment #8496496 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8496496 -
Flags: review?(dbaron)
Attachment #8503584 -
Flags: review?(kgilbert)
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 15•10 years ago
|
||
Lot's of orange b/c "Assertion failure: maxTopRowIndex >= 0". e.g. content/xul/content/crashtests/363791-1.xul
Assignee | ||
Comment 16•10 years ago
|
||
So let's clamp that so it's not negative. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f5a904d47243
Attachment #8503584 -
Attachment is obsolete: true
Attachment #8503584 -
Flags: review?(kgilbert)
Attachment #8503595 -
Flags: review?(kgilbert)
Comment 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
The failures on Try looks unrelated, so let's land this. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f5a904d47243
Keywords: checkin-needed
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/120d00383f29
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/120d00383f29
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 21•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Comment 22•10 years ago
|
||
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?
Flags: needinfo?(mats)
Assignee | ||
Comment 24•10 years ago
|
||
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?
Assignee | ||
Comment 25•10 years ago
|
||
> 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 26•10 years ago
|
||
Comment on attachment 8503595 [details] [diff] [review] fix, v2 Beta+
Attachment #8503595 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 27•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/4fd0f4651a61
Comment 28•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/4fd0f4651a61
status-b2g-v2.1:
--- → fixed
status-b2g-v2.2:
--- → fixed
Updated•10 years ago
|
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.
Description
•