Location bar briefly paints previous autocomplete result before the new one

NEW
Unassigned

Status

()

Toolkit
Autocomplete
7 years ago
3 years ago

People

(Reporter: Robert Sayre, Unassigned)

Tracking

Trunk
All
Mac OS X
Points:
---

Firefox Tracking Flags

(blocking2.0 .x+)

Details

(Whiteboard: [softblocker][final?])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Created attachment 488287 [details]
screencap of results for "a" displaying after I've typed "n"

Clip available here:

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

Comment 2

7 years ago
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?
OS: Mac OS X → Windows 7
(Reporter)

Updated

7 years ago
blocking2.0: --- → ?

Comment 3

7 years ago
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.
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.
Created attachment 488321 [details] [diff] [review]
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.
(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.
Created attachment 488333 [details] [diff] [review]
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...
Attachment #488321 - Attachment is obsolete: true
(Reporter)

Comment 8

7 years ago
(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?
Someone with time to make sure it doesn't break anything, and ideally to write some tests.
Assignee: nobody → adw
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86 → All
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.
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.
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

7 years ago
Probably best to ask roc about it.
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.)
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.
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?
Yes.
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.
Oh, nsMenuPopupFrame::InvalidateInternal and subsequently FrameLayerBuilder::InvalidateThebesLayerContents are already getting called when the popup is opened and from nsMenuPopupFrame::DoLayout, when it calls Layout.
(In reply to comment #19)
> and from nsMenuPopupFrame::DoLayout

s/DoLayout/LayoutPopup/

Comment 21

7 years ago
Is this a regression or has it always occurred?
It occurs in b7 but not b6.  I'll try to narrow it down further.
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.
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.
Assignee: adw → nobody
Component: Location Bar → Graphics
OS: All → Mac OS X
Product: Firefox → Core
QA Contact: location.bar → thebes
Version: unspecified → Trunk
Status: ASSIGNED → NEW
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).
Component: Graphics → Autocomplete
Product: Core → Toolkit
QA Contact: thebes → autocomplete
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.
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?
blocking2.0: ? → betaN+
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?
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.
Whiteboard: [soft blocker]
Whiteboard: [soft blocker] → [softblocker]
The GFX part of this bug is bug 562138.
Depends on: 562138

Comment 31

7 years ago
Is there a good reason that this blocks betaN?  If not, it should be moved over to final+.
Whiteboard: [softblocker] → [softblocker][final?]
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

7 years ago
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.)
We no longer use GL acceleration for popup windows (since bug 631339), so this has probably morphed into a white flash.

Comment 35

7 years ago
(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...
** 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
blocking2.0: betaN+ → .x+
You need to log in before you can comment on or make changes to this bug.