1495808 - Port bug 1416363: Replace use of XUL <colorpicker>

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Jorg K (CEST = GMT+2)]

Nine call sites:
https://searchfox.org/comm-central/search?q=%3Ccolorpicker&case=false&regexp=false&path=

Comment 1

[Richard Marti (:Paenglab)]

Attachment: colorpicker.patch WIP

This converts the picker to the html input.

Problems:
- The picker in Display/Plain text Messages and Display/Colors... don't show initially the correct colour. I think with assigning the colours on init should fix it. I'll try it later.
- The Tag colorpicker in Display/Tags does show initially the colour but doesn't set it when click Okay in dialog.
- The same happens on the foreground/background colorpicker in composer. Could somebody look please?

Comment 2

[Richard Marti (:Paenglab)]

Attachment: colorpicker.patch WIP

Problem 1 is solved.

I can't find a solution for the other two.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: colorpicker.patch WIP

Fixed all prefs picker. Only the picker in composer (EdColorPicker.*) doesn't give the changed colour to the composer.

Comment 4

[:aceman]

Comment on attachment 9014166 [details] [diff] [review]
colorpicker.patch WIP

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

Thanks for taking this.

Sadly on Linux the composer editor color picker is stuck and inoperable yet. But the pickers in prefs do work fine.

::: editor/ui/dialogs/content/EdColorPicker.js
@@ +166,1 @@
>  function ChangePalette(palette)

Remove this function.

::: editor/ui/dialogs/content/EdColorPicker.xul
@@ +33,1 @@
>      persist="palettename"

Drop this 'persist' attribute when there is no "palettename" attribute anymore.

@@ +34,5 @@
>      onclick="SetDefaultToOk();"
>      ondblclick="if (onAccept()) window.close();"
>      onkeypress="SelectColorByKeypress(event);"
>      onselect="SelectColor();"/>
>  <!-- Web palette is not implemented???

So this web palette will probably never get implemented and you also remove "palettename" (as m-c does) so drop this comment block.

::: mail/components/preferences/colors.xul
@@ +27,5 @@
> +    function init() {
> +      document.getElementById("foregroundtextmenu").value = document.getElementById("browser.display.foreground_color").value;
> +      document.getElementById("backgroundmenu").value = document.getElementById("browser.display.background_color").value;
> +      document.getElementById("unvisitedlinkmenu").value = document.getElementById("browser.anchor_color").value;
> +      document.getElementById("visitedlinkmenu").value = document.getElementById("browser.visited_color").value;

Wrap these lines.

::: mail/components/preferences/compose.js
@@ +32,5 @@
>          document.getElementById("composePrefs").selectedIndex = preference.value;
>      }
>  
> +    document.getElementById("textColorButton").value = document.getElementById("msgcompose.text_color").value;
> +    document.getElementById("backgroundColorButton").value = document.getElementById("msgcompose.background_color").value;

Wrap long lines.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: colorpicker.patch WIP 4

In prefs/Composition the "Restore Defaults" didn't update the buttons -> fixed.

I'll address the nits tomorrow.

Comment 6

[:aceman]

The compose text color dialog (from editor) contains also elements that seem redundant now:
<textbox id="ColorInput" style="width: 8em" oninput="SetColorSwatch(); SetDefaultToOk();"/>
<spacer id="ColorPickerSwatch"/>

The textual display of the color code (ColorInput) is part of the new picker from <input type="color"> so does not need to be duplicated here. At least on Linux, can you check other platforms?
Well, the new picker is not a 100% replacement, as you could input named colors in the field, e.g. "blue". In the new picker you can do that too, save the result and reopen the picker and you see the name will be changed to the #RRGGBB representation of that "blue" color.

The ColorPickerSwatch shows the chosen color on a small rectangle. This is also redundant now as the color is shown directly on the <input type="color"> "button".

Can we now remove these and the code referencing them?

Comment 7

[:aceman]

Comment on attachment 9014185 [details] [diff] [review]
colorpicker.patch WIP 4

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

::: editor/ui/dialogs/content/EdColorPicker.xul
@@ -32,5 @@
>      persist="palettename"
>      onclick="SetDefaultToOk();"
>      ondblclick="if (onAccept()) window.close();"
>      onkeypress="SelectColorByKeypress(event);"
>      onselect="SelectColor();"/>

I'm not sure 'onselect' works here, see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/color . Maybe it needs to be "onchange" (as inhttps://bugzilla.mozilla.org/attachment.cgi?id=9013924) or "oninput".

Comment 8

[Richard Marti (:Paenglab)]

Attachment: colorpicker.patch

The m-c patches are now in m-i and will land on one of the next merges.

This works on Windows. The problem on Linux is the modal opening of the EdColorPicker.xul. But when I remove the modal the picker works but the opening dialogs don't get the colour, also not on Windows.

Maybe you have a solution for this problem?

Comment 9

[:aceman]

Comment on attachment 9014853 [details] [diff] [review]
colorpicker.patch

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

Thanks, this seems to work nicely now.

Only the color picker in compose foreground/background is still stuck and inoperable.
However, we have found out, that the dialog newTagDialog.xul is opened from prefs (display->tags->new) and there the picker works fine. When the same dialog is opened from Message->Tag->New tag, the picker gets stuck. The difference is that the latter dialog is opened as "modal". I can assume that may be a toolkit but and I'll file that against m-c.

::: mail/components/preferences/compose.js
@@ +33,5 @@
>        if (preference.value)
>          document.getElementById("composePrefs").selectedIndex = preference.value;
>      }
>  
> +

Do not add this empty line.

Comment 10

[Richard Marti (:Paenglab)]

Attachment: colorpicker.patch

Addressed the review comment and changed the strings a bit (reviewed by Jörg over IRC).

Comment 11

[Richard Marti (:Paenglab)]

Attachment: colorpicker.patch

This time all correctly changed.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7ee3e492186a
Replace existing usages of XUL colorpicker with input[type='color']. r=aceman,jorgk

Comment 13

[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 14

[Richard Marti (:Paenglab)]

Closing and reopening the prefs shows then the correct colours. And after the second reset, msgcompose.background_color has #FFFFFF. When opening the composer the correct colours are used, so only a display error. Maybe a race with the reset functions?

Comment 15

[Richard Marti (:Paenglab)]

Attachment: 1495808-fup.patch

I tried the patch on Mac and the colour button in composer colour dialog is too tall and doesn't look good. Removing the height definition makes the button look better. The coloured area inside the button is then still big enough.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 9015139 [details] [diff] [review]
1495808-fup.patch

Wrong reviewer ;-)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d1c3b7b0b9c2
Follow-up: Remove the #ColorPicker height definition on Mac. r=jorgk