944737 - [windows] Rendering glitches on color picker dialog when opened simultaneously in 2 Firefox windows

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect, P3)

Description

[Paul Silaghi, QA [:pauly]]

Attachment: color picker.png

28.0a1 (2013-11-28), Win 7 x64
STR:
1. Open http://diveintohtml5.info/examples/input-type-color.html in 2 Firefox windows
2. Open the color picker simultaneously in both windows
3. Close the color picker in the second window
4. In the first window, click in the custom color panel

Actual results:
Rendering issues

Comment 1

[Paul Silaghi, QA [:pauly]]

Attachment: color picker2.png

Comment 2

[Arnaud Bienner]

I confirm I can reproduce it here. It's weird :(

In bug 929496 we discussed the possibility to have only one color picker instance opened at a time.
If we fix bug 929496 this way, this will fix this bug by side effect.

Comment 3

[Paul Silaghi, QA [:pauly]]

This bug is happening on Windows only.

(In reply to Arnaud Bienner from comment #2)
> In bug 929496 we discussed the possibility to have only one color picker
> instance opened at a time.
> If we fix bug 929496 this way, this will fix this bug by side effect.
I was just going to file another report for showing only one color picker on Mac. This is causing the following: open the color picker from the first window, choose a color, leave the color picker opened, double click in the second Firefox window on the color element will change to the same color in both windows.
Do you think this is a separate issue ?

Comment 4

[Paul Silaghi, QA [:pauly]]

One other possible issue would be that the color doesn't get selected from the custom color panel if black or white are selected in the basic colors palette from the left. This is also reproducible on Chrome.

Please let me know which of the above do you consider real issues and should I file separately.

Comment 5

[Arnaud Bienner]

Sorry for the confusion: bug 929496 is for Linux (GTK) only.

> Do you think this is a separate issue ?

Yes: about Mac OS, IIRC there is a system limitation which prevent opening several color picker at the same time. So I don't think we can do anything about this (I think we discuss this on IRC, because I don't find anything on bug 875756).

> One other possible issue would be that the color doesn't get selected from
> the custom color panel if black or white are selected in the basic colors
> palette from the left. This is also reproducible on Chrome.

Probably a separate issue but I don't think it's our fault here, as we use Windows' native color picker.
Probably worth to check how the native color picker behaves in other circumstances (e.g. changing theming colors in Windows).

Comment 6

[Paul Silaghi, QA [:pauly]]

(In reply to Arnaud Bienner from comment #5)
> Probably worth to check how the native color picker behaves in other
> circumstances (e.g. changing theming colors in Windows).
Same behavior. So I guess it's not a Firefox issue.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Paul Silaghi, QA [:pauly] from comment #6)
> (In reply to Arnaud Bienner from comment #5)
> > Probably worth to check how the native color picker behaves in other
> > circumstances (e.g. changing theming colors in Windows).
> Same behavior. So I guess it's not a Firefox issue.

I think that means this is INVALID as a Mozilla bug, then -- sound good? (i.e. it's a bug in system libraries, not in Firefox, and it's probably not something we have the ability to fix)

(If this were much more severe like a security bug or a crasher, it might be worth keeping this open & looking for workarounds. But given that it's an uncommon use-case (2 pickers open from 2 windows) and that the effects are pretty mild, I don't think we're in that category.)

Comment 8

[Arnaud Bienner]

I agree with Daniel, and I don't think it's blocking issue.

However, do we want to try to find a workaround (for next releases)? e.g. allow only one color picker instance to be opened at a time (closing the old one, opening a new one)?

Comment 9

[Paul Silaghi, QA [:pauly]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> I think that means this is INVALID as a Mozilla bug
Notice that comment 6 is a response to the issue mentioned in comment 5.
This bug is about comment 0.

Comment 10

[Daniel Holbert [:dholbert]]

Ah, my mistake. Thanks for the clarification. (and FWIW, I can reproduce the "many crosshairs" issue from the first screenshot, on Windows 8, using up-to-date Aurora.)

In that case, I tend to think this is the type of bug that we could ship with, but would feel bad about, & would really like to see it fixed before shipping. Bumping up priority.

> (In reply to Arnaud Bienner from comment #8)
> However, do we want to try to find a workaround (for next releases)? e.g.
> allow only one color picker instance to be opened at a time

That seems like one possible reasonable workaround. FWIW, this is what Chrome seems to do.

> (closing the old
> one, opening a new one)?

(I'd rather that we refuse to open the second picker [as Chrome does], rather than force-closing the first one. Closing the first one would be a (minor) dataloss issue.  e.g. someone might've just customized the *perfect* color, and then if we implicitly cancel it and lose their work, they're gonna be very upset. Or alternately, if we try to avoid that by *accepting* the color when we forcibly close it, we might be stomping on an *old* color that they really wanted to save (because they were intending to cancel the first popup). That'd also be a dataloss issue.)

Comment 11

[Arnaud Bienner]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> (I'd rather that we refuse to open the second picker [as Chrome does],

OK, but let's do it in a better way ;)
For me, on Windows 7, Chrome still refuses to open a color picker even after the first one was closed.
Finally, when trying to refresh the page, it crashes.

Comment 12

[Daniel Holbert [:dholbert]]

As long as AsyncColorChooser::Run() is only ever called on the main thread [1], we should be able to just declare a static bool inside of that function, and bail before showing the dialog if someone else has set the bool to true.

You can use an AutoRestore<bool> to make sure its value is restored, too. For reference, see e.g. mRunningSample in http://mxr.mozilla.org/mozilla-central/source/dom/smil/nsSMILAnimationController.cpp#353
(though in this case, the bool wouldn't be a member-var -- it can just be a static local-var, initialized to 'false'.)

[1] which I think is the case, due to the use of NS_DispatchToMainThread to invoke it. Probably worth asserting NS_IsMainThread() just to be sure, though (since the use of this bool, and of the existing static buffer, won't be threadsafe)

Comment 13

[Daniel Holbert [:dholbert]]

jimm seems agreeable to Comment 12 (via IRC), so let's go with that, unless someone comes up with something better.

Arnaud, do you have time to do that (if not, I can take a crack at it), and does comment 12 make sense to you?

Comment 14

[Arnaud Bienner]

Sure, I will try to have a look tomorrow.

Comment 15

[Daniel Holbert [:dholbert]]

Thanks! Reclassifying into Widget|Win32, and CC'ing jimm, since I imagine he'll be your reviewer for this.

Comment 16

[Arnaud Bienner]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> [1] which I think is the case, due to the use of NS_DispatchToMainThread to
> invoke it. Probably worth asserting NS_IsMainThread() just to be sure,
> though (since the use of this bool, and of the existing static buffer, won't
> be threadsafe)

OK.
But are we sure we don't want to allow other threads to open color picker?
I don't know if this can be a valid use case.
But if so, we should probably surround this bool value manipulation with a mutex.

Comment 17

[Daniel Holbert [:dholbert]]

The NS_DispatchToMainThread here:
  http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsColorPicker.cpp#153
means we'll only ever invoke this code on the main thread.

That (combined with an assertion as a sanity-check) means we don't need a mutex.

> I don't know if this can be a valid use case.

It's not, really -- Gecko doesn't really use multiple threads, except for a few special cases like in networking code.  So for most Gecko code, you don't need to worry about being thread-safe.

Comment 18

[Arnaud Bienner]

Thinking again about this, one more thing: I know the current be behavior is ugly, however I'm wondering if the new one might not be worse:
- currently, we have the ugly "multiple crosshairs" thing, but this doesn't prevent user to select a color.
- with the new one, I'm afraid some users might open a color picker in a first window, then try open to open a new one in another window (which is hiding the first and its color picker) and feel Firefox is broken, and will not think about closing the first color picker (mainly because it's not visible anymore).

Comment 19

[Arnaud Bienner]

Attachment: Allow only one color picker at a time

Anyway, here is a patch which implement the behavior described in comment 12.

But, as I said in comment 18, I'm still not sure if this is a good idea or not.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8358506 [details] [diff] [review]
Allow only one color picker at a time

>+bool AsyncColorChooser::isAColorPickerOpen = false;

As noted in comment 12, I think this should be a static local variable (declared in the function-body).
(maybe next to the existing static local variable "customColors")

> nsColorPicker::Open(nsIColorPickerShownCallback* aCallback)
> {
>   NS_ENSURE_ARG(aCallback);
>+  NS_PRECONDITION(NS_IsMainThread(),
>+      "Color pickers can only be opened from main thread currently");
>+  // Allow only one color picker to be opened at a time, to workaround bug 944737
>+  if (AsyncColorChooser::isAColorPickerOpen) {
>+    NS_WARNING("Currently, it's not possible to open two color pickers at a time");
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }

Per comment 12, I was imagining that this all be added inside of AsyncColorChooser::Run().

That way:
 (1) It's trivially true that the IsMainThread() assertion will be honored (because AsyncColorChooser is only ever fired with DispatchToMainThread)
 (2) That main-thread invariant ensures we won't have race conditions with the bool
 (3) The bool ensures we won't be reentrant into ChooseColor()

and those protections are all localized and easy to verify.

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Bienner from comment #18)
> - currently, we have the ugly "multiple crosshairs" thing, but this doesn't
> prevent user to select a color.

The second screenshot (attachment 8340431 [details]) looks like it'd prevent a user from picking a color.

> - with the new one, I'm afraid some users might open a color picker in a
> first window, then try open to open a new one in another window (which is
> hiding the first and its color picker) and feel Firefox is broken, and will
> not think about closing the first color picker (mainly because it's not
> visible anymore).

This could happen, but I don't think it's a likely enough scenario (and the results aren't broken enough) that we should prefer an alternative solution that involves graphical corruption.

But perhaps more importantly, I think this bug indicates that calling the Windows ChooseColor() API in this way (recursively*) is likely unsupported and could have other subtle bugs (maybe even crashers).

(*It may not be obvious why this is a recursive call - more coming in next comment)

Comment 22

[Arnaud Bienner]

(In reply to Daniel Holbert [:dholbert] from comment #21)
> The second screenshot (attachment 8340431 [details]) looks like it'd prevent
> a user from picking a color.

Ah yes.
On my side, I just ended up with multiple crosshairs.

> But perhaps more importantly, I think this bug indicates that calling the
> Windows ChooseColor() API in this way (recursively*) is likely unsupported
> and could have other subtle bugs (maybe even crashers).

Good point.

Comment 23

[Daniel Holbert [:dholbert]]

Expanding on the recursive call a bit -- to be clear about what's going on here (in this bug's STR), when we open up two colorpickers -- when the second one fires, our callstack looks something like: 

 HandleEvents()
   HandleNextEvent()
     AsyncColorChooser::Run()
       ChooseColor()
         <!-- NOTE: ^this function *spins our event loop*, to allow us to stay responsive.
              It basically makes a recursive call to our HandleEvents() function: -->
         HandleEvents()
           HandleNextEvent()
             AsyncColorChooser::Run()
               ChooseColor()

(event-handling function names are likely wrong; they're just here for illustrative purposes)

So the point is, we end up calling ChooseColor recursively, which I'm suspecting the Windows API might not expect. (given the broken behavior that it triggers)

Comment 24

[Daniel Holbert [:dholbert]]

Basically, I think you should be able to get away with just adding the following near the beginning of Run() (somehwere before calling ChooseColor):

  static bool isAColorPickerOpen = false;

  if (isAColorPickerOpen) {
    // prevent reentrant calls
    return;
  }
  AutoRestore<bool> autoRestore(isAColorPickerOpen);
  isAColorPickerOpen = true;

Comment 25

[Daniel Holbert [:dholbert]]

(I haven't tested that exact code, but something like that should work)

Comment 26

[Arnaud Bienner]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> Per comment 12, I was imagining that this all be added inside of
> AsyncColorChooser::Run().

The problem with this is that I think we don't have any way to warn a problem appears, as nsColorPicker::Open will return NS_OK.
As a result, here: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLInputElement.cpp#882 we set mPickerRunning to true, and the value is never changed, preventing a new color picker to open for this page once we failed.
This change in HTMLInputElement has been introduced recently (bug 946479), mainly for input type file: not sure if it's pertinent for input type color.
This might work though if we call the callback anyway, with the initial color: the color will not change. Also, when the color didn't change, we don't update the state and so we don't trigger any event (but it sounds a bit like a hack to rely on this behavior IMO).

Comment 27

[Arnaud Bienner]

Attachment: Allow only one color picker at a time v2

Will be more meaningful with a patch.
- is it OK to return NS_OK from nsColorPicker::Open even if AsyncColorChooser::Run will fail?
- is it OK to call the callback even if we didn't open the color picker?

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Bienner from comment #26)
> The problem with this is that I think we don't have any way to warn a
> problem appears, as nsColorPicker::Open will return NS_OK.

This is probably fine, IMHO. ::Run can just return NS_OK, too.  The dialog will just fail to appear, and the color will remain unchanged, right?

(I don't think we need to warn the caller that anything failed. If we really wanted to warn the user, we could just make ::Run pop up a simpler dialog in this case, saying that a color picker is already open in another window, and asking the user to dismiss that. I'm not sure that's worth it though.)

> As a result, here:
> http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/
> HTMLInputElement.cpp#882 we set mPickerRunning to true, and the value is
> never changed, preventing a new color picker to open for this page once we
> failed.

Ah, right. We probably still need to invoke mCallback to fix that, right? (this code:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsColorPicker.cpp?mark=118-120#118 )
Presumably that's how we set mPickerRunning back to false, in the normal case?


So then maybe we want to structure this more like:

  static bool isAColorPickerOpen = false;
  if (!isAColorPickerOpen) {
    // Good, we're not a reentrant call
    AutoRestore<bool> autoRestore(isAColorPickerOpen);
    isAColorPickerOpen = true;
    [ALL THE EXISTING ::Run IMPL GOES HERE, STOPPING JUST BEFORE mCallback HANDLING]
  } // if we want to warn the user, we could do so in an "else" clause here.

  if (mCallback) {
    mCallback->Done(mColor);
  }
  return NS_OK;

Comment 29

[Daniel Holbert [:dholbert]]

(ah, right -- your new patch handles mCallback, too. Nice.)

(In reply to Arnaud Bienner from comment #27)
> Will be more meaningful with a patch.
> - is it OK to return NS_OK from nsColorPicker::Open even if
> AsyncColorChooser::Run will fail?

Seems fine to me.

> - is it OK to call the callback even if we didn't open the color picker?

Seems fine to me, too. (I don't know what kind of callbacks we're expecting, but if any of them aren't OK with the user opening the colorpicker and hitting "cancel" on it [which is effectively what this patch is equivalent to], then they're being unreasonable.)

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 8358542 [details] [diff] [review]
Allow only one color picker at a time v2

>+  if (isAColorPickerOpen) {
>+    NS_WARNING("Currently, it's not possible to open two color pickers at a time");
>+    if (mCallback) {
>+      BGRIntToRGBString(mInitialColor, mColor);
>+      mCallback->Done(mColor);
>+    }
>+    return NS_ERROR_NOT_AVAILABLE;

There's no real point in having a special return value here; it's going to get ignored anyway.

I think this would be a bit cleaner if it were structured to share the mCallback-invocation and "return NS_OK" clause, per comment 28. (I was missing the mInitialColor-->mColor conversion, though -- you could do that with a NS_WARNING inside of an "else" clause, where I've got the comment about "if we want to warn the user".)

Aside from that, this looks good to me.

Comment 31

[Arnaud Bienner]

Attachment: Allow only one color picker at a time v3

Cleaner version of the patch.

As discussed, I don't return an error anymore, but just put a warning.

To avoid too many (useless) conversions when we don't show the color picker (initial color string -> initial color DWORD -> initial color string) I delayed the "initial color -> DWORD color" conversion from nsColorPicker::Init to AsyncColorChooser::Run

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 8358622 [details] [diff] [review]
Allow only one color picker at a time v3

>diff --git a/widget/windows/nsColorPicker.cpp b/widget/windows/nsColorPicker.cpp
>-AsyncColorChooser::AsyncColorChooser(DWORD aInitialColor, nsIWidget* aParentWidget, nsIColorPickerShownCallback* aCallback)
>+AsyncColorChooser::AsyncColorChooser(const nsAString& aInitialColor, nsIWidget* aParentWidget, nsIColorPickerShownCallback* aCallback)

Rewrap this line to make it < 80 characters.

> AsyncColorChooser::Run()
> {
>   CHOOSECOLOR options;
>   static COLORREF customColors[16] = {0} ;

Declare these inside the "if" body, since that's the only place they're used.

>-  AutoDestroyTmpWindow adtw((HWND) (mParentWidget.get() ?
>-    mParentWidget->GetNativeData(NS_NATIVE_TMP_WINDOW) : nullptr));
>+  NS_PRECONDITION(NS_IsMainThread(),
>+      "Color pickers can only be opened from main thread currently");

Make this MOZ_ASSERT (which is fatal in debug builds), to make sure it doesn't get missed, if we ever change things such that it starts failing (which would be very bad).

>diff --git a/widget/windows/nsColorPicker.h b/widget/windows/nsColorPicker.h
>+  AsyncColorChooser(const nsAString& aInitialColor, nsIWidget* aParentWidget, nsIColorPickerShownCallback* aCallback);

As above, rewrap this line while you're here.

With that, this looks good to me! (Good call on reducing the needless string-conversions, too.)

r=me

Comment 33

[Arnaud Bienner]

Attachment: Allow only one color picker at a time v4 [r=dholbert]

Updated version  of the patch, with Daniel's comments applied.
Jim, could you please also review it?

Comment 34

[Arnaud Bienner]

Attachment: Allow only one color picker at a time v4: qdiff -w

Same patch without whitespaces changes, to ease review.

Comment 35

[Jim Mathies [:jimm]]

Comment on attachment 8358638 [details] [diff] [review]
Allow only one color picker at a time v4 [r=dholbert]

Review of attachment 8358638 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the added code cleanup!

::: widget/windows/nsColorPicker.cpp
@@ +109,2 @@
>  
> +  static bool isAColorPickerOpen = false;

nit - please change this to something like 'sColorPickerOpen'. Prepending 's' indicates 'static'.

Comment 36

[Arnaud Bienner]

Attachment: Allow only one color picker at a time v5

Thanks for the review!
Here is an updated version of a patch with s/isAColorPickerOpen/sColorPickerOpen

Comment 37

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Bienner from comment #36)
> Thanks for the review!
> Here is an updated version of a patch with
> s/isAColorPickerOpen/sColorPickerOpen

Doesn't look like that change made it in, actually -- this is the same as the previous patch, aside from the commit message.

Looks like maybe you attached the wrong version?

(Also: while you're at it -- maybe rename 'customColors' to 'sCustomColors', so we don't end up being inconsistent about static-variable-naming in this function?)

Comment 38

[Arnaud Bienner]

Attachment: Allow only one color picker at a time v5

Really sorry :( I think I forgot to hg qrefresh before creating the new patch.
Thanks a lot for checking :)
This one should be OK (I checked carefully this time ;).
Also, I've s/customColors/sCustomColors, as you suggested.

Comment 39

[Daniel Holbert [:dholbert]]

No worries; we all forget to qref sometimes.

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/3f7225e6d33a

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f7225e6d33a

Comment 41

[Manuela Muntean [Away]]

Verified as fixed on Win 7 64-bit with latest Nightly 29.0a1 (build ID: 20140115030229): I can't open the color picker twice, and after selecting a color in the custom color panel, I don't see the faulty behavior detailed in the above screenshots.

Comment 42

[Daniel Holbert [:dholbert]]

Comment on attachment 8358918 [details] [diff] [review]
Allow only one color picker at a time v5

Manuela suggested over email that we should fix this on Aurora, and I agree. Requesting approval.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression; just a bug in a new feature. Bug 930277 is what enabled the feature by default

User impact if declined: Graphical corruption, possible confusion from which color picker goes with which <input> element.

Testing completed (on m-c, etc.): Been on m-c for over a week

Risk to taking this patch (and alternatives if risky): Pretty targeted, well-understood patch, so seems low risk to me. Alternative would be to just do nothing.

String or IDL/UUID changes made by this patch: None

Comment 43

[Daniel Holbert [:dholbert]]

[setting status-firefox27:unaffected since this feature is preffed off there]

Comment 44

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/89acdc983be0

Comment 45

[Ioana (away)]

Verified as fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (20140128004001). The color picker gets opened the first time, then you can't open it in other windows until closing it.

...wouldn't it make more sense for it to close and open for the window you last tried to open it from? When I clicked to open teh picker in the second window and chose a color, I expected it to be for that window, not for one I where I opened the picker a while before.

Comment 46

[Daniel Holbert [:dholbert]]

(In reply to Ioana Budnar, QA [:ioana] from comment #45)
> ...wouldn't it make more sense for it to close and open for the window you
> last tried to open it from?

Not necessarily. See second half of Comment 10 for why that's probably more "dataloss-ish" than the strategy implemented in this bug.

Comment 47

[Ioana (away)]

(In reply to Daniel Holbert [:dholbert] from comment #46)
> (In reply to Ioana Budnar, QA [:ioana] from comment #45)
> > ...wouldn't it make more sense for it to close and open for the window you
> > last tried to open it from?
> 
> Not necessarily. See second half of Comment 10 for why that's probably more
> "dataloss-ish" than the strategy implemented in this bug.

I agree to what was said there, but the current behavior is not very intuitive for the user (especially since most apps work the other way around.

Comment 48

[Gabriela [:gaby2300]]

I came across this bug (Opens color picker in twice for both windows separately) during last beta testday (testday-20140214) using the latest beta and Windows 7 64-bit.

Comment 49

[Bogdan Maris, Desktop Test Engineering]

I reproduced this on Ubuntu 13.10 x64 as well using Firefox 28 beta 3. Should we reopen this bug or log a new one?

Comment 50

[Arnaud Bienner]

(In reply to Gabriela from comment #48)
> I came across this bug (Opens color picker in twice for both windows
> separately) during last beta testday (testday-20140214) using the latest
> beta and Windows 7 64-bit.

Strange, I tested again using the latest beta on Windows 7, and it's working fine for me: I cannot open a second color picker until the first one is closed, so the initial problem reported cannot appear anymore.
I don't understand why it's not working for you :(
Are you doing anything else specific?

Comment 51

[Arnaud Bienner]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #49)
> I reproduced this on Ubuntu 13.10 x64 as well using Firefox 28 beta 3.
> Should we reopen this bug or log a new one?

Looks like you're confused: this bug is Windows specific.

Comment 52

[Gabriela [:gaby2300]]

(In reply to Arnaud Bienner from comment #50)
> (In reply to Gabriela from comment #48)
> > I came across this bug (Opens color picker in twice for both windows
> > separately) during last beta testday (testday-20140214) using the latest
> > beta and Windows 7 64-bit.
> 
> Strange, I tested again using the latest beta on Windows 7, and it's working
> fine for me: I cannot open a second color picker until the first one is
> closed, so the initial problem reported cannot appear anymore.
> I don't understand why it's not working for you :(
> Are you doing anything else specific?

I don't think so. Please note that Bogdan Maris can also reproduce it this on Ubuntu 13.10 x64 as well using Firefox 28 beta 3.

Comment 53

[Arnaud Bienner]

If he's really talking about Ubuntu.(In reply to Gabriela from comment #52)
> I don't think so. Please note that Bogdan Maris can also reproduce it this
> on Ubuntu 13.10 x64 as well using Firefox 28 beta 3.

What are you talking about? Are you sure the problem you're trying to report is related to this bug? This bug is about rendering issues when opening two color pickers simultaneously on Windows, thus it is Windows specific (check comment 1).

Comment 54

[Bogdan Maris, Desktop Test Engineering]

The scenario we testes is that you cannot open two color pickers using two different windows. 
I realize now that my comment was unnecessary here since there is bug 929496 and the intended behavior on Linux is to allow at most 1 dialog per color-input button. (so, multiple dialogs can be displayed simultaneously if you have multiple buttons). Sorry for the confusion created.

So the problem that Gabriela stated in comment 48 is still a problem that was fixed here AFAICT. 
So Gabriela can you please answer Arnaud's question from comment 50?

Comment 55

[Arnaud Bienner]

Thanks for the clarification Bogdan :)
Indeed, we have a different behavior on Windows because of system limitation.

And thanks for changing the title Daniel: hopefully this should avoid confusion in the future.

Gabriela, could you please give us more details about the problem you're facing? I think it's something different than the problem originally reported in this bug. If so, we will probably need to open a new bug.

Comment 56

[Gabriela [:gaby2300]]

> Gabriela, could you please give us more details about the problem you're
> facing? I think it's something different than the problem originally
> reported in this bug. If so, we will probably need to open a new bug.

Arnaud, the problem was I could open the colour picker in both windows, when the expected is that you can open it in one window only. It seems to be a different problem than the one originally
reported in this bug so, as you said, we will probably need to open a new bug. I can do it if you like.

Comment 57

[Daniel Holbert [:dholbert]]

(In reply to Gabriela from comment #56)
> Arnaud, the problem was I could open the colour picker in both windows, when
> the expected is that you can open it in one window only. It seems to be a
> different problem than the one originally
> reported in this bug so, as you said, 

...though if this really happens, it indicates that this bug's fix (preventing the second color picker) isn't sticking, which makes it possibly worth following up on here instead of in a new bug.

I tried to reproduce what Gabriela described, on Windows 8 64-bit with Firefox 28 beta 3 (the current beta release offered at http://www.mozilla.org/en-US/firefox/beta/ ), but I couldn't reproduce -- the second window wouldn't open a color picker while the first window's picker was open. (good)

My version info:
about:support:  Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
about:buildconfig: Built from https://hg.mozilla.org/releases/mozilla-beta/rev/1f89dd5f5d38

Gabriela, can you double-check whether you're still seeing this behavior, and if so, post that ^^ information from the Firefox beta build that you're hitting this with?

Comment 58

[Gabriela [:gaby2300]]

Daniel, I tried again and I got the same result as before: the second window did open a color picker while the first window's picker was open.

My version info:
about:support: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
about:buildconfig: Built from https://hg.mozilla.org/releases/mozilla-beta/rev/2985d92ccf72

Comment 59

[Daniel Holbert [:dholbert]]

I tried to reproduce what Gabriela is describing, on a different machine (now running Windows 7), using latest Firefox Beta & a fresh Firefox profile, and I'm still unable to reproduce (i.e. I can't get the color picker to spawn from the second window, when the first window has a color picker up). My info on that machine:
 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
 Built from https://hg.mozilla.org/releases/mozilla-beta/rev/303a852bcba5
and I'm using http://diveintohtml5.info/examples/input-type-color.html as my testcase.

Given that this remaining issue is mysterious and it may take a while to figure out the exact set of conditions required to reproduce, let's spin it off into a separate bug after all. Would you mind filing, with the exact steps you're using to reproduce?