406194 - Rich url bar keeps old scroll position even on new input

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Ed Lee :Mardak]

Typing something in and scrolling down then cancel or start input results in the same scroll position instead of going back to the top.

Comment 1

[(not reading, please use [e-mail] instead)]

this is a lot like the other bug where I'm now clearing the selected index on invalidate() for the same reason.

details coming.

Comment 2

[(not reading, please use [e-mail] instead)]

thanks for the bug report, Edward.

I believe the fix for bug #406487 will fix this.

When we cancel, I need to set the selected index to -1.

Comment 3

[(not reading, please use [e-mail] instead)]

actually, this is something else.

when we set the selected index to -1, we call

this.richlistbox.ensureSelectedElementIsVisible();

which calls:

this.ensureElementIsVisible(this.selectedItem);

the selected item is null, so ensureElementIsVisible() does nothing.

I'm proposing that we fix it so that if given null, ensureElementIsVisible() calls this.scrollBoxObject.scrollTo(0, 0);

Comment 4

[(not reading, please use [e-mail] instead)]

alternatively, the fix could be moved into autocomplete.xml, but ensureSelectedElementIsVisible() is part of richlistbox.xml

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 6

[Simon Bünzli]

Please note that in a multi-select richlistbox it's possible to move the focus around without selecting any element. In that case, I'd expect the focused item to remain visible. So IMO this patch should be specific to the richautocomplete binding.

Comment 7

[(not reading, please use [e-mail] instead)]

Simon, can you elaborate on how this would break the multi-select case?

Upon moving focus, without selection, do we end up calling ensureElementIsVisible() (with null) or ensureSelectedElementIsVisible()?

Comment 8

[Simon Bünzli]

Ourselves, we should never be calling ensureElementIsVisible with a null argument. However other callers might (potentially through the deprecated ensureSelectedElementIsVisible but maybe also directly).

So, why don't you do

- this.richlistbox.ensureSelectedElementIsVisible();
+ this.richlistbox.ensureElementIsVisible(this.richlistbox.selectedItem ||
+                                         this.richlistbox.firstChild);

in autocomplete.xml?

Besides: From an API point of view, having unexpected side-effects isn't really a good solution, either. IMO we should rather throw when passing null into ensureElementIsVisible than doing the equivalent of |this.ensureElementIsVisible(this.firstChild)|.

Should you still insist on making this change, please ask at least Neil Deakin for review.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 292866 [details] [diff] [review]
patch

I agree with Simon. His proposed solution sounds good, too.

Comment 10

[(not reading, please use [e-mail] instead)]

Simon, thanks for the additional explanation and the suggestion.

I'll implement it, test it, and attach a new patch.

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: patch, Simon's suggestion

Simon's suggestion works like a charm.

Comment 12

[(not reading, please use [e-mail] instead)]

for litmus, here is how I tested this:

1) in the url bar, type h (assuming you have more than 10 items in history)
2) scroll down with the keyboard down key so that the scroll bar thumb is no longer at the top of the scroll bar.

3) hit escape
4) type t (so the url bar now has ht)

actual results:
scroll bar thumb is at the old position, not at the top.

expected results:
scroll bar thumb should be at the top

Comment 13

[(not reading, please use [e-mail] instead)]

over to simon, as this is his patch.

based on Gavin's comment ("His proposed solution sounds good, too."), I did r=sspitzer/gavin on checkin.

fixed.

Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.x
ml
new revision: 1.101; previous revision: 1.100
done

Comment 14

[(not reading, please use [e-mail] instead)]

thanks again, Simon.

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=5047

in-litmus+

Comment 16

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2007122704 Minefield/3.0b3pre

Comment 17

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv