287616 - [FIXr]need three clicks to open a colorpicker with type=button

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

For a colorpicker with the type set to button, i need hree click to open it when
the focus before starting to try to open it is on a textfield.

This is a regression, i'm working on a timeframe.

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: testcase

The testcase is simple. Just focus the textfield, and then try to open the
colorpicker.

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

by downloading builds from archive.mozilla.org, i have the following window:
2005012710 works
2005012805 fails

(gtk1 builds, but i originally found this using gtk2)

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

from bonsai, i see two checkins that might have cause this:
bug 261238 and maybe bug 279308

cc-ing patch authors.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

This is indeed caused by bug 279308. I removed the changes in
nsPopupSetFrame.cpp, and then i only needed one click, like it should. (but i
new get warnings on the console, so just backing out that part isn't the fix)

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, this is a problem only in the classic theme, as far as Neil and I can tell.

And if I make the "open" attribute change sync, the issue seems to go away. 
This attribute is being set on the <colorpicker> element.

Also, the popup does open (click, then hit the right arrow key to see this,
say).  It just doesn't reallly render for some reason.

So what does the classic theme do specially with the "open" attribute compared
to modern?  I tried messing with the classic colorpicker.css to make it be more
like modern, but that didn't help.

Looking at what style changes are associated with the "open" attribute on
colorpicker, I can't see why there would possibly be an issue here....

Comment 6

[[e-mail]]

It's odd that commenting out lines 63-77 of Classic's colorpicker.css should
make any difference as Modern actually has more css rules, not fewer.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Does removing those lines in fact help?

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

commenting out those lines does fix the bug. Commenting out one of the two
blocks doesn't. You need them removed both.

Also, using modern fixes this problem. But copying the colorpicker.css from
modern over classic version does not fix it. (although the picked does look blue
and modern)

Maybe somehow related to the textfield? Because if you forgot to focus it, you
also only need one click.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

indeed, removing -moz-appearance: textfield; from textbox.css fixes the problem.
(the modern of textbox.css doesn't have it in the first place)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

The exact testcase for this bug got "fixed" by bug 288365 (presumably because
that changed ordering around somewhere, somehow).  But the real problem, in my
opinion, is that style changes are processed asynchronously while rendering
changes due to widget state changing are synchronous.  This patch fixes the
latter to just follow the async codepath that style changes follow.  I tested
that this fixes the issue even in a build in which ApplyRenderingChangeToTree
does a DEFERRED batch.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180492 [details] [diff] [review]
Proposed patch

Requesting 1.8 approval... This one has a little bit of risk, but I think
overall making this follow the normal restyle codepath should make things more
reliable.

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 180492 [details] [diff] [review]
Proposed patch

a=asa

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed.