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
Target Milestone: --- → Firefox 3 M11
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
Created attachment 292866 [details] [diff] [review] patch
Attachment #292866 - Flags: review?(gavin.sharp)
10 years ago
No longer depends on: 406487
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()?
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.
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
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
Flags: in-litmus? → in-litmus+
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.