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

New ColorPicker submits form #14361

Closed
The-io-dev opened this issue Jun 8, 2023 · 6 comments · Fixed by #14424
Closed

New ColorPicker submits form #14361

The-io-dev opened this issue Jun 8, 2023 · 6 comments · Fixed by #14424
Assignees
Labels
intro Good first ticket. package:font package:ui squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@The-io-dev
Copy link

The-io-dev commented Jun 8, 2023

📝 Provide detailed reproduction steps (if any)

  1. Have an editor with color picker inside a
  2. open the color picker, and validate a color

✔️ Expected result

The validation of the color should only update the color defined within ckeditor perimeter.

❌ Actual result

it also submits the form

❓ Possible solution

🩹🩹🩹 A workaround is available until fix is released: #14361 (comment) 🩹🩹🩹

the button being of type="submit" it is expected behavior from HTML.

the switch to a type="button" should correct it.

📃 Other details

It might have sens to add an eslint rule to enforce type="button" on all , since the default type value is submit, but i don't know the whole project.


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

@The-io-dev The-io-dev added the type:bug This issue reports a buggy (incorrect) behavior. label Jun 8, 2023
@mlewand mlewand added the squad:features Issue to be handled by the Features team. label Jun 8, 2023
@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Jun 8, 2023
@mlewand mlewand added package:ui package:font and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Jun 8, 2023
@mlewand
Copy link
Contributor

mlewand commented Jun 8, 2023

Thanks for a report! We were clearly doing smoke tests outside of a form element thus we missed this important bug.

I'm adding it to our sprint backlog and we'll release the fix in June release.


Fixing the issue is one thing but I'd like us to avoid having such issues in the future.

Thanks for suggesting linter rule for submits, however we have a good portion of abstraction over the buttons.

Precisely speaking, this is the line where the issue is:

I actually find submit type to be a value here (it adds proper semantics). So what we need here is to properly prevent default's button[type=submit] action.

@mlewand mlewand added the intro Good first ticket. label Jun 8, 2023
@mlewand
Copy link
Contributor

mlewand commented Jun 8, 2023

One way to prevent this is to wrap our manual tests in a form - this could be done in the default template. The action should lead to a page that would show "congratulations you've just sent a form!" which would be a clear indicator whether that was or wasn't a desired behavior.

@mlewand
Copy link
Contributor

mlewand commented Jun 8, 2023

Until we release a proper fix, you can add the following code in your app:

window.setInterval( () => {
	Array.from( document.querySelectorAll( '.ck.ck-color-table .ck-color-picker-page-view [type="submit"]') ).forEach( button => {
		if ( button.dataset.ckeSubmitWorkaround ) {
			return;
		}
	
		button.addEventListener( 'click', event => event.preventDefault() );

		button.dataset.ckeSubmitWorkaround = true;
	} )
}, 300 );

Since it's polling the structure it's not super effective, however there's a simple safeguard to make sure this code is taxing as little as possible.

@illia-stv
Copy link
Contributor

Taken over by #14275 .

@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. and removed status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Jun 12, 2023
@Dumluregn
Copy link
Contributor

There are more buttons that may have the same side effect - e.g. "Save" buttons in table properties and table cell properties. Let's make sure they work as expected too.

illia-stv added a commit that referenced this issue Jun 20, 2023
…mits-form

Fix (font): The accept button in the color picker will no longer submit forms . Closes #14361.
@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 20, 2023
@CKEditorBot CKEditorBot added this to the iteration 64 milestone Jun 20, 2023
@AliaksandrBortnik
Copy link

We faced the same issue related to our form submissions due to the submit button of color picker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
intro Good first ticket. package:font package:ui 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.

6 participants