Closed Bug 504384 Opened 12 years ago Closed 12 years ago

Excessive Flickering with the Asynchronous Location Bar

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: sdwilsh, Assigned: Mardak)

References

()

Details

Attachments

(3 files)

Users are experiencing excessive flickering of autocomplete results when using the asynchronous location bar.

I have found that removing the synchronous call to adjustHeight in invalidate seems to make this problem go away.  Right now, we call adjustHeight synchronously, and on a timeout.  This was added in bug 415190.

The reason for this is described in the last part of bug 415190 comment 5:
But potentially we will show the old number of rows momentarily when there's
fewer rows after an invalidate -- i.e., starting a new search and the timeout
hasn't updated the height. Perhaps we should update the height on call and on
timeout0?

However, this is likely not going to happen if we aren't thrashing the main thread.  Even then, if it does happen, showing the old number of rows isn't terrible (there will either be some extra whitespace at the end if we have less results, or there we won't immediately be tall enough for all of our results).
Just to update from irc, perhaps try updating the height only after appending the first set of visible (6) results.
Attached patch v1.0Splinter Review
This removes the synchronous call to adjustHeight.  The adjustHeight call running in the setTimeout will run after _appendCurrentResult has added our results, and after any overflow has happened that we might hit (see _adjustAcItem).

I found that doing only the synchronous call after we add our items doesn't actually solve this problem, but doing it in the setTimeout does.
Attachment #388762 - Flags: review?(gavin.sharp)
Whiteboard: [needs review gavin]
Curious, how does this behave when you've typed out something long and have say.. 5 results and then type another word to reduce down to 2 results?
(In reply to comment #1)
> Just to update from irc, perhaps try updating the height only after appending
> the first set of visible (6) results.
The current patch will actually do that because of the setTimeout.  I decided to have it above the call to _appendCurrentResult because it would run after the second call to _appendCurrentResult (it calls itself in a setTimeout), and we wouldn't want to wait that long.
(In reply to comment #3)
> Curious, how does this behave when you've typed out something long and have
> say.. 5 results and then type another word to reduce down to 2 results?
Seems to work just fine.
Comment on attachment 388762 [details] [diff] [review]
v1.0

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml

>       <method name="_invalidate">

>+            // Adjust the height, but do so on a timeout because items heights
>+            // have a delay to them

"because items heights have a delay to them" is oddly phrased (and somewhat frustratingly vague). Perhaps "need to delay this because otherwise adjustHeight gets the wrong height values", with an XXX comment to followup and figure out why that is?
Attachment #388762 - Flags: review?(gavin.sharp) → review+
Comment on attachment 388762 [details] [diff] [review]
v1.0

>+++ b/toolkit/content/widgets/autocomplete.xml
>-            // Dynamically update height until richlistbox.rows works (bug 401939)
>             // Adjust the height immediately and after the row contents update
>-            this.adjustHeight();
>+            // Adjust the height, but do so on a timeout because items heights
You probably meant to delete the original Adjust the hight comment?
Whiteboard: [needs review gavin]
(In reply to comment #7)
> You probably meant to delete the original Adjust the hight comment?
No, wanted to have a comment there.
(In reply to comment #8)
> (In reply to comment #7)
> > You probably meant to delete the original Adjust the hight comment?
> No, wanted to have a comment there.
Er, I see what you meant now...
http://hg.mozilla.org/projects/places//rev/246b44df7dd3

Although, I'm not sure it's fixed now.  My debug build was showing flickering again.
Whiteboard: [fixed-in-places]
i don't see flickering caused by adjustHeight, on current build... what i see seems like flickering due to closePopup.

do you have any step to try reproducing the kind of flickering you did originally see?
Keep typing one letter at a time and you will see the flickering.  You don't see it with mozilla-central, but you do on the places branch.  I'll be backing out the patch I landed here later today since it didn't seem to work.
Alternatively, get rid of the _notifyResults(true) from startSearch.
Attachment #391097 - Attachment is patch: true
Attachment #391097 - Attachment mime type: application/octet-stream → text/plain
Attached patch v2 altSplinter Review
the fact is that i don't see much problems with adjustHeight, i see like if the popup is closed open... but actually i was playing with the old results collapsing. btw, once the hang due to locking will be fixed, this is already much usable than before... i'll keep experimenting changes for flickering.
Comment on attachment 391099 [details] [diff] [review]
v2 alt

>-    this._notifyResults(true);
Does this one liner actually do the trick?  Can you explain why please (although I think I know why)?
(In reply to comment #16)
> (From update of attachment 391099 [details] [diff] [review])
> >-    this._notifyResults(true);
> Does this one liner actually do the trick?  Can you explain why please
> (although I think I know why)?
So with more testing.. it doesn't completely solve the problem, but it makes the first result not flicker.

The reason it helps is that right now startSearch immediately calls notifyResults with 0 results, so the popup will invalidate with 0 results. We soon get some results from handleResult and show them. That causes flicker for the first item.

The rest of the items will still flicker sometimes, and I've instrumented invalidate, handle result, and process row to see what they're being called with..

1) it's possible for normal search to conflict with adaptive, so we end up calling notifyResults after handleResult calls processRow a few times but actually adds nothing.

2) some reason handleResult always (?) only processes one row for the very first handleResult. This is why the removal of notifyResults(true) for 0 results removes flickering for the first row. So every time you type, the list drops down to 1 item momentarily with the 1 line fix -- whereas before it would drop to 0. 

If somehow handleResult provided more data in the first batch, it wouldn't flicker as much. Alternatively if we waited to notifyResults with more results, it wouldn't be as bad.
(In reply to comment #17)
> The reason it helps is that right now startSearch immediately calls
> notifyResults with 0 results, so the popup will invalidate with 0 results. We
> soon get some results from handleResult and show them. That causes flicker for
> the first item.
Right, so we should remove that notify line, and I don't think it will cause any problems.

> 1) it's possible for normal search to conflict with adaptive, so we end up
> calling notifyResults after handleResult calls processRow a few times but
> actually adds nothing.
We should only ever call _notifyResults if we actually added something, right?  That is what the haveMatches variable is supposed to be checking, and AFAICT, is checking.

> 2) some reason handleResult always (?) only processes one row for the very
> first handleResult. This is why the removal of notifyResults(true) for 0
> results removes flickering for the first row. So every time you type, the list
> drops down to 1 item momentarily with the 1 line fix -- whereas before it would
> drop to 0. 
That's because storage code dispatches results every 100ms, up to 15 results per batch, starting when the consumer calls executeAsync.
Depends on: 506959
Using an hourly build with bug 506959 fixed and "v2 alt" with the single line removal of notifyResult from startSearch, results are amazing !

Amazing !

Like no flickering at all. Instrumenting how many items handleResult is getting, for a few letters, the first handleResult gets enough (12) results most of the time.

Land it ! ;)
Just FYI, I used this build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/places-macosx/1248812693/

Built from http://hg.mozilla.org/projects/places/rev/ecb4e8aabac4

And modified components/nsPlacesAutoComplete.js and deleted the notifyResults.
Assignee: sdwilsh → nobody
Component: Autocomplete → Places
QA Contact: autocomplete → places
Comment on attachment 391099 [details] [diff] [review]
v2 alt

I meant to give this r+ yesterday, but seemed to have forgotten to.  r=sdwilsh

Do you need me to land this?
Attachment #391099 - Flags: review+
http://hg.mozilla.org/projects/places/rev/a846e4cb810c
Don't tell autocomplete that we have 0 results ongoing when we just started the search, so it doesn't cause the UI to show 0 results and immediately show more.
Assignee: nobody → edilee
Whiteboard: [fixed-in-places]
Target Milestone: --- → mozilla1.9.2a1
http://hg.mozilla.org/mozilla-central/rev/a846e4cb810c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-places]
You need to log in before you can comment on or make changes to this bug.