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)

defect
Not set
minor

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)

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.
OS: Windows 7 → All
Hardware: x86_64 → All
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)
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
Component: General → XUL
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)
Blocks: 1067087
[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
Attached image screenshot (obsolete) —
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
Attachment #8496857 - Attachment is obsolete: true
See Also: → 1074236
(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.
Blocks: 1066456
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?
Flags: needinfo?(mats)
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)
Attached patch fix (obsolete) — Splinter Review
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)
Severity: normal → minor
Lot's of orange b/c "Assertion failure: maxTopRowIndex >= 0".
e.g. content/xul/content/crashtests/363791-1.xul
Attached patch fix, v2Splinter Review
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 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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/120d00383f29
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
See Also: → 1085050
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
Depends on: 1085050
See Also: 1085050
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)
Yeah, this seems low-risk to me.
Flags: needinfo?(mats)
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+
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.