Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Improved filtering out disallowed attributes on child elements. #1790

Merged
merged 5 commits into from
Sep 13, 2019
Merged

Conversation

oskarwrobel
Copy link
Contributor

@oskarwrobel oskarwrobel commented Sep 4, 2019

Suggested merge commit message (convention)

Fix: Improved filtering out disallowed attributes on child elements. ckeditor/ckeditor5#4535.


Additional information

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

@coveralls
Copy link

coveralls commented Sep 4, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling f6a31f8 on t/1789 into 30de8c7 on master.

Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

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

Please check the test that I added.

Unfortunately, this is much harder than this. Merging nodes creates completely new nodes on the fly – ones which are not present in the beginning.

@Reinmar
Copy link
Member

Reinmar commented Sep 5, 2019

We need to be creative and write a couple more tests where text nodes would be merged during the process. I fear that there's a range of potentially broken implementations that we need to avoid.

I think that the implementation may look like this:

  1. Scan the list of nodes for incorrect attributes. Create a range for each of those places.
  2. Iterate over the list of those ranges and pass them to removeAttribute().

This will be safe because merging text nodes does not change positions.

@Reinmar
Copy link
Member

Reinmar commented Sep 5, 2019

BTW, I don't remember why we're checking whether the right type of operation is used. I think that it is enough if one or two tests check that. The other ones don't have to.

@oskarwrobel oskarwrobel requested a review from Reinmar September 11, 2019 12:46
@Reinmar
Copy link
Member

Reinmar commented Sep 13, 2019

That was a really cool idea with range.getPositions()! Iterating over the range and taking value.items would not work because it'd hit the same problem. So the only alternative would be iterating over the range and using nodeBefore || parent as you did, so I actually prefer your solution.

@Reinmar
Copy link
Member

Reinmar commented Sep 13, 2019

I only removed continue; because it makes reading the code harder. Because you actually need to understand that what's later in this loop will not happen when the former happened. if-else represents that semantics more directly.

@Reinmar Reinmar merged commit c5033b6 into master Sep 13, 2019
@Reinmar Reinmar deleted the t/1789 branch September 13, 2019 08:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants