Rich url bar keeps old scroll position even on new input

VERIFIED FIXED in Firefox 3 beta3

Status

()

Firefox
Bookmarks & History
P3
normal
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: Mardak, Assigned: Simon Bünzli)

Tracking

({regression})

Trunk
Firefox 3 beta3
regression
Points:
---
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Typing something in and scrolling down then cancel or start input results in the same scroll position instead of going back to the top.
this is a lot like the other bug where I'm now clearing the selected index on invalidate() for the same reason.

details coming.
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.

Assignee: nobody → sspitzer
Depends on: 406487
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 M11

Updated

10 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
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);
Status: NEW → ASSIGNED
alternatively, the fix could be moved into autocomplete.xml, but ensureSelectedElementIsVisible() is part of richlistbox.xml
(Assignee)

Comment 6

10 years ago
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.
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()?
(Assignee)

Comment 8

10 years ago
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 on attachment 292866 [details] [diff] [review]
patch

I agree with Simon. His proposed solution sounds good, too.
Attachment #292866 - Flags: review?(gavin.sharp) → review-
Simon, thanks for the additional explanation and the suggestion.

I'll implement it, test it, and attach a new patch.
Created attachment 293161 [details] [diff] [review]
patch, Simon's suggestion

Simon's suggestion works like a charm.
Attachment #292866 - Attachment is obsolete: true
Attachment #293161 - Flags: review+
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
Flags: in-litmus?
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
Assignee: sspitzer → zeniko
Status: ASSIGNED → NEW
thanks again, Simon.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
https://litmus.mozilla.org/show_test.cgi?id=5047

in-litmus+
Flags: in-litmus? → in-litmus+

Comment 16

10 years ago
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2007122704 Minefield/3.0b3pre
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.