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

RGBA/HSLA colors are incorrectly encoded #4351

Closed
jacekbogdanski opened this issue Nov 3, 2020 · 9 comments
Closed

RGBA/HSLA colors are incorrectly encoded #4351

jacekbogdanski opened this issue Nov 3, 2020 · 9 comments
Assignees
Labels
good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. plugin:colordialog The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:bug A bug.
Milestone

Comments

@jacekbogdanski
Copy link
Member

Type of report

Bug

Provide detailed reproduction steps (if any)

  1. Open https://ckeditor.com/latest/samples
  2. Select some text.
  3. Open colordialog and add hsla(150,50%,52%,0.2) color.
  4. Open colordialog again.

Expected result

Color should be converted to the correct hashcode including the alpha channel. Note that on the HTML level we are still keeping the original value, so this conversion shouldn't introduce a regression.

It would be good to find out why we are actually unifying colors to hashcode and if it's not really needed, we could instead use the original color value for every color including rgb/hsl.

Actual result

Color is converted to #rgba(71, 194, 133, 0.2)

Other

The issue is also reproducible with rgba colors.

@jacekbogdanski jacekbogdanski added type:bug A bug. status:confirmed An issue confirmed by the development team. plugin:colordialog The plugin which probably causes the issue. labels Nov 3, 2020
@f1ames
Copy link
Contributor

f1ames commented Nov 3, 2020

Related to #4333.

@f1ames
Copy link
Contributor

f1ames commented Nov 3, 2020

It would be good to find out why we are actually unifying colors to hashcode and if it's not really needed, we could instead use the original color value for every color including rgb/hsl.

Maybe it is needed for older IEs since they have only partial support?

@f1ames
Copy link
Contributor

f1ames commented Nov 3, 2020

Hmmm, that's actually quite interesting, this issue relates to a fact that # is added on the beginning of the color name while #4333 to a fact that # (or any other starting character) is removed. So those seems similar, but in fact, this case is related to displaying color name in color dialog (after reopening). And #4333 to displaying color in color history panel.

Still the fact that one part removes # while the other adds it means those mechanisms are related trying to preserve valid color format.

@jacekbogdanski
Copy link
Member Author

jacekbogdanski commented Nov 4, 2020

I suppose we are doing it in at least 2 places (could be wrong). If yes, it would be good to unify it into one function. Playing around with magic strings results in such issues.

Maybe it is needed for older IEs since they have only partial support?

Yeah, looks like, so it would be good to go with hexadecimal values.

@f1ames f1ames added good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. size:S labels Dec 22, 2020
@sculpt0r sculpt0r assigned sculpt0r and unassigned sculpt0r Mar 18, 2021
@Comandeer Comandeer self-assigned this Mar 23, 2021
@Comandeer
Copy link
Member

Comandeer commented Mar 23, 2021

The issue is not as trivial as it seems. The whole normalization of colors happens in colorbutton plugin (which has… two different methods for normalizing – public method and private function). Both these methods assume that color is a hex and adds # at the beginning. Additionally, colordialog plugin adds # on its own.

My proposal is to unify normalization in colorbutton plugin and use hexes generated by CKEDITOR.tools.color exclusively. The only place where the original name of the color would be displayed is the color dialog. The only downside is the fact that it would change the behaviour of the colorbutton and all dependent plugin (including colordialog), but it seems like a move into the right direction. The whole code would be easier to maintain and more colors would be supported just thanks to better normalization.

@Dumluregn
Copy link
Contributor

☝🏻 I strongly agree that a normalisation would be useful and help simplify the whole plugin (actually a few plugins). You are probably aware of it but I want to mention that we also use color dialog in tabletools plugin and the way colors are displayed there is wild (in cell props dialog they are converted to rgb format 🤷🏻 ).

@Comandeer
Copy link
Member

I went a simple way, with just adding support for RGBA/HSLA values and extracted rewriting color handling flow into a separate issue.

@f1ames
Copy link
Contributor

f1ames commented Mar 31, 2021

It should by fixed by #4592. We already have PR here #4591.

@f1ames f1ames added status:blocked An issue which development is blocked by another issue (internal or external one). and removed status:blocked An issue which development is blocked by another issue (internal or external one). labels Mar 31, 2021
@CKEditorBot
Copy link
Collaborator

Closed in #4591

@CKEditorBot CKEditorBot added this to the 4.16.1 milestone Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. plugin:colordialog The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:bug A bug.
Projects
None yet
Development

No branches or pull requests

6 participants