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

Font color and font background color throw error while having buttons in the main toolbar and the balloon toolbar #15658

Merged

Conversation

illia-stv
Copy link
Contributor

@illia-stv illia-stv commented Jan 8, 2024

Suggested merge commit message (convention)

Fix (font): Font color and font background should work in both the main toolbar and the balloon toolbar. Closes #15580.

MINOR BREAKING CHANGE (font): colorSelectorView property will no longer accessible from ColorUI plugin in @ckeditor/ckeditor5-font/src/ui/colorui.ts .


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@illia-stv
Copy link
Contributor Author

You could easily reproduce the problem by following next steps:

  • in font-color.js in manual test in Font package import BalloonToolbar from @ckeditor/ckeditor5-ui
  • add into plugin's array BalloonToolbar
  • add into config also balloonToolbar: [ 'bold', 'italic', 'fontColor' ]
  • run manual test and change color by contextual balloon and error will be reproduced

@illia-stv illia-stv self-assigned this Jan 8, 2024
Copy link
Contributor

@pszczesniak pszczesniak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you create a manual test that covers that case? You can place it in tests/manual/tickets/ISSUE_ID - it will be easier to test for everyone and issue will have reference in manual test.

Other than that - solution looks 👍

@illia-stv illia-stv requested a review from pszczesniak January 9, 2024 10:51
Copy link
Contributor

@pszczesniak pszczesniak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@mmotyczynska mmotyczynska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this is not the correct way to fix this bug.

The staticColorsGrid in our case is undefined for a reason. We need to fix it so that it's not undefined.

What happens (current implementation):
If we add more than one toolbar button with the font color (can be in the same toolbar, no need to create a separate toolbar), then in the colorui only the last reference to colorSelectorView is stored in this line:

this.colorSelectorView = addColorSelectorToDropdown( {

In other words, if we have two font color buttons in a toolbar, then the first colorSelectorView is created and stored in this.colorSelectorView, and then the second one is created and overrides the first one.

In this case, we have two views, but only one has a reference in colorui (which is wrong). This is an old regression introduced in ckeditor/ckeditor5-font#39. I don't think anyone noticed it until now, because it's a very rare use case to have the same button twice.

The solution would be to change the line above back to const colorSelectorView and update the rest of the code accordingly, along with the tests that fail.

Copy link
Contributor

@mmotyczynska mmotyczynska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct the test so that it actually verifies the fix.

packages/ckeditor5-font/tests/manual/tickets/15580/1.js Outdated Show resolved Hide resolved
packages/ckeditor5-font/tests/manual/tickets/15580/1.md Outdated Show resolved Hide resolved
packages/ckeditor5-font/tests/ui/colorui.js Outdated Show resolved Hide resolved
packages/ckeditor5-font/tests/ui/colorui.js Outdated Show resolved Hide resolved
@illia-stv illia-stv requested a review from pszczesniak January 15, 2024 14:55
@niegowski niegowski merged commit 63aaa3e into master Jan 19, 2024
6 checks passed
@niegowski niegowski deleted the ck/15580-font-color-and-font-background-color-throw-error branch January 19, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants