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

Border color parser does not work correctly in some cases #14309

Closed
illia-stv opened this issue Jun 1, 2023 · 2 comments · Fixed by #14334
Closed

Border color parser does not work correctly in some cases #14309

illia-stv opened this issue Jun 1, 2023 · 2 comments · Fixed by #14334
Assignees
Labels
package:engine squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@illia-stv
Copy link
Contributor

illia-stv commented Jun 1, 2023

📝 Provide detailed reproduction steps (if any)

  1. Go to https://ckeditor.com/docs/ckeditor5/latest/features/tables/tables.html
  2. Choose any cell in table
  3. Go to cell properties
  4. Choose style of border and define width e.g. "5px"
  5. Paste this "hsl( 0, 100%, 50% )" into color input for borders

✔️ Expected result

image

❌ Actual result

image

Look, if we remove spaces in color format like this "hsl(0, 100%, 50%)" instead "hsl( 0, 100%, 50% )" it works well.

screen-capture.2.webm

Problem is actually in this part of code.

export function getShorthandValues( string: string ): Array<string> {
return string
.replace( /, /g, ',' ) // Exclude comma from spaces evaluation as values are separated by spaces.
.split( ' ' )
.map( string => string.replace( /,/g, ', ' ) ); // Restore original notation.
}

So for 'hsl( 0, 100%, 50% )' it returns array [ 'hsl(' , ',0, 100%, 50%', ')' ]

Screenshot 2023-06-01 at 13 46 27

It suppose to parse few colors for border (top, bottom, left and right) like:
getShorthandValues( 'red blue RGB(0, 0, 0)'); -> [ 'red', 'blue', 'RGB(0, 0, 0)' ]

❓ Possible solution

return Array.from( string.matchAll( /rgb\([^\)]+\)|hsl(a)?\([^\)]+\)|(#)?\w+/gi ) ).map( i => i[ 0 ] );

If you have ideas, you can list them here. Otherwise, you can delete this section.

📃 Other details

  • Browser: …
  • OS: …
  • First affected CKEditor version: …
  • Installed CKEditor plugins: …

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

@illia-stv illia-stv added type:bug This issue reports a buggy (incorrect) behavior. package:engine labels Jun 1, 2023
@mlewand mlewand added the squad:features Issue to be handled by the Features team. label Jun 1, 2023
@oleq
Copy link
Member

oleq commented Jun 2, 2023

The more general regex from the refinement call \w+\([^)]+\)|(#)?\w+. Should work with colors and with stuff like

calc(10vh - 50px) min(30vh, 100px)

(margins, paddings).

@oleq
Copy link
Member

oleq commented Jun 2, 2023

@oleq oleq self-assigned this Jun 2, 2023
@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Jun 2, 2023
arkflpc added a commit that referenced this issue Jun 6, 2023
…zer-white-spaces

Fix (engine): The CSS shorthand value normalizer should work for values with white spaces. Closes #14309.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Jun 6, 2023
@CKEditorBot CKEditorBot added this to the iteration 64 milestone Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants