Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

normalizeColorOptions in TablePropertiesUI breaks UX of ColorInputData #6791

Closed
tomalec opened this issue May 11, 2020 · 4 comments · Fixed by #6801
Closed

normalizeColorOptions in TablePropertiesUI breaks UX of ColorInputData #6791

tomalec opened this issue May 11, 2020 · 4 comments · Fixed by #6801
Assignees
Labels
package:table type:bug This issue reports a buggy (incorrect) behavior.

Comments

@tomalec
Copy link
Contributor

tomalec commented May 11, 2020

📝 Provide detailed reproduction steps (if any)

This issue was discovered during the review of #6784

  1. Open table properties (or table cell properties).
  2. Set border color and background color to any defined value, for example, "Dim grey".
  3. Close the toolbar.
  4. Reset the state by typing editor.setData(editor.getData()) in the browser console.
  5. Open table (cell) properties once again.

Animated demo of steps above

✔️ Expected result

"Dim grey" should be shown in both input boxes.

❌ Actual result

Border color input shows actual color value: "hsl(0, 0%, 30%)".

📃 Other details

After a brief investigation with @Reinmar we discovered that removing .replace( / /g, '' )

model: color.replace( / /g, '' ),
label: color,
hasBorder: false,
view: {
name: 'span',
styles: {
color
}
}
};
} else {
return {
model: color.color.replace( / /g, '' ),

Solves the problem.
There is a mismatch between normalized and non-normalized values set by the element, and set by the state restore.
We were not sure why this normalization was there, and what side effect we might introduce by removing it. However, it seems to me that we should use, preserve, and accept values in exactly the same shape as they were given in the initial config.

  • Browser: Chrome
  • OS: Win10
  • CKEditor version: master (@fbec6b2af)
  • Installed CKEditor plugins: table

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@jodator
Copy link
Contributor

jodator commented May 12, 2020

@Reinmar & @tomalec there's no "Dim Grey" color in CSS specs :wink: . All CSS color keywords are single word. So basically "Dim grey" should be treated as an error IMO.

Additionally the Firefox & Chrome do not support such values (I've checked if maybe they normalize it somehow):

ps.: The isColor() method do check for proper color names:

const COLOR_NAMES = new Set( [
// CSS Level 1
'black', 'silver', 'gray', 'white', 'maroon', 'red', 'purple', 'fuchsia',
'green', 'lime', 'olive', 'yellow', 'navy', 'blue', 'teal', 'aqua',
// CSS Level 2 (Revision 1)
'orange',
// CSS Color Module Level 3
'aliceblue', 'antiquewhite', 'aquamarine', 'azure', 'beige', 'bisque', 'blanchedalmond', 'blueviolet', 'brown',
'burlywood', 'cadetblue', 'chartreuse', 'chocolate', 'coral', 'cornflowerblue', 'cornsilk', 'crimson', 'cyan',
'darkblue', 'darkcyan', 'darkgoldenrod', 'darkgray', 'darkgreen', 'darkgrey', 'darkkhaki', 'darkmagenta',
'darkolivegreen', 'darkorange', 'darkorchid', 'darkred', 'darksalmon', 'darkseagreen', 'darkslateblue',
'darkslategray', 'darkslategrey', 'darkturquoise', 'darkviolet', 'deeppink', 'deepskyblue', 'dimgray', 'dimgrey',
'dodgerblue', 'firebrick', 'floralwhite', 'forestgreen', 'gainsboro', 'ghostwhite', 'gold', 'goldenrod',
'greenyellow', 'grey', 'honeydew', 'hotpink', 'indianred', 'indigo', 'ivory', 'khaki', 'lavender', 'lavenderblush',
'lawngreen', 'lemonchiffon', 'lightblue', 'lightcoral', 'lightcyan', 'lightgoldenrodyellow', 'lightgray',
'lightgreen', 'lightgrey', 'lightpink', 'lightsalmon', 'lightseagreen', 'lightskyblue', 'lightslategray',
'lightslategrey', 'lightsteelblue', 'lightyellow', 'limegreen', 'linen', 'magenta', 'mediumaquamarine',
'mediumblue', 'mediumorchid', 'mediumpurple', 'mediumseagreen', 'mediumslateblue', 'mediumspringgreen',
'mediumturquoise', 'mediumvioletred', 'midnightblue', 'mintcream', 'mistyrose', 'moccasin', 'navajowhite',
'oldlace', 'olivedrab', 'orangered', 'orchid', 'palegoldenrod', 'palegreen', 'paleturquoise', 'palevioletred',
'papayawhip', 'peachpuff', 'peru', 'pink', 'plum', 'powderblue', 'rosybrown', 'royalblue', 'saddlebrown', 'salmon',
'sandybrown', 'seagreen', 'seashell', 'sienna', 'skyblue', 'slateblue', 'slategray', 'slategrey', 'snow',
'springgreen', 'steelblue', 'tan', 'thistle', 'tomato', 'turquoise', 'violet', 'wheat', 'whitesmoke', 'yellowgreen',
// CSS Color Module Level 4
'rebeccapurple',
// Keywords
'currentcolor', 'transparent'
] );
so maybe it should be displayed in this UI as en error.

@Reinmar
Copy link
Member

Reinmar commented May 12, 2020

@jodator That's not the point. We of course know that "Dim grey" is not a CSS value.

The problem is that some algorithms in the editor work on a normalized CSS color values (with deletes spaces). And some don't. There's a mess now and it leads to the bug reported by @tomalec. We're setting the same color in two inputs and after opening the balloon again it's loaded back in just on of them. WTH, right?

So we tried to debug this and we found out that at one point all spaces are stripped from CSS color values. Why is UI's component ColorGrid stripping spaces from values of colors that were passed to it is a mystery to me. The UI should be about displaying the UI, not doing some unnecessary tricks for apparently no reason.  

My proposal is to:

  • Stop normalizing color values passed to the UI. You configure it to use rgb   (   34, 234, 234) then let it use that value.
  • Use a better matching algorithm here because it's still possible that a normalized or miss-formatted value will come here from the editor data.

@jodator
Copy link
Contributor

jodator commented May 12, 2020

@jodator That's not the point. We of course know that "Dim grey" is not a CSS value.

Oh OK - sorry, I was a bit anchored by the value name and mapped it to a CSS values. I see the problem now. The proposal make sense. Hopefully no one will try to use rgb       (     34, 234, 234) :D

@tomalec
Copy link
Contributor Author

tomalec commented May 12, 2020

To clarify on "Dim grey".
Naturally, it's not the CSS value, but it's our feature to support a custom-defined color list. See

export const defaultColors = [
{
color: 'hsl(0, 0%, 0%)',
label: 'Black'
},
{
color: 'hsl(0, 0%, 30%)',
label: 'Dim grey'
},

What I was referring to, is that this exact feature is bugged due to OP.

@Reinmar Reinmar added this to the iteration 32 milestone May 12, 2020
@tomalec tomalec self-assigned this May 15, 2020
jodator added a commit that referenced this issue May 15, 2020
Fix (table): When the state is restored or the user enters color value manually, color input is able to provide a matching color label. Closes #6791.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:table type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
3 participants