Closed Bug 563777 Opened 10 years ago Closed 10 years ago

Awesomebar refuses to scroll down when moving the selection using the Down key; it jumps back although selection stays.


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)



(Reporter: Aleksej, Assigned: mbrubeck)



(1 file, 1 obsolete file)

The nightly builds of Fennec branches 1.9.2 and mozilla-central.

Steps to reproduce:
1. Have awesomebar drop-down list which exceeds the screen height (e.g. by browsing a little, but you'll have to make sure the URLs get into the drop-down, see bug 555911).
2. When the drop-down list is displayed (by clicking the address bar), start pressing Down to move the selection down until you get to the bottom of the screen and further.

Expected results:
When the selection gets below the screen border, the drop-down list is scrolled and stays in a position that makes the selection completely visible.

Actual results:
When the selections gets below the screen border, the drop-down list is scrolled to make the selection visible, but immediately jumps back to make the first item of the list ("See all bookmarks") visible instead.

This may be related to bug 547307 ("Cannot reach the bottom of the page using keyboard").
Matt, can you take a look at this bug?
This is a regression from bug 551185.
Assignee: nobody → mbrubeck
Attached patch patch (obsolete) — Splinter Review
The problem is that choosing a selection fires an "input" event, and we scroll to the top on input events.  Here's a patch to avoid scrollToTop on these non-user-typed input events.
Attachment #445020 - Flags: review?(mark.finkle)
Attached patch alternate patchSplinter Review
This patch rolls back the two previous patches and moves the initial offending scroll check to the end of the invalidate method.

Moving it to the end should avoid the slow down we saw initially in bug 513949. One theory for the slow down was the reflow that was happening when the panel was being displayed. This would account for why the same code was faster when placed in the "closePopup" method.

We should test on device to make sure this truly doesn't regress the time it takes to display the awesomebar. IMO, this is a cleaner fix than the other fixes applied to the code since we landed bug 513949.
Attachment #445027 - Flags: review?(mbrubeck)
Comment on attachment 445027 [details] [diff] [review]
alternate patch

Worth a try.  I haven't tested the performance on N900 myself yet, curious to see if there's any effect.
Attachment #445027 - Flags: review?(mbrubeck) → review+
Attachment #445020 - Attachment is obsolete: true
Attachment #445020 - Flags: review?(mark.finkle)
pushed to m-b:

needs to bake and approval before landing on m-1.1
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 445027 [details] [diff] [review]
alternate patch

I like moving .scrollTo to the invalidate code because that's the places where it make sense, we need to be sure that this patch does not regress performance on first run where there was a kind of xpconnect penalty.
Attachment #445027 - Flags: review?(21) → review+
VERIFIED FIXED on Linux i686 with:
You need to log in before you can comment on or make changes to this bug.