975468 - <input type="color"> , Mac OS X: when picking color for one <input>, if you click on a different <input>, the first one clones its color

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Claude Pache]

Attachment: Minimal HTML file containing two input/type=color's

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140218122424

Steps to reproduce:

* open a document with two <input type="color">s;
* click on the first <input>; the Mac OS X color picker appears;
* without closing the color picker, select the second <input>.


Actual results:

* The first <input> receives the color of the second <input>.
* Moreover, it is no longer possible to change the color of the first <input> using the color picker.


Expected results:

* The first <input> should not receive the color of the second <input>.
* It should be possible to select the first <input> again and change its color.

Comment 1

[Daniel Holbert [:dholbert]]

Flagging as "tracking-firefox28", since that's the first release with the <input type="color"> feature enabled (per bug 930277).

(I suspect this bug here isn't severe enough to make us decide to disable the feature in this release, but it's at least worth tracking so we can make that determination in connection with any other related issues that might crop up while Firefox 28 is on Beta channel.)

Comment 2

[Claude Pache]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> (I suspect this bug here isn't severe enough to make us decide to disable
> the feature in this release)

When I tested the feature, I found the manifestation of this bug not only quite confusing, but, worse, positively destructive: As you go on and select the second <input type=color>, not only the choice you carefully made for the first <input> is unexpectedly lost, but also you are not able to correct the failure by reselecting the first <input> and using the color picker. Thus, you are doomed to submit an unwanted value (unless you use debugging tools).

Comment 3

[Manuela Muntean [Away]]

I can also reproduce this issue with Firefox 28 beta 4 on both Mac OS X 10.9 in 32-bit and 64-bit mode.

Comment 4

[Manuela Muntean [Away]]

This is also reproducible with latest Nightly and Aurora on both Mac OS X 10.9 in 32-bit and 64-bit mode.

With latest Aurora I experience the following: after closing the color picker and Aurora, when I relaunch Aurora and the profile manager window appears, automatically appears a color picker window, out of the blue. Any thoughts/suggestions?

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Claude Pache from comment #0)
> Actual results:
> * The first <input> receives the color of the second <input>.

I can confirm this, using Firefox Nightly on OS X 10.9.1.  This part is definitely broken.

> * Moreover, it is no longer possible to change the color of the first
> <input> using the color picker.

I can confirm this, too, using a current Nightly. (After clicking the second button, the color picker seems to be irreversably tied to the second button; it doesn't get retargeted to the first button when I click the first button again.)

Unlike the first part (about one button cloning the other button's color), *this* part didn't reproduce in an old Nightly that I had lying around from December 2013.  (not sure about the nightly's exact date, because it's been upgraded to the latest version now).

So this part is a regression in the last ~2 months.

These two issues probably have different underlying causes, and it might make sense to spin off a second bug for one or the other.

Arnaud, any chance you could take a look at this?

Comment 6

[Arnaud Bienner]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Arnaud, any chance you could take a look at this?

Unfortunately not, as I don't have a Mac :( and from what I understood, this happens only on OS X.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> > * Moreover, it is no longer possible to change the color of the first
> > <input> using the color picker.
[...]
> So this part is a regression in the last ~2 months.
> 
> These two issues probably have different underlying causes, and it might
> make sense to spin off a second bug for one or the other.

I spun off a separate bug for this "can't get color picker back to targeting the first <input>" issue.

Let's keep this bug here about the first ACTUAL RESULTS from comment 0, "The first <input> receives the color of the second <input>."

Comment 8

[Daniel Holbert [:dholbert]]

(The separate bug being bug 976724)

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

It's late in FF28 beta and you're right that we shouldn't hold back the feature entirely here but what about backing out enabling it in Mac OS X until these two regressions can be resolved so we don't ship an inferior experience to that portion of users?

Comment 10

[Daniel Holbert [:dholbert]]

Assuming we can't get a sufficiently low-risk fix in time, I suspect you're right.

There's enough collective brokenness (this bug, plus bug 976724, plus the random-colorpicker-dialog-at-startup thing hinted at in comment 4) with this feature on Mac OS X that I'd agree we should probably hold off on shipping this on that platform, if we can't get low-risk fixes for a subset of these bugs in time.

Comment 11

[Arnaud Bienner]

IMHO (even if this makes me sad :p) we should disable <input type="color">, but on OS X only (as those bugs don't impact other platforms) for Firefox 28: even if there is no regression as it is a new feature, we will falsy claim that we support input type color, preventing authors to fallback to another color picker.

About the bug itself:
IIRC on OS X we can't have many color pickers opened at a time, as we have to use a shared instance (namely: NSColorPanel sharedColorPanel).
Thus, I'm wondering if we shouldn't prevent a user to select another color until he's done with the current input, like we did for Windows in bug 944737 (for another technical limitation though).
Unlike Windows, it will clearer here to which input the color picker belongs to, as we update the input's color swatch while choosing a color (thanks to mCallback->Update).

Comment 12

[Arnaud Bienner]

We still need someone with a Mac to work on this.

Mounir, I know you're quite busy since you left Mozilla, but as you worked on this, any chance for you to have a look at this?

Comment 13

[Arnaud Bienner]

Steven, Markus, Benoit: maybe one of you can have a deeper look to this bug otherwise?
I would have liked to take care of this myself but unfortunately I don't have a Mac :(
As you reviewed Mounir's patch in bug 875756 maybe you can have a look to this bug and help us? (or maybe you know someone who can take care of it?)

If we go for the solution I proposed in comment 11, IMHO that should be very too complicated to implement, supposing I'm right and this can solve the issues we currently have.

Comment 14

[Mounir Lamouri (:mounir)]

Unfortunately, I no longer have a Mac too. Reading at the code, I think one possible scenario is that when we open a new color picker, the previous one gets closed but Update() gets called one the first one. At that point, mColor was set to the value passed to init() but mCallback did not change yet so we end up calling mCallback->Update(mColor) with the new color value...

Comment 15

[Mounir Lamouri (:mounir)]

Forget that previous comment... For some reasons I thought that there was only one instance of nsColorPicker...

Actually, the issue is probably that nsColorPicker is kept alive until Done() is called and Done() is called when the window is closed and I guess the chain of events might be:
- user click on second <input type='color'>
- we create a new NSColorPanelWrapper
- we open a NSColorPanel and update the color
- the previous NSColorPanelWrapper gets a colorChanged event
- the previous NSColorPanelWrapper gets a windowwillClose event

If that's the case, I think one solution would be to make nsColorPicker aware when there is already a native color picker open so it could cleanly handle the transition. One implementation of that would be to have a static nsColorPicker::mColorPanel I guess.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

We backed this out of 28 so it should be unaffected there, tracking for 29 and beyond.

Comment 17

[Arnaud Bienner]

Attachment: bug975468.patch

Comment 18

[André Reinald]

Comment on attachment 8387671 [details] [diff] [review]
bug975468.patch

Looks good to me.

Comment 19

[Arnaud Bienner]

Comment on attachment 8387671 [details] [diff] [review]
bug975468.patch

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

Just for the sake of sanity, as I usually don't write code for OS X, I would like to have Mounir's point of view about this as well, as he worked on this feature.

Comment 20

[Arnaud Bienner]

We tested the patch with Andre and everything seems to work as expected now.

Once thing though, which might be improved (even if it's not a blocking issue): if the input is deleted (e.g. we close the tab/window) the color picker remains opened.
Even if it's safe, as we can change the color in the color picker or close without crashing or any side effect, it would probably be less confusing to close it when it's associated input is removed IMHO.
I will open a follow up bug for this.

Comment 21

[Benoit Girard (:BenWa)]

No time ATM with blockers

Comment 22

[Mounir Lamouri (:mounir)]

Arnaud, I will try to have a look at this week-end. Otherwise on Monday. Please, send me an email if I forget.

Comment 23

[Arnaud Bienner]

Attachment: bug975468-2.patch

Cleaner patch, with commit message, etc.
Marking it as r+ directly as already reviewed (comment 18).

Comment 24

[Arnaud Bienner]

I'm still interested in Mounir's point of view about my patch.
However, I would like to have this patch integrated and, as it has already been reviewed, I believe it's fine to ask it to be checked in.
I'm also clearing needinfo requests which aren't needed anymore now we have a patch ready.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16d6b2e332e0

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out for OSX debug non-unified bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecedfea04ffc

https://tbpl.mozilla.org/php/getParsedLog.php?id=36081314&tree=Mozilla-Inbound

Comment 27

[Arnaud Bienner]

Add missing include. Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=a1b95c7ff8a3

Comment 28

[Arnaud Bienner]

Attachment: bug975468-3.patch

Add missing include.

Comment 29

[Arnaud Bienner]

Push to try failed, but I'm not sure why... the error isn't the same as the one reported on comment 26, and I doubt it's related to my changes, but the error is not very meaningful...
I tried another push to try (https://tbpl.mozilla.org/?tree=Try&rev=94463974f40f), and it failed with the same error.

But I believe this is not related to my changes, and that my patch is OK: marking as "checkin needed" then.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11685d14b3b4

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11685d14b3b4

Comment 33

[Arnaud Bienner]

Comment on attachment 8390811 [details] [diff] [review]
bug975468-3.patch

[Approval Request Comment]

Bug caused by (feature/regressing bug #):
No regression: new feature added in bug 875756.

User impact if declined:
<input type="color"> will appear broken when if user plays with multiple different input type color.

Testing completed (on m-c, etc.): 
on m-c

Risk to taking this patch (and alternatives if risky):
Medium, since it hasn't been in m-c/nightlies for a long time.
However, IMHO we need this fix for FF 29. Otherwise we will probably want to disable input type color on this platform, given that the feature looks broken under some circumstances without this patch. But I believe we will have all the beta stage to detect any regression and that should be enough.

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

Comment 34

[Mounir Lamouri (:mounir)]

Comment on attachment 8390811 [details] [diff] [review]
bug975468-3.patch

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

Sorry for the delay :( But good thing is that looks good to me ;)

Comment 35

[Manuela Muntean [Away]]

Verified as fixed with latest Nightly on Mac OS X 10.8.5.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/869537b5d287

Comment 37

[Manuela Muntean [Away]]

Verified as fixed with latest Aurora 30.0a2 (build ID: 20140318004002) on Mac OS X 10.8.5.

Comment 38

[Manuela Muntean [Away]]

Verified as fixed with Firefox 29 beta 1 on Mac OS X 10.8.5.

Comment 39

[serzhooo]

The bug is still relevant.
Link for demo: https://bug975468.bmoattachments.org/attachment.cgi?id=8379839

Firefox 87.0 (64-bit) macOS, BigSur 11.2.3

Comment 40

[Daniel Holbert [:dholbert]]

Hi serzhooo! Thanks for letting us know. Can you clarify what behavior you're seeing?

Here's what was originally reported as the buggy behavior here:

Actual results:

• The first <input> receives the color of the second <input>.
• Moreover, it is no longer possible to change the color of the first <input> using the color picker.

I just retested with Firefox 89 nightly on BigSur, and I cannot reproduce the first issue (i.e. for me, the first input does not duplicate the color of the second input). But I do see the second issue (I can't change the color of the first input anymore); that might deserve its own bug.

(Side note: it looks like I actually did spin off bug 976724 on that second part, but it was duped back to this one because it was thought that this bug's fix would address it. And it's possible that it did address it at the time but something else is needed for current macOS; who knows.)

Comment 41

[danjah]

Can confirm I am also seeing at least part of this bug come back.

macos Big Sur, FF 88.0:

• opening the colour picker and modifying a colour correctly affects the selected input
• without closing the picker, double click the next input and modifying a colour correctly affects the newly selected input
• now double click the first input again, try to modify the colour - the second input is incorrectly updated instead.

Additionally, in my use case I have a larger number of color inputs on one view, and what our testers have found on the same browser/OS combo is that after trying to manipulate several of the color inputs with the OS picker all inputs become non-functional, except possibly the first one interacted with. No browser crash of hang that I can see, they just don't trigger anything.