609694 - Location bar briefly paints previous autocomplete result before the new one

Status: RESOLVED INACTIVE

(Toolkit :: Autocomplete, defect)

Description

[Robert Sayre]

I've found that the location bar briefly paints its previous state before painting new results. The effect is easier to see when a result had been selected from the dropdown, because then the buggy flash of old results includes a highlighted row. The STR below include choosing a result because the attached movies and screengrabs do that.

STR:

1) Enter "a" into the location bar
2) arrow key down to the second result (craigslist in the video)
3) command-A, delete. This clears the location bar
4) Enter the letter "n"
5) BUG! The old results for "a" are painted, with craigslist still selected
6) The results for "n" are painted.

Thanks to Rainer for filming this.

Comment 1

[Robert Sayre]

Attachment: screencap of results for "a" displaying after I've typed "n"

Clip available here:

http://www.youtube.com/watch?v=NuA4GRIuObs

Comment 2

[Robert Sayre]

Also, literally everything in the UI is fading in. You can scrub the video in the quicktime player and see "n" fade into the location bar. Is that intentional?

Comment 3

[Ed Lee :Mardak]

You can probably see it better if you copy some random text "blakjwf wkajef kawjf lwajfl wakjfelkwj" and select-all then paste. Seems like the list isn't cleared until the next result (or no results) are given to the UI.

But clearing immediately on every keystroke probably isn't ideal either as the list will keep flashing with results, disappearing on new keypress, reappearing when results come in.

Comment 4

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

The problem is actually that the autocomplete controller invalidates before opening the popup, and the popup refuses to invalidate when the popup is closed. So for a given search invocation, we do:

<receive first chunk of results>
Invalidate() (does nothing, popup isn't opened)
Open()
<receive second chunk>
Invalidate() (populate results)
Open() (does nothing, popup already open)

If you chain two of these together, you get a flash of the old content after the first Open, because we haven't invalidated to clear out the old results.

Comment 5

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

Attachment: partial patch

I've had this in my queue for almost a year :(
https://hg.mozilla.org/users/gsharp_mozilla.com/patches/file/0/autocompleteCleanup

This code really is a mess. This patch is wrong in that it removes the invalidation done in openAutocompletePopup(), which supposedly depends on the width attribute being set. I suppose it could just not do that, but then we'd be invalidating twice, which is probably not acceptable.

Comment 6

[Justin Dolske [:Dolske]]

(In reply to comment #2)
> Also, literally everything in the UI is fading in.

Could just be slow LCD response, I've noticed the similar things in my random pokings with filming UI at 60fps... State changes often take ~3 frames to change.

Comment 7

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

Attachment: real partial patch

Actually that was the wrong patch - it resolves the inconsistency in the other direction, by always invalidating after open, which wouldn't fix this bug. This one always invalidates before opening.

This code has no test coverage and I'm kind of terrified about changing it...

Comment 8

[Robert Sayre]

(In reply to comment #7)
> Created attachment 488333 [details] [diff] [review]
> real partial patch

This bug is driving me crazy. What's needed to make this patch whole?

Comment 9

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

Someone with time to make sure it doesn't break anything, and ideally to write some tests.

Comment 10

[Drew Willcoxon :adw]

Something weird is going on.  Even if all the items in the richlistbox are collapsed and updated, and then the popup is opened (without uncollapsing the items), the old text of the items briefly flashes before disappearing.  I think I'm fighting some unintended side effect of something or other.

Removing all items before the popup is updated works though.

Comment 11

[Drew Willcoxon :adw]

You can see the old contents of the popup if you break here:

http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuPopupFrame.cpp#496

Which is right after this line:

  viewManager->SetViewVisibility(view, nsViewVisibility_kShow);

If you keep using gdb's "finish" command to go through the stack frames afterward, you'll see that the new contents are not shown until a paint event is dispatched and handled.

Comment 12

[Drew Willcoxon :adw]

I'm deep in layout code.  Neil, do you know why the popup would contain its old contents when it's made visible and not show its new contents until later, when it's painted?

The new contents appear on this line in nsViewManager::DispatchEvent when aEvent->message == NS_PAINT:

http://mxr.mozilla.org/mozilla-central/source/view/src/nsViewManager.cpp#911

Comment 13

[Neil Deakin]

Probably best to ask roc about it.

Comment 14

[Drew Willcoxon :adw]

roc, the awesomebar's popup contains a richlistbox, and while the popup is closed, its richlistitems are collapsed.  When the popup is opened later, the items briefly flash before disappearing.  Do you know why that would happen and how it could be prevented?  (I've been digging since comment 11, but I'm no layout hacker.)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It could be a retained layer. Perhaps at some point we get a paint event and nothing has invalidated the retained layer for some reason, so we use the retained layer to paint.

> Invalidate() (does nothing, popup isn't opened)

How is this Invalidate triggered? It should lead eventually to FrameLayerBuilder::InvalidateThebesLayerContents even if the widget is hidden.

Another thing is that we should probably kill the current layer manager for a widget by calling mLayerManager->Destroy() and nulling out mLayerManager when a popup widget is hidden. It'll save memory too.

Comment 16

[Drew Willcoxon :adw]

Thanks roc.

(In reply to comment #15)
> > Invalidate() (does nothing, popup isn't opened)
> 
> How is this Invalidate triggered? It should lead eventually to
> FrameLayerBuilder::InvalidateThebesLayerContents even if the widget is hidden.

That Invalidate is a method on the autocomplete popup (nsIAutoCompletePopup), which I believe is triggered by nsAutoCompleteController as it processes its autocomplete results.  Its implementation is here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#981

But I don't see how it leads to the function you mention.  Are we talking about different Invalidates?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Comment 18

[Drew Willcoxon :adw]

I presume you're talking about nsIFrame::Invalidate.  I don't know how or when that's triggered on an nsMenuPopupFrame, so I stuck a call to Invalidate in various places before the call to viewManager->SetViewVisibility in nsMenuPopupFrame::LayoutPopup using the (non-empty) nsRect created in that function.  After that I can see FrameLayerBuilder::InvalidateThebesLayerContents getting called (and not returning early), but it doesn't appear to have any effect since the flashing still occurs.

Comment 19

[Drew Willcoxon :adw]

Oh, nsMenuPopupFrame::InvalidateInternal and subsequently FrameLayerBuilder::InvalidateThebesLayerContents are already getting called when the popup is opened and from nsMenuPopupFrame::DoLayout, when it calls Layout.

Comment 20

[Drew Willcoxon :adw]

(In reply to comment #19)
> and from nsMenuPopupFrame::DoLayout

s/DoLayout/LayoutPopup/

Comment 21

[Neil Deakin]

Is this a regression or has it always occurred?

Comment 22

[Drew Willcoxon :adw]

It occurs in b7 but not b6.  I'll try to narrow it down further.

Comment 23

[Drew Willcoxon :adw]

Aha.  The problem is present in the 9-30 nightly but not the 9-29.  That's these changes:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a60414d076b5&tochange=5a2012482a63

Which include turning on OpenGL accelerated layer composition by default on OS X.  With layers.accelerate-all=false, the problem doesn't occur.

Comment 24

[Drew Willcoxon :adw]

Moving to Core::Graphics per comment 23, and since this seems be Mac-only, setting platform to OS X.  I've done all I can do, so unassigning.

Comment 25

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

Drew: the issue you're seeing is different, I suggest you file it separately. This bug as filed is entirely an autocomplete issue, described in comment 4, and has existed for a long time (at least since we added the awesomebar, and perhaps earlier).

Comment 26

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

And my "real partial patch" fixed it for me, so I guess I wasn't seeing whatever issue you're seeing when I tested it on Mac around the time I posted it.

Comment 27

[Dietrich Ayala (:dietrich)]

While it's not a regression in the AC code, perhaps something has occurred in the Firefox 4 development cycle to make the problem more visible? Going to block for investigation.

Can someone who can reproduce this try to get a regression range?

Comment 28

[Dietrich Ayala (:dietrich)]

Gavin(In reply to comment #9)
> Someone with time to make sure it doesn't break anything, and ideally to write
> some tests.

Probably enough coverage from the autocomplete api consumers in tests in the tree already for catching regressions.

Is it possible to write a test that validates the expected behavior in this change?

Comment 29

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

I'm not sure we have very good coverage for autocomplete popup invalidation (particularly for rich-popups).

I think I may have been too quick to dismiss Drew's findings; this does seem to be easier to reproduce on trunk than it is in 3.6, so maybe some other low-level change somehow made it worse, or there are multiple issues at play.

Comment 30

[Markus Stange [:mstange]]

The GFX part of this bug is bug 562138.

Comment 31

[(no longer active)]

Is there a good reason that this blocks betaN?  If not, it should be moved over to final+.

Comment 32

[Dietrich Ayala (:dietrich)]

Per comment #28, there's concern about the lack of automated test coverage for the autocomplete changes that need to be made here.

If bug 562138 is fixed, this might be mitigated enough to leave the AC code alone for this release.

Comment 33

[(no longer active)]

It would be great for someone who can reproduce this to see if my patch in bug 562138 helps (I can't reproduce the original problem stated in comment 0.)

Comment 34

[Markus Stange [:mstange]]

We no longer use GL acceleration for popup windows (since bug 631339), so this has probably morphed into a white flash.

Comment 35

[(no longer active)]

(In reply to comment #34)
> We no longer use GL acceleration for popup windows (since bug 631339), so this
> has probably morphed into a white flash.

I didn't get a (noticeable) white flash either...

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 7 automatically changed from blocking2.0:betaN+ to blocking2.0:.x during the endgame of Firefox 4 for the following reasons:

 - it was marked as a soft blocking issue for a beta
 - the whiteboard indicated that it might be appropriate for a .x release

Comment 37

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.