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

RESOLVED FIXED in mozilla1.9alpha8

Status

()

P1
major
RESOLVED FIXED
12 years ago
4 months ago

People

(Reporter: froodian, Assigned: bzbarsky)

Tracking

({regression})

Trunk
mozilla1.9alpha8
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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
"unstyled" = gfx grey box button
Status: UNCONFIRMED → NEW
Ever confirmed: true
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)

Updated

12 years ago
Flags: blocking1.9+

Comment 3

12 years ago
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.

Updated

12 years ago
Severity: trivial → major

Comment 4

12 years ago
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

12 years ago
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.
Created attachment 263625 [details] [diff] [review]
Indeed

Josh, does this help?  I assume it does, since I doubt your stuff depends on XBL ctors... running there... ;)
Attachment #263625 - Flags: superreview?(roc)
Attachment #263625 - Flags: review?(roc)

Comment 8

12 years ago
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...
> 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...

Updated

12 years ago
Assignee: joshmoz → bzbarsky
Component: Widget: Cocoa → Layout: View Rendering
What bz said. Without this patch we can't guarantee there won't be paints before the restyle has taken effect.
QA Contact: cocoa → ian
(Assignee)

Updated

12 years ago
Priority: -- → P1
Summary: Aqua buttons flash unstyled on click-and-hold → [FIX]Aqua buttons flash unstyled on click-and-hold
Target Milestone: --- → mozilla1.9alpha5
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.  :(
(Assignee)

Updated

12 years ago
Blocks: 366205
I suppose we could set a member indicating that we're inside a reflow-only flush and prevent restyle processing there....
A reflow-only flush doesn't cause synchronous paints, right? So there won't be synchronous restyling.
Well...  The thing is, EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC) (which a reflow-only flush does call) calls FlushPendingInvalidates(), which calls WillPaint().
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?
> 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.
(Assignee)

Updated

12 years ago
Depends on: 279703
(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?
I think I am, for now.  :(
Also happens on Windows. 
OS: Mac OS X → All
Hardware: Macintosh → All
Target Milestone: mozilla1.9alpha5 → mozilla1.9alpha6
I doubt I'll be able to work on this.
Assignee: bzbarsky → roc
Keywords: helpwanted
Priority: P1 → --
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Duplicate of this bug: 387602

Updated

12 years ago
Summary: [FIX]Aqua buttons flash unstyled on click-and-hold → buttons flash unstyled on click-and-hold

Updated

12 years ago
Duplicate of this bug: 389529
Created attachment 277359 [details] [diff] [review]
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).
Attachment #263625 - Attachment is obsolete: true
Attachment #263625 - Flags: superreview?(roc)
Attachment #263625 - Flags: review?(roc)
(Assignee)

Updated

12 years ago
Depends on: 392318
Created attachment 277495 [details] [diff] [review]
Removes the dead SVG code
Assignee: roc → bzbarsky
Attachment #277359 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #277495 - Flags: superreview?(roc)
Attachment #277495 - Flags: review?(roc)
(Assignee)

Updated

12 years ago
Keywords: helpwanted
Priority: -- → P1
Summary: buttons flash unstyled on click-and-hold → [FIX]buttons flash unstyled on click-and-hold
+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.
Created attachment 277596 [details] [diff] [review]
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.
Attachment #277495 - Attachment is obsolete: true
Attachment #277596 - Flags: superreview?(roc)
Attachment #277596 - Flags: review?(roc)
Attachment #277495 - Flags: superreview?(roc)
Attachment #277495 - Flags: review?(roc)
Can't we (shouldn't we) coalesce multiple position-changed events into one?
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....
Attachment #277596 - Flags: superreview?(roc)
Attachment #277596 - Flags: superreview+
Attachment #277596 - Flags: review?(roc)
Attachment #277596 - Flags: review+
Comment on attachment 277596 [details] [diff] [review]
Updated to comments

Make sure to always process restyles before reflow.  Risk is fairly low.
Attachment #277596 - Flags: approval1.9?
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+.
Attachment #277596 - Flags: approval1.9? → approval1.9+
Created attachment 277650 [details] [diff] [review]
Merged to not depend on bug 392318
Attachment #277596 - Attachment is obsolete: true
Checked in.
No longer depends on: 392318
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Created attachment 277663 [details] [diff] [review]
Fix orange

Makes sure to flush layout before moving caret. I checked callers, and they seem reasonably safe...
Attachment #277663 - Flags: superreview?(roc)
Attachment #277663 - Flags: review?(roc)
Attachment #277663 - Flags: superreview?(roc)
Attachment #277663 - Flags: superreview+
Attachment #277663 - Flags: review?(roc)
Attachment #277663 - Flags: review+
There are an awful lot of callers to CharacterMoved etc, did you really check them all?
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.
Did this cause tdhtml regression in bl-bldlnx03
(In reply to comment #36)
> Did this cause tdhtml regression in bl-bldlnx03
> 

Though, tp and tp2 on the same machine went down
It's possible that it did: the test most affected ("slidein" as far as I can tell) now looks smoother...
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?
Depends on: 393723
I think so, yes...
(Assignee)

Updated

11 years ago
Depends on: 393801
Component: Layout: View Rendering → Layout: Web Painting
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.