968993 - selecting spans of different color in HTML message compose produces CSS warning

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[:aceman]

STR
1.open compose in HTML mode
2.change color of a span of text
3.then select text spans with different colors

Result
Warning: Expected color but found 'mixed'.  Error in parsing value for 'background-color'.  Declaration dropped.
Source file: chrome://messenger/content/messengercompose/messengercompose.xul
Line: 0, Column: 17
Source code: background-color:mixed

The "text color" icon changes color to transparent.

I suspect this is caused by some of the code around onHighlightColorChange() in editor/ui/composer/content/editor.js

Comment 1

[Josiah Bruner [:jsbruner]]

Attachment: Fix.

This makes it so we never set the "mixed" attribute in onBackgroundColorChange(), onFontColorChange(), and onHighlightColorChange().

It also adds a multicolor icon that we use for "mixed" colors, which will require a ui-review.

Blake, sorry about this, but you're my best choice currently. :)

Comment 2

[Josiah Bruner [:jsbruner]]

Attachment: Fix.

Err. So one of those else ifs was suppose to be a new if statement...

Comment 3

[Josiah Bruner [:jsbruner]]

Attachment: Fix.

*Sigh*. Missed something else. Fixed here.

Comment 4

[:aceman]

Comment on attachment 8374442 [details] [diff] [review]
Fix.

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

Isn't the icon missing in the patch?

::: editor/ui/composer/content/editor.js
@@ +950,5 @@
> +      }
> +      
> +      if (button.getAttribute("isMixed")) {
> +        button.removeAttribute("isMixed");
> +      }

Doesn't this remove the attribute you have just set above?

Comment 5

[Josiah Bruner [:jsbruner]]

Attachment: Fix.

My goodness, something's wrong with me today. :) Maybe I need extra caffeine or something. Thanks aceman.

Anyway, *this* should work well.

Comment 6

[:aceman]

Comment on attachment 8374476 [details] [diff] [review]
Fix.

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

Looks better, I'll test it later today.

This may potentially affect Seamonkey so let's CC somebody from there. Also you will probably need to find a reviewr for the /editor part, I don't think bwinton is enough for that module.

::: editor/ui/composer/content/editor.js
@@ +942,5 @@
>      {
>        // No color set - get color set on page or other defaults
> +      if (color == "mixed") {
> +        button.setAttribute("isMixed", "true");
> +      } else if (button.getAttribute("isMixed")) {

I think .hasAttribute() would be better.

Comment 7

[:aceman]

Comment on attachment 8374476 [details] [diff] [review]
Fix.

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

This works great for me, thank :) The icon also has good contrast to me.
I would have liked if we could solve this via pure css, not a new icon file, but maybe we can reuse it at some other place later.

::: editor/ui/composer/content/editor.js
@@ +972,5 @@
> +        button.removeAttribute("isMixed");
> +      }
> +      
> +      if (!color || color == "mixed") {
> +        color = gDefaultTextColor; 

Please kill the trailing whitespace in these lines (in all 3 functions).

Comment 8

[Josiah Bruner [:jsbruner]]

Attachment: Fix.

Addressed Aceman's feedback.

Comment 9

[:aceman]

Comment on attachment 8375713 [details] [diff] [review]
Fix.

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

::: editor/ui/composer/content/editor.js
@@ +947,5 @@
> +        button.removeAttribute("isMixed");
> +      }
> +
> +      if (!color || color == "mixed") {
> +        color = "transparent"; 

Still a space after this line (at least splinter shows it).

@@ +972,5 @@
> +        button.removeAttribute("isMixed");
> +      }
> +
> +      if (!color || color == "mixed") {
> +        color = gDefaultTextColor; 

Still a space after this line (at least splinter shows it).

@@ +997,5 @@
> +        button.removeAttribute("isMixed");
> +      }
> +
> +      if (!color || color == "mixed") {
> +        color = gDefaultBackgroundColor; 

Still a space after this line (at least splinter shows it).

Comment 10

[Josiah Bruner [:jsbruner]]

Attachment: Fix.

Comment 11

[:aceman]

Comment on attachment 8375738 [details] [diff] [review]
Fix.

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

Looks fine now :)

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8375738 [details] [diff] [review]
Fix.

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

I mostly like it, other than the nits below.  Having said that, I'm confident you can fix them without me needing to re-review the patch, so r=me and ui-r=me with those fixed.  :)

Thanks!

::: editor/ui/composer/content/editor.js
@@ +943,5 @@
>        // No color set - get color set on page or other defaults
> +      if (color == "mixed") {
> +        button.setAttribute("isMixed", "true");
> +      } else if (button.hasAttribute("isMixed")) {
> +        button.removeAttribute("isMixed");

https://developer.mozilla.org/en-US/docs/Web/API/Element.removeAttribute says:
"Attempting to remove an attribute that is not on the element doesn't raise an exception."
Therefore we don't need the 'if (button.hasAttribute("isMixed"))', and can just use an "else".

(Also below.)

::: mail/themes/osx/mail/compose/messengercompose.css
@@ +1391,5 @@
>    -moz-margin-end: 9px;
>  }
>  
> +#TextColorButton[isMixed="true"] {
> +  background-image: url("chrome://messenger/skin/icons/multicolor.png");

On OSX at least, I think you also want "background-size: cover;", or it looks like https://dl.dropboxusercontent.com/u/2301433/Screenshots/Multicolour.png
(Better, I think, would be to make the icon the correct size for that area.  ;)

I haven't checked Linux or Windows, but if they have the same problem, please fix them too.

Comment 13

[Ian Neal]

Comment on attachment 8375738 [details] [diff] [review]
Fix.

>+++ b/editor/ui/composer/content/editor.js
It might be worth having a helper function to get rid of the repeated code which is called with the button id and default colour as arguments e.g. ButtonColorChange("HighlightColorButton", "transparent")

The following applies to the three sets of code.

>     if (button)
>     {
>       // No color set - get color set on page or other defaults
This comment probably belongs further down the code (by the !color test) and is not strictly correct any more, as it is either not set or mixed. Comment needs to finish with a full stop too.
>+      if (color == "mixed") {
>+        button.setAttribute("isMixed", "true");
>+      } else if (button.hasAttribute("isMixed")) {
>+        button.removeAttribute("isMixed");
>+      }
>+
>+      if (!color || color == "mixed") {
>+        color = "transparent";
>+      }
For all the if statements, as they are single line, the {} are not needed.
> 
>       button.setAttribute("style", "background-color:"+color+" !important");
Nit: There should be a space either side of the plus signs.

The icon and the theme changes are missing for SeaMonkey.

f- for the moment.

Comment 14

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Here I've added the helper function and addressed the other nits.

However, I do not have a way to test the SM stuff, so let me know if the CSS change works.

Comment 15

[Ian Neal]

Comment on attachment 8378364 [details] [diff] [review]
Patch.

I've not tested as yet, this is just a note of a few things I've spotted in the code.

>+++ b/editor/ui/composer/content/editor.js

>+function ChangeButtonColor(commandID, id, defaultColor) {
Preferred format for the arguments would be to use aCommandID, aID and aDefaultColor

>+  var commandNode = document.getElementById(commandID);
>   if (commandNode)
You could return early here by testing !commandNode

>   {
>     var color = commandNode.getAttribute("state");
>+    var button = document.getElementById(id);
>     if (button)
Again could return early by testing !button

The best person for ui-review on the SeaMonkey css/icon changes would be [e-mail]

I'll try and do some testing in the next few days.

Comment 16

[Josiah Bruner [:jsbruner]]

Ian, are you going to finish up this review, or should I pass it on to Neil?

Comment 17

[Ian Neal]

Yes, I will still review it but you will need a SeaMonkey ui-review from Neil too

Comment 18

[Josiah Bruner [:jsbruner]]

Comment on attachment 8378364 [details] [diff] [review]
Patch.

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

Sounds good. Flagging Neil for a ui-review.

Neil: Definitely let me know what to do with the theme code as well, since I have no idea how SM handles that.

Comment 19

[[e-mail]]

Comment on attachment 8378364 [details] [diff] [review]
Patch.

Drive-by comments:

>+      if (color == "mixed")
>+        button.setAttribute("isMixed", "true");
>+      else
>+        button.removeAttribute("isMixed");
[Could just write button.setAttribute("isMixed", color == "mixed"); or even button.setAttribute("color", color); and then style on #TextColorButton[color="mixed"] instead. Note: I have a slight preference for "mixed" over "isMixed".]

>new file mode 100644
>index 0000000000000000000000000000000000000000..74c558cb11316282b9bec362709edb4bc5b588f5
>GIT binary patch
>literal 2970
Please crush your PNGs. This one only needs to be 164 bytes.

Comment 20

[[e-mail]]

Sadly we don't appear to have precedent for this.
One app simply has an icon with a palette symbol on it that opens the colour picker.
Another app uses a dual menubutton, so the button simply reapplies the last picked colour.

Comment 21

[Ian Neal]

Comment on attachment 8378364 [details] [diff] [review]
Patch.

Yes, this looks great. Good suggestion by Neil wrt mixed vs IsMixed though, f=me with that addressed.

Comment 22

[[e-mail]]

Comment on attachment 8378364 [details] [diff] [review]
Patch.

So, it turns out that, at least in SeaMonkey 2 and later, the editor highlight colour boxes are the wrong size, because the switch from XPFE to toolkit's toolbar.xml removed support for the tbalign="center" attribute. (It was incompatible with customisable toolbars.) Sadly this makes the toolbar stretch all of its items to fit. The highlight colour boxes are supposed to be 14px wide and 12px high (12px by 10px if you allow for the border). Please could you redesign your image to an appropriate size and I'll look into fixing the size of the colour boxes in a separate bug. (And don't forget to crush your image.)

Comment 23

[Josiah Bruner [:jsbruner]]

Perhaps I don't understand, but why does the image need to change? It uses 'cover', so why is a smaller icon required? The icon doesn't need to completely display itself, only fill it.

Comment 24

[[e-mail]]

Because an icon which is the correct size in the first place is superior to one which has been scaled to fit.

Comment 25

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

How does this look? I went with 14 by 12.

Comment 26

[Josiah Bruner [:jsbruner]]

Attachment: Final Patch. Checked-in

There, this uses a different 12 by 10 pixel icon for SM.

I'll land this when the tree reopens...

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/736e032e270e

Comment 28

[Richard Marti (:Paenglab)]

Attachment: 968993-Aero.patch

Aero rule and icon is missing. This patch fixes this.

Comment 29

[Josiah Bruner [:jsbruner]]

Comment on attachment 8397262 [details] [diff] [review]
968993-Aero.patch

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

Good catch. Thanks.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/df12dc77a0c8