Closed Bug 1495808 Opened Last year Closed Last year

Port bug 1416363: Replace use of XUL <colorpicker>

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: jorgk, Assigned: Paenglab)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

Attached patch colorpicker.patch WIP (obsolete) — Splinter Review
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?
Assignee: nobody → richard.marti
Attached patch colorpicker.patch WIP (obsolete) — Splinter Review
Problem 1 is solved.

I can't find a solution for the other two.
Attachment #9013981 - Attachment is obsolete: true
Attached patch colorpicker.patch WIP (obsolete) — Splinter Review
Fixed all prefs picker. Only the picker in composer (EdColorPicker.*) doesn't give the changed colour to the composer.
Attachment #9014131 - Attachment is obsolete: true
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.
Attachment #9014166 - Flags: feedback+
Attached patch colorpicker.patch WIP 4 (obsolete) — Splinter Review
In prefs/Composition the "Restore Defaults" didn't update the buttons -> fixed.

I'll address the nits tomorrow.
Attachment #9014166 - Attachment is obsolete: true
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 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".
Attached patch colorpicker.patch (obsolete) — Splinter Review
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?
Attachment #9014185 - Attachment is obsolete: true
Attachment #9014853 - Flags: review?(acelists)
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.
Attachment #9014853 - Flags: review?(acelists) → review+
Attached patch colorpicker.patch (obsolete) — Splinter Review
Addressed the review comment and changed the strings a bit (reviewed by Jörg over IRC).
Attachment #9014853 - Attachment is obsolete: true
Attachment #9014924 - Flags: review+
Keywords: checkin-needed
Depends on: 1496836
This time all correctly changed.
Attachment #9014924 - Attachment is obsolete: true
Attachment #9014933 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7ee3e492186a
Replace existing usages of XUL colorpicker with input[type='color']. r=aceman,jorgk
Status: NEW → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
There is a bug here. STR:
Set composition background colour to red in the preferences.
Reset default: White, reset default again: Black :-(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
Depends on: 1497041
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.
Attachment #9015139 - Flags: review?(jorgk)
Comment on attachment 9015139 [details] [diff] [review]
1495808-fup.patch

Wrong reviewer ;-)
Attachment #9015139 - Flags: review?(jorgk) → review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d1c3b7b0b9c2
Follow-up: Remove the #ColorPicker height definition on Mac. r=jorgk
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Keywords: checkin-needed
Resolution: --- → FIXED
Blocks: 1497128
Type: defect → task
You need to log in before you can comment on or make changes to this bug.