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

I/6154: Allow selection on object element (ie. on table cells) #1820

Merged
merged 27 commits into from
Feb 3, 2020
Merged

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Jan 28, 2020

Suggested merge commit message (convention)

Enhancement: Allow selection on object elements. Closes ckeditor/ckeditor5#6154.


Additional information

  • This PR introduces required selection post-fixer changes for table selection.

  • Those are a minimal engine changes required to allow multi-range selection on object elements (table cells).

  • Two tests in image and table were required update because a test case:

[]<object></object>

was post-fixed by proposed post-fixer to:

[<object></object>]

fixed to:

<paragraph>[]</paragraph><object></object>

This case might need to be checked wheter it is OK or not.

Sub PRs:

@coveralls
Copy link

coveralls commented Jan 28, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 007da5b on i/6154 into 48bea53 on master.

@Reinmar
Copy link
Member

Reinmar commented Jan 29, 2020

From what I can see you mostly removed outdated code and added this single function for fixing intersecting ranges. That sounds fine. I guess we could have some more tests for some less and more ugly scenarios. E.g.:

  • a couple of cells selected, but not like now when one's directly next to another,
  • some selection around rows – like [<td>x</td>]<td>x</td>[<td>x</td></tr>],

Also, I thought about a case when one range is inside a cell and the other outside. Maybe it should be a general rule that if a single range is an object range, all ranges must be object ranges?

@Reinmar Reinmar self-requested a review January 29, 2020 11:16
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.

As commented.

@jodator
Copy link
Contributor Author

jodator commented Jan 30, 2020

@Reinmar I've added some scenarios and when thinking about other ones it looked like they're covered elsewhere (mostly by the limit boundary-crossing tests). One test already spotted some error in the code so that one got fixed.

@jodator jodator requested a review from Reinmar January 30, 2020 14:16
Copy link
Contributor Author

@jodator jodator left a comment

Choose a reason for hiding this comment

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

Self R- 👎

Do not merge.

src/model/utils/selection-post-fixer.js Show resolved Hide resolved
@jodator
Copy link
Contributor Author

jodator commented Jan 31, 2020

Self R+ 👍

It turned out to be some problems with manual tests during refactoring.

Review ;)

@Reinmar Reinmar merged commit 0dec72d into master Feb 3, 2020
@Reinmar Reinmar deleted the i/6154 branch February 3, 2020 15:45
@Reinmar
Copy link
Member

Reinmar commented Feb 3, 2020

BTW, there's no "Enhancement" type of change yet. I usually use "Other".

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.

Allow selection to be set on object elements
3 participants