1416363 - Replace the XUL colorpicker widget with input[type=color]

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[Brian Grinstead [:bgrins]]

There's an empty XBL binding for 'colorpickertile':

https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/colorpicker.xml#558
https://dxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#926
https://dxr.mozilla.org/mozilla-central/source/accessible/xul/XULColorPickerAccessible.cpp

It's only bound to images with the .colorpickertile class, and is empty except for setting [role="xul:colorpickertile"]. I suspect that we can copy the role attribute onto each image instead, but we'll need to confirm that the role attribute on a plain XUL image with no XBL is overriding the image role set by Bug 1403231.

You can see these by opening about:preferences, going to "Fonts and Colors" and clicking the "Colors" button.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1416363 - Simplify colorpicker bindings.

Review commit: https://reviewboard.mozilla.org/r/211530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211530/

Comment 2

[Tim Nguyen :ntim]

The attached patch works, except for bug 1429248, which needs to be fixed beforehand.

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 8941254 [details]
Bug 1416363 - Simplify colorpicker bindings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211530/diff/1-2/

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8941254 [details]
Bug 1416363 - Simplify colorpicker bindings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211530/diff/2-3/

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8941254 [details]
Bug 1416363 - Simplify colorpicker bindings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211530/diff/3-4/

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The commit can be more appropriated named "remove XUL Color Picker and replace it with OS Color Picker widget" :)

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #6)
> The commit can be more appropriated named "remove XUL Color Picker and
> replace it with OS Color Picker widget" :)

Indeed. :ntim, can you please check with product / UX about if this is what we want in the preferences UI before requesting code review? In particular we'll lose the ability to specify a set of colors in a palette unless/until we implement the list attr (bug 960984)

Comment 8

[Tim Nguyen :ntim]

Hi Tina,

I am looking to replace the current colorpicker UI with color tiles to choose from, in the Fonts & Colors > Colors section; with a colorpicker that just brings up the native colorpicker.

Would that change be ok ?

Comment 9

[Tim Nguyen :ntim]

Attachment: Before

Comment 10

[Tim Nguyen :ntim]

Attachment: After

Comment 11

[Meridel [:meridel]]

Re-directing question to Michelle, who is providing oversight on Preferences

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8941254 [details]
Bug 1416363 - Simplify colorpicker bindings.

https://reviewboard.mozilla.org/r/211530/#review228054

::: accessible/base/nsAccessibilityService.cpp:1489
(Diff revision 4)
>      accessible = new XULButtonAccessible(aContent, aDoc);
>  
>    } else if (role.EqualsLiteral("xul:checkbox")) {
>      accessible = new XULCheckboxAccessible(aContent, aDoc);
>  
>    } else if (role.EqualsLiteral("xul:colorpicker")) {

Given that you have removed `role="xul:colorpicker"` in the binding, should this moved to XULMap.h instead?

Comment 13

[Tina Hsieh[:Tina_Hsieh] UX]

(In reply to Tim Nguyen :ntim from comment #8)
> Hi Tina,
> 
> I am looking to replace the current colorpicker UI with color tiles to
> choose from, in the Fonts & Colors > Colors section; with a colorpicker that
> just brings up the native colorpicker.
> 
> Would that change be ok ?

Hello Tim,
I'm going to clear the needinfo for now since Michelle is on the needinfo list :)

Comment 14

[Brian Grinstead [:bgrins]]

Renaming this to reflect reality, since colorpickertile is now gone as of Bug 1442029

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The other place where XUL color picker is used is the "select background color" UI in the "Set desktop background" dialog. You can find it by right click on an image and select "Set As Desktop Background...".

It however only shows up on Windows.

Comment 16

[Tim Nguyen :ntim]

Unassigning to reflect real status.

Comment 17

[Tim Nguyen :ntim]

Hi Philipp,

Could you please answer comment 8 or forward to someone who could ?

Comment 18

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Generally this should be fine.
What does this look like on Windows 7 and 10?

Comment 19

[Tim Nguyen :ntim]

Attachment: win-color-picker.png

(In reply to (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #18)
> Generally this should be fine.
> What does this look like on Windows 7 and 10?
It brings up the native windows color picker, which should look like this screenshot.

Comment 20

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Ah, I'm only now realizing that this is the behavior we already have on the web, and that this change will only affect internal pages like about:preferences.
In that case, please go ahead!

Comment 21

[Tim Nguyen :ntim]

Thanks!

Comment 22

[Brian Grinstead [:bgrins]]

(CCing some TB folks). FYI we are planning to replace xul <colorpicker> with <input type="color">. Looks like it's used in a number of places in TB (https://searchfox.org/comm-central/search?q=%3Ccolorpicker&path=) so if you want to keep the current UI you'll need to restore the current impl. FWIW I don't see any red flags that would prevent it from being migrated to a CE: https://github.com/bgrins/xbl-analysis/blob/34c2f839571a2f451b6dc2144c22ce7adcc5d92c/elements/generated/Colorpicker.js.

Comment 23

[Jorg K (CEST = GMT+2)]

Thanks, I filed bug 1495808 for the TB work.

Comment 24

[Tim Nguyen :ntim]

Attachment: Bug 1416363 - Replace existing usages of XUL colorpicker with input[type='color'].

Comment 25

[Tim Nguyen :ntim]

Attachment: Bug 1416363 - Remove colorpicker binding and related code.

Depends on D7575

Comment 26

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04b2b17ca39392f4f1c40512502209052967edc4

Comment 27

[Tim Nguyen :ntim]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d537db19bbefca6980364be8e96181964d6e9b0d

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7335b0ef725a
Replace existing usages of XUL colorpicker with input[type='color']. r=jaws

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/933956374571
Remove colorpicker binding and related code. r=bgrins,surkov

Comment 30

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/7335b0ef725a
https://hg.mozilla.org/mozilla-central/rev/933956374571

Comment 31

[Jorg K (CEST = GMT+2)]

There is a bug here. STR:
Set composition background colour to red in the preferences.
Reset default: White, reset default again: Black :-(

Comment 32

[Jorg K (CEST = GMT+2)]

Grrr, that was for TB, posted to the wrong bug, sorry.