540340 - In the initial loading of the show cookie dialogue, the tree of the list of cookies scrolls to 2nd line.

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2pre) Gecko/20100117 Firefox/3.5.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2pre) Gecko/20100117 Namoroka/3.6pre ID:20100117053836

In the initial loading of the show cookie dialogue, the tree of the list of cookies scrolls to 2nd row.

Though the first row is selected, it is displayed by the second row.

Reproducible: Always

Steps to Reproduce:
1.Start Namoroka/Minfield
2.(If necessary)Navigate several web site. and store enough number of cookies.
3.Tools > Options... > Plivacy
4.Select "Use custom setting for history" in the history group.
5.Click "Show Cookies..."

Actual Results:  
The tree of the list of cookies scrolls to 2nd row.


Expected Results:  
The tree of the list of cookies should scroll to 1nd row.


This issue happens on Namoroka and Minefield.
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2pre) Gecko/20100117 Namoroka/3.6pre ID:20100117053836
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100116 Minefield/3.7a1pre ID:20100116051420

Comment 1

[Alice0775 White]

Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/9dbded90af2a
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090113 Firefox/3.2a1pre ID:20090113035246

Broken:
http://hg.mozilla.org/mozilla-central/rev/89811ac1b35a
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090114 Firefox/3.2a1pre ID:20090114034711

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9dbded90af2a&tochange=89811ac1b35a

Candidate regress bug:
Bug 454632 -  Resizing tree widget always re-scrolls to current index

Comment 2

[Alice0775 White]

(In reply to comment #0)

s/1nd/1st/

Comment 3

[(mostly gone) XtC4UaLL [:xtc4uall]]

the issue isn't limited to initially opening this window, but also explicit selecting the fist entry and reopening the dialogue afterwards.

Comment 4

[Alice0775 White]

And 
It happens  in Tools > Options... > Content
"Allowed Sites - Popups" and  "Exceptions - Images" window.

Comment 5

[Neil Deakin]

Attachment: check page length before scrolling, and clean up return values

The fix is in EnsureRowIsVisibleInternal to check if mPageLength is 0 before scrolling.

I also cleaned up some return values in the scroll methods as most were unused

I need to work out what is special about the cookies and other dialogs in order to create a test.

Comment 6

[[e-mail]]

Comment on attachment 424114 [details] [diff] [review]
check page length before scrolling, and clean up return values

>-  nsresult rv = EnsureRowIsVisibleInternal(parts, aRow);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-  UpdateScrollbars(parts);
>-  return rv;
>-}
>-
>-nsresult nsTreeBodyFrame::EnsureRowIsVisibleInternal(const ScrollParts& aParts, PRInt32 aRow)
>-{
>-  if (!mView)
>-    return NS_OK;
>+  EnsureRowIsVisibleInternal(parts, aRow);
>+  UpdateScrollbars(parts);
>+  return NS_OK;
>+}
Sigh, hg diff made a mountain out of a molehill. Maybe separate cleanup?

>+  if (!mView || !mPageLength)
>+    return;
> 
>   if (mTopRowIndex <= aRow && mTopRowIndex+mPageLength > aRow)
>-    return NS_OK;
>-
>-  if (aRow < mTopRowIndex)
>-    ScrollToRowInternal(aParts, aRow);
>-  else {
>-    // Bring it just on-screen.
>-    PRInt32 distance = aRow - (mTopRowIndex+mPageLength)+1;
>-    ScrollToRowInternal(aParts, mTopRowIndex+distance);
>-  }
IMHO mPageLength can legally be zero, but that's no excuse to miscalculate which row to scroll to. On the other hand I rather like the idea that keepcurrentinview will have no effect when the tree is collapsed.

>-  // Our internal scroll method, used by all the public scroll methods.
>-  nsresult ScrollInternal(const ScrollParts& aParts, PRInt32 aRow);
>-  nsresult ScrollToRowInternal(const ScrollParts& aParts, PRInt32 aRow);
>+  // Our internal scroll methods, used by all the public scroll methods.
>+  void ScrollInternal(const ScrollParts& aParts, PRInt32 aRow);
>   nsresult ScrollToColumnInternal(const ScrollParts& aParts, nsITreeColumn* aCol);
I'd rather we keep the name ScrollToRowInternal.

Comment 7

[Neil Deakin]

Attachment: this version excludes the cleanup but adds a test

Hopefully Neil will decide what behaviour he likes here.

Comment 8

[[e-mail]]

Comment on attachment 424237 [details] [diff] [review]
this version excludes the cleanup but adds a test

OK, let's give this a whirl.

Comment 9

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/505db0be4476

Comment 10

[Dietrich Ayala (:dietrich)]

Awkward, but not broken, so not a blocker.