Closed Bug 977301 Opened 10 years ago Closed 10 years ago

Disable <input type="color"> for the Firefox 28 release

Categories

(Core :: Layout: Form Controls, defect, P2)

Product:
Core
Component:
Layout: Form Controls
Version:
28 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla28
Tracking Flags:
Tracking Status
firefox28 + verified
relnote-firefox --- 29+

People

(Reporter: dholbert, Assigned: arnaud.bienner)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [testday-20140227])

Attachments

(1 file, 1 obsolete file)

disable_input_type_color.patch
5.23 KB, patch
dholbert
: review+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review 
We're discovering some issues with the new <input type="color"> feature during its time on Beta channel.

Rather than shipping a semi-broken feature, I'm leaning towards turning it off for Firefox 28 (which will hopefully allow us enough time to address the issues that have been discovered, and ship a much better version in Firefox 29).

In particular, the bugs worrying me are:
 - Mac OS X bugs bug 975468, bug 976724 [which, per bug 976724 comment 9 through 11, are alreadly likely to make us disable this feature at least on Mac]
 - Windows bug 944737 (which is supposed to be fixed, via allowing only one colorpicker at a time, though Gabriela's reports there (e.g. bug 944737 comment 58) seem to indicate that the fix might be incomplete)
 - Non-platform-specific bug 977038 and bug 977029
 - And possibly bug 949403 (it worries me slightly to adjust the baseline of a form control down the line, after web designers have already positioned that control at precise spots in their layout)

Any one of these probably wouldn't block shipping this feature, but collectively, I think they mean we should punt on this for at least one release.
(In reply to Daniel Holbert [:dholbert] from comment #0)
>  - Mac OS X bugs bug 975468, bug 976724 [which, per bug 976724 comment 9
> through 11, are alreadly likely to make us disable this feature at least on
> Mac]

Indeed: we should disable input type color for OS X.

>  - Windows bug 944737 (which is supposed to be fixed, via allowing only one
> colorpicker at a time, though Gabriela's reports there (e.g. bug 944737
> comment 58) seem to indicate that the fix might be incomplete)

I'm still a bit skeptical about this bug, as no one else is able to reproduce it. I don't mean Gabriela is wrong, but just that it might under special conditions which aren't clear yet.

>  - Non-platform-specific bug 977038 and bug 977029

...which have the same root cause, and a patch (almost) ready to fix it.

>  - And possibly bug 949403 (it worries me slightly to adjust the baseline of
> a form control down the line, after web designers have already positioned
> that control at precise spots in their layout)

Doesn't sound like a big issue for me too.

> Any one of these probably wouldn't block shipping this feature, but
> collectively, I think they mean we should punt on this for at least one
> release.

So, for me, only 2 bugs: Mac OS bug 975468, and non-platform specific bug 977038.

But yes maybe safer to disable it for FF 28. Another good reason for this is consistency: we have will not have the feature enabled on all desktop platforms (OS X missing), which is a bit strange.
And also, even if I'm confident we can fix these bugs before FF 28 hits stable, there will be not lot of time remaining for beta testers to check there is no other bugs, or regression caused by the fixes we are going to do.
Firefox 28 hits stable _today_ for your purposes (the last beta for non-critical bugs is today).  I seriously doubt you can get those bugs fixed and into all branches today.

That also means that the disabling needs to land today (ASAP), if we're doing it.
(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #2)
> Firefox 28 hits stable _today_ for your purposes (the last beta for
> non-critical bugs is today).  I seriously doubt you can get those bugs fixed
> and into all branches today.

I didn't know; I thought it will hit stable the 17th March.
17th March is when users get it.  Today is the last day you can check things in that are not fixing critical stop-ship issues.
I'm able to reproduce the bug (I can open the color picker in both windows) using Windows XP and Firefox 28 Beta 6.

ID Build: Mozilla/5.0 (Windows NT 5.1; rv:28.0) Gecko/20100101 Firefox/28.0
Buildconfig: Built from https://hg.mozilla.org/releases/mozilla-beta/rev/303a852bcba5
(In reply to Gabriela from comment #5)
> I'm able to reproduce the bug (I can open the color picker in both windows)
> using Windows XP and Firefox 28 Beta 6.

Gabriela, as discussed in bug 944737 comment 59, could you please open a new bug for this issue?
Please include all the step you perform to reproduce to get this result (as we can't reproduce it, I suspect it happens under certain circumstances only).
Also, I will be interesting to know if in your case it results in the same rendering issues originally described in bug 944737 comment 1?
Arnaud, I will open a new bug as soon as I can including all the steps I perform to reproduce it. (I just follow Moztrap's testcase #11269).
I don't have any rendering issues.
Whiteboard: [testday-20140227]
Attached patch disable_input_type_color.patch (obsolete) — — Splinter Review 
Patch to disable input type color (DOM support + styling through eIntID_ColorPickerAvailable/-moz-system-metric(color-picker-available)) for all platforms.
I've also updated reftest, because most of them should fail now.
I have been able to test this patch only on Windows.

I let you decide if you want to disable input type color; but if you go for it, hopefully this would help.
Attachment #8383040 - Flags: feedback?(dholbert)
Comment on attachment 8383040 [details] [diff] [review]
disable_input_type_color.patch

Thanks, Arnaud!

(and thanks bz for the heads-up about the time-crunch on this; I hadn't realized that today was the last day for non-critical bugs)

>+++ b/layout/reftests/forms/input/color/reftest.list
>@@ -1,14 +1,14 @@
> # Simple test. Should fail on platforms where input type color isn't activated
> # yet. Missing platforms are B2G (bug 875751), Android (bug 875750) and Metro
> # (bug 895464).
>-fails-if(B2G||Android) == input-color-1.html input-color-1-ref.html
>+!= input-color-1.html input-color-1-ref.html

Semantic nit: These should be annotated as "fails ==" instead of "!="

> # Despite the "default-preferences" line above, B2G and Android are still
> # excluded from some style in forms.css, which makes the following tests fail.
>-fails-if(B2G||Android) == margin-padding-1.html margin-padding-1-ref.html
>+!= margin-padding-1.html margin-padding-1-ref.html

We should drop that comment, since it doesn't make sense without the fails-if(B2G||Android) annotations that are being removed here.

Also: looks like mobile/android/app/mobile.js still has the pref turned on; we should disable it there, too. (We should actually remove the line there, so that it can rely on all.js. I think that line is just cruft from before we had a color picker implementation on android.)
Attachment #8383040 - Flags: feedback?(dholbert) → feedback+
Updated with your comments applied.
Haven't changed mobile.js, as we should apply this patch on top of bug 977643's patch IMHO.
If it makes things, easier, I can change mobile.js as part of this patch though.
Attachment #8383040 - Attachment is obsolete: true
Comment on attachment 8383093 [details] [diff] [review]
disable_input_type_color.patch

Looks good to me. It should be fine to layer this on top of bug 977643, for the mobile.js tweak, I think.
Attachment #8383093 - Flags: review+
Comment on attachment 8383093 [details] [diff] [review]
disable_input_type_color.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 930277 enabled this feature; various bugs mentioned in comment 0 are what are prompting us to disable it (for now)

User impact if declined: weird behavior; see bugs in comment 0. (basically, incorrect behavior when users try to interact with more than one color picker, and when users use form reset buttons)

Testing completed (on m-c, etc.): verified that reftests pass locally (indicating that it is indeed being disabled).

Risk to taking this patch (and alternatives if risky): low; just a pref tweak (along with a nsLookAndFeel change, which is what controls the styling of input[type="color"] in forms.css)

String or IDL/UUID changes made by this patch: none
Attachment #8383093 - Flags: approval-mozilla-beta?
Depends on: 977643
Manuela, can you please make sure this is tested in the next Beta?
Flags: needinfo?(manuela.muntean)
QA Contact: manuela.muntean
Keywords: verifyme
Assignee: nobody → arnaud.bienner
Priority: -- → P2
(In reply to Arnaud Bienner from comment #6)
> (In reply to Gabriela from comment #5)
> > I'm able to reproduce the bug (I can open the color picker in both windows)
> > using Windows XP and Firefox 28 Beta 6.
> 
> Gabriela, as discussed in bug 944737 comment 59, could you please open a new
> bug for this issue?
> Please include all the step you perform to reproduce to get this result (as
> we can't reproduce it, I suspect it happens under certain circumstances
> only).
> Also, I will be interesting to know if in your case it results in the same
> rendering issues originally described in bug 944737 comment 1?

I opened a new bug and I CC'd to you: https://bugzilla.mozilla.org/show_bug.cgi?id=977828.
I didn't have any rendering issues.
Attachment #8383093 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Sylvestre is away [as it's midnight in his timezone] and #planning is quiet, so I'm going to assume the approval-mozilla-beta+ implicitly applies to bug 977643 as well (which per comment 10, could really be considered part of this bug; we only split it out because it's cleanup that's worth taking on all branches, unlike the rest of this bug).

I'll land both patches on beta later today.
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #15)
> I'll land both patches on beta later today.

(Actually, I'm going to do that tomorrow morning, which lsblakk assures me should be fine.)
Flags: needinfo?(dholbert)
Flags: needinfo?(dholbert)
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #13)
> Manuela, can you please make sure this is tested in the next Beta?

Due to comment 16, this skipped Firefox 28 beta 7. I'll test this as soon as beta 8 builds are available.
Flags: needinfo?(manuela.muntean)
Landed on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/38792cacf700
Flags: needinfo?(dholbert)
[setting "in-testsuite+", because Arnaud updated the reftests for this feature to be labeled "fails", which tests that this actually took effect.]
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
status-firefox28: affected → fixed
Target Milestone: --- → mozilla28
I updated Nucleus and I changed the release notes about <input type="color"> from 28 to 29.
relnote-firefox: ? → 29+
removing tracking and status for > 28 as we are indeed shipping this feature in 29.
status-firefox29: affected → ---
status-firefox30: affected → ---
tracking-firefox29: + → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: