Closed Bug 975468 Opened 6 years ago Closed 6 years ago

<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

Categories

(Core :: Widget: Cocoa, defect)

28 Branch
All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox28 - unaffected
firefox29 + verified
firefox30 + verified

People

(Reporter: claude.pache, Assigned: arnaud.bienner)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
Component: Untriaged → Layout: Form Controls
Product: Firefox → Core
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.)
Depends on: 930277
(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).
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: x86 → All
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?
(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?
Flags: needinfo?(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.
Flags: needinfo?(arnaud.bienner)
(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>."
Summary: <input type="color"> , Mac OS X: confusion about the target of the color picker → <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
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?
Flags: needinfo?(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.
Flags: needinfo?(dholbert)
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).
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?
Flags: needinfo?(mounir)
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.
Flags: needinfo?(smichaud)
Flags: needinfo?(mstange)
Flags: needinfo?(bgirard)
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...
Flags: needinfo?(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.
We backed this out of 28 so it should be unaffected there, tracking for 29 and beyond.
Attached patch bug975468.patch (obsolete) — Splinter Review
Attachment #8387671 - Flags: review?(areinald)
Comment on attachment 8387671 [details] [diff] [review]
bug975468.patch

Looks good to me.
Attachment #8387671 - Flags: review?(areinald) → review+
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.
Attachment #8387671 - Flags: review?(mounir)
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.
Blocks: 980996
No time ATM with blockers
Flags: needinfo?(bgirard)
Arnaud, I will try to have a look at this week-end. Otherwise on Monday. Please, send me an email if I forget.
Attached patch bug975468-2.patch (obsolete) — Splinter Review
Cleaner patch, with commit message, etc.
Marking it as r+ directly as already reviewed (comment 18).
Attachment #8387671 - Attachment is obsolete: true
Attachment #8387671 - Flags: review?(mounir)
Attachment #8390530 - Flags: review+
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.
Flags: needinfo?(smichaud)
Flags: needinfo?(mstange)
Keywords: checkin-needed
Add missing include. Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=a1b95c7ff8a3
Add missing include.
Attachment #8390530 - Attachment is obsolete: true
Attachment #8390811 - Flags: review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/11685d14b3b4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Duplicate of this bug: 976724
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
Attachment #8390811 - Flags: approval-mozilla-aurora?
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 ;)
Attachment #8390811 - Flags: review+
Attachment #8390811 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed with latest Nightly on Mac OS X 10.8.5.
Status: RESOLVED → VERIFIED
Verified as fixed with latest Aurora 30.0a2 (build ID: 20140318004002) on Mac OS X 10.8.5.
Verified as fixed with Firefox 29 beta 1 on Mac OS X 10.8.5.
QA Contact: manuela.muntean
You need to log in before you can comment on or make changes to this bug.