Closed Bug 1416363 Opened 2 years ago Closed Last year

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

Categories

(Toolkit :: XUL Widgets, task, P5)

task

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox64 --- fixed

People

(Reporter: bgrins, Assigned: ntim)

References

Details

(Whiteboard: [xbl-flatten-inheritance])

Attachments

(6 files)

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.
Priority: -- → P5
Assignee: nobody → ntim.bugs
The attached patch works, except for bug 1429248, which needs to be fixed beforehand.
Depends on: 1429248
The commit can be more appropriated named "remove XUL Color Picker and replace it with OS Color Picker widget" :)
(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)
See Also: → 960984
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 ?
Flags: needinfo?(thsieh)
Attached image Before
Attached image After
Flags: needinfo?(mwalkington)
Re-directing question to Michelle, who is providing oversight on Preferences
Flags: needinfo?(mwalkington) → needinfo?(mheubusch)
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?
(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 :)
Flags: needinfo?(thsieh)
Renaming this to reflect reality, since colorpickertile is now gone as of Bug 1442029
Depends on: 1442029
Summary: Remove the colorpickertile binding, which is used only for setting a role → Consider replacing the XUL colorpicker widget with input[type=color]
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.
Unassigning to reflect real status.
Assignee: ntim.bugs → nobody
Hi Philipp,

Could you please answer comment 8 or forward to someone who could ?
Flags: needinfo?(philipp)
Generally this should be fine.
What does this look like on Windows 7 and 10?
Flags: needinfo?(philipp)
Attached image 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.
Flags: needinfo?(philipp)
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!
Flags: needinfo?(philipp)
Thanks!
Flags: needinfo?(mheubusch)
Assignee: nobody → ntim.bugs
(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.
Status: NEW → ASSIGNED
Summary: Consider replacing the XUL colorpicker widget with input[type=color] → Replace the XUL colorpicker widget with input[type=color]
Blocks: 1470830
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7335b0ef725a
Replace existing usages of XUL colorpicker with input[type='color']. r=jaws
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/933956374571
Remove colorpicker binding and related code. r=bgrins,surkov
Depends on: 1496836
https://hg.mozilla.org/mozilla-central/rev/7335b0ef725a
https://hg.mozilla.org/mozilla-central/rev/933956374571
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.