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

Rich Text: Unrelated attributes are applied to Rich Text FormatType #14858

Closed
technote-space opened this issue Apr 7, 2019 · 3 comments · Fixed by #15070
Closed

Rich Text: Unrelated attributes are applied to Rich Text FormatType #14858

technote-space opened this issue Apr 7, 2019 · 3 comments · Fixed by #15070
Labels
[Package] Rich text /packages/rich-text

Comments

@technote-space
Copy link
Contributor

technote-space commented Apr 7, 2019

Describe the bug
There is an error in the program, which causes strange behavior as described in the title.

To reproduce

  1. Register format type which has no attributes
    tagName: span, className: my-span (hereinafter referred to format type A)
  2. Register format type which has style attribute
    e.g. install and activate Advanced Rich Text Tools for Gutenberg plugin
    https://wordpress.org/plugins/advanced-rich-text-tools/
    (e.g. background color)
    (hereinafter referred to format type B)
  3. Input text and apply format type A to Rich Text on block editor
  4. Apply format type B to Rich Text, which is
  • same node as format type A
  • prior to format type A
  • not the beginning of the node
  1. Clear format type B
    ⇒ the attributes that format type B had is applied to format type A
    (the attributes are not applied on the front end or after reloading editor)

Expected behavior
Unrelated attributes should not be applied.

Screenshots
screenshot

Desktop (please complete the following information):

  • OS: Windows10
  • Browser:
    • ❌: Chrome (72.0.3626.121)
    • ❌: FireFox (65.0.2)
    • ✅: Edge (42.17134.1.0)
  • Gutenberg plugin v5.1.0 ~ v5.4.0, WP5.2 beta1

Additional context
https://github.com/WordPress/gutenberg/blob/release/5.4/packages/rich-text/src/to-dom.js#L212

for ( let ii = 0; ii < currentAttributes.length; ii++ ) {
	const { name } = currentAttributes[ ii ];

	if ( ! futureChild.getAttribute( name ) ) {
		currentChild.removeAttribute( name );
	}
}

I think that it should be changed as shown below.

for ( let ii = currentAttributes.length; --ii >= 0; ) {
	const { name } = currentAttributes[ ii ];

	if ( ! futureChild.getAttribute( name ) ) {
		currentChild.removeAttribute( name );
	}
}

I tried the above changes locally and confirmed that they worked correctly.

  • OS: Windows10
  • Browser:
    • ✅: Chrome (72.0.3626.121)
    • ✅: FireFox (65.0.2)
    • ✅: Edge (42.17134.1.0)
  • Gutenberg plugin v5.4.0
@youknowriad youknowriad added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... and removed [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Apr 18, 2019
@youknowriad
Copy link
Contributor

cc @ellatrix in case you have more info here.

@ellatrix
Copy link
Member

@technote-space Thanks! Would you be able to make a PR with the suggested code change and perhaps a test case?

@ellatrix ellatrix added [Package] Rich text /packages/rich-text and removed [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Apr 19, 2019
@technote-space
Copy link
Contributor Author

I created PR #15070
Please review it.

ellatrix pushed a commit that referenced this issue Apr 24, 2019
* #14858

* add test cases, sort attributes before compare (#14858)

* fix lint error

* revert [sort attributes before compare], add test case parameter [expected]  (#14858)

* trivial change

* Simplify

* Remove redundant expected parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Rich text /packages/rich-text
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants