375436 - [FIX]buttons flash unstyled on click-and-hold

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

[froodian (Ian Leue)]

Camino 2007032505 (1.2+)

STR:

1. Find an aqua-themed button (like the "Commit" button on this page)
2. Click-and-hold

What happens: Flashes as unstyled momentarily, and then goes back to being aqua

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

"unstyled" = gfx grey box button

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

You don't even have to click and hold; a simple click will cause it most places (though you have to have faster eyes ;) to see it then)

Comment 3

[Stuart Morgan]

There's a brief moment where IsWidgetStyled says the button is active and should thus have an inset border, but it still has an outset border for some reason I haven't been able to track down yet.

Comment 4

[Josh Aas]

When the button is pushed NS_EVENT_STATE_HOVER and NS_EVENT_STATE_ACTIVE get set and according to CSS that should set the border to inset instead of offset. That eventually happens, but not until later. We get about 10 events with hover and active set but the border is still outset before the border flips to inset.

Seems timing related, there is some delay between when NS_EVENT_STATE_HOVER and NS_EVENT_STATE_ACTIVE gets set and when the CSS for the border kicks in.

Comment 5

[Josh Aas]

Here's what is going on (thanks to bz for the help)...

The event state changes on the button and then a restyle event is sent. The restyle event should set the border correctly according to the event state, but before that event is getting processed something is causing us to paint. So this is a timing bug, we need to figure out why we're painting before the restyle.

Comment 6

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

We may want to flush restyles here:
http://lxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#5822

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Indeed

Josh, does this help?  I assume it does, since I doubt your stuff depends on XBL ctors... running there... ;)

Comment 8

[Josh Aas]

That does fix the problem. Are you sure that isn't a band-aid that covers up a paint we sholdn't be doing? We definitely don't need to be doing any extra painting in the already slow Mac trunk...

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Are you sure that isn't a band-aid that covers up a paint we sholdn't be doing? 

Not at all.  I do think this is the right patch in general, but we could well be doing extra paints here too...

Comment 10

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

What bz said. Without this patch we can't guarantee there won't be paints before the restyle has taken effect.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm... Although, won't this patch trigger restyles from a reflow-only flush (via the end of the view update batch)?  We probably don't want that....

Dammit, we need compositor.  :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

I suppose we could set a member indicating that we're inside a reflow-only flush and prevent restyle processing there....

Comment 13

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

A reflow-only flush doesn't cause synchronous paints, right? So there won't be synchronous restyling.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Well...  The thing is, EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC) (which a reflow-only flush does call) calls FlushPendingInvalidates(), which calls WillPaint().

Comment 15

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

Ok, so how bad it would be to do away with reflow-only flushes? To look at it another way, after a reflow-only flush we're going to have to restyle anyway before we paint the updates associated with that reflow; when should that restyle happen?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Ok, so how bad it would be to do away with reflow-only flushes?

That's a good question.  I think we should land bug 279703 and then see -- menus are a lot of the consumers of that stuff, and a lot of that stuff migth become safer.

> when should that restyle happen?

Ideally, that restyle would happen once we have unwound out of all member methods of frames (which are the things a restyle could destroy).  I _think_ that should be good enough if we're not running XBL constructors synchronously.

Comment 17

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

(In reply to comment #16)
> > Ok, so how bad it would be to do away with reflow-only flushes?
> 
> That's a good question.  I think we should land bug 279703 and then see --
> menus are a lot of the consumers of that stuff, and a lot of that stuff migth
> become safer.

So are we waiting for that to happen before we progress here?

Comment 18

[Boris Zbarsky [:bzbarsky]]

I think I am, for now.  :(

Comment 19

[Gary van der Merwe]

Also happens on Windows. 

Comment 20

[Boris Zbarsky [:bzbarsky]]

I doubt I'll be able to work on this.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: This should do it

The SVG code is still wrong; I'm waiting on an answer in the newsgroup.  And this depends on the changes for bug 392318 (though if the SVG stuff comes through I'll merge this to trunk, I guess).

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Removes the dead SVG code

Comment 25

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

+class nsPositionChangedEvent : public nsRunnable
+{

Instead of holding an nsWeakFrame here, I would prefer the frame to hold an nsRevocableEventPtr to the event and revoke it when the frame is destroyed.

Good other than that.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Merged to trunk (where the SVG code is already gone), and changed things to revoke events on Destroy() and to process pending position-changed events when a sync position change happens.

Comment 27

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

Can't we (shouldn't we) coalesce multiple position-changed events into one?

Comment 28

[Boris Zbarsky [:bzbarsky]]

I thought about it... but that seemed like a lot of work given directions, especially since when I was debugging this stuff sometime I think I've seen a direction of "up" but a negative offset....

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277596 [details] [diff] [review]
Updated to comments

Make sure to always process restyles before reflow.  Risk is fairly low.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 277596 [details] [diff] [review]
Updated to comments

a1.9=dbaron, although you don't actually need it since the bug is blocking1.9+.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to not depend on bug 392318

Comment 32

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix orange

Makes sure to flush layout before moving caret. I checked callers, and they seem reasonably safe...

Comment 34

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

There are an awful lot of callers to CharacterMoved etc, did you really check them all?

Comment 35

[Boris Zbarsky [:bzbarsky]]

They all come down to command controllers of various sorts...  Callers of those should generally expect them to do things that can cause script to run (since that's already the case).

I did check that the immediate callers seem to hold strong refs as needed, etc.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Did this cause tdhtml regression in bl-bldlnx03

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #36)
> Did this cause tdhtml regression in bl-bldlnx03
> 

Though, tp and tp2 on the same machine went down

Comment 38

[Boris Zbarsky [:bzbarsky]]

It's possible that it did: the test most affected ("slidein" as far as I can tell) now looks smoother...

Comment 39

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

FWIW, boxset and maya (CaminoTrunk) both showed Tdhtml regressions about this time, too.

On boxset, from ca. 725ms to ca. 760ms
On maya, from ca. 910ms to ca. 955ms

I presume from comment 38 that this increase is one we're going to live with in exchange for more correct behavior?

Comment 40

[Boris Zbarsky [:bzbarsky]]

I think so, yes...