-
Notifications
You must be signed in to change notification settings - Fork 40
T/1436: Fixing selection that cross limit node boundaries. #1450
Conversation
const start = range.start; | ||
const end = range.end; | ||
|
||
// Flat range on the same text node is always valid - no need to fix: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this comment isn't entirely correct – I mean the "same" part. What about:
<p>f[oo<inlineWidget/>ba]r</p>
Also, what about:
<p>[foo]</p>
In this case, neither the start's nor end's textNode
getter will return something.
And there's one more case which is missed here:
<p>[<inlineWidget/>foo<inlineWidget/>]</p>
You might've had a different goal by adding this condition – filtering some cases which might be broken by the following logic of this function. But, if the logic of such a check isn't "complete" in a bigger picture, a wider context of the entire algorithm, then it may be misleading. So, instead of patching this algorithm for a hidden purpose, let's thing wider.
E.g. what kind of non-collapsed selections don't need to be fixed? Those which meet the following criteria (all):
-
schema.getLimitElement( range.start ) == schema.getLimitElement( range.end )
– it means the selection starts and ends in the same limit element, so it does not cross any limit element boundary. If a range violates this rule, it needs to be extended to cover the lowest common limit element of both its ends (schem.getLimitElement( range )
).Note: Right now,
schema.getLimitElement()
works only with entire selections, but it's not hard to "downgrade" it to support single ranges (returns LCA like for the entire selection) and single positions (returns a lowest limit element in which this position is contained) too. -
schema.checkChild( range.start, '$text' ) && schema.checkChild( range.end, '$text' )
– both its ends are in a place where a text is allowed. They don't need to be placed in/next to a text (<p>[<inlineWidget/>...
) – it's enough that a text is allowed there.Note: A range containing a block widget will not be rooted in a place where text is allowed, so it will not pass this check. However, it doesn't mean that it's incorrect – I was describing a range which is certainly correct (i.e. a possible early return check), not an algorithm for deciding whether a range is correct or not.
Therefore, if a range violates this rule, it needs to be checked further:
- Does it select an object element? If yes, it's ok. If not it needs to be fixed.
- Hm... and that seems to be all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By structuring the algorithm as I described above, the rules by which it works will be easier to read. Right now, it consists of "let's try fixing this, let's try that" which is harder for me to digest.
@@ -1123,12 +1123,12 @@ describe( 'Schema', () => { | |||
schema.extend( 'img', { allowAttributes: 'bold' } ); | |||
schema.extend( '$text', { allowIn: 'img' } ); | |||
|
|||
setData( model, '[<p>foo<img>xxx</img>bar</p>]' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the semantics of this test. Let's better use parse()
, especially that later on we do selection.getRanges()
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still not resolved.
@@ -59,37 +63,41 @@ describe( 'Selection post-fixer', () => { | |||
} ); | |||
|
|||
it( 'should react to structure changes', () => { | |||
setModelData( model, '<paragraph>[]foo</paragraph><image></image>' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was wrong with this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, you're simplifying the test.
OK, so far, the most important bit is my comment about structuring this algorithm. I think you could try to restructure it so the rules by which it works are more visible. I think I described the outline in my comment, but of course that's just the outline. You also need the "fix it" parts. In general, it should look more or less like this:
This way you can easily separate the "check" from the "fix" code (you can even have a set of |
@Reinmar ready to re-review. |
src/model/schema.js
Outdated
@@ -586,22 +586,32 @@ export default class Schema { | |||
* Returns the lowest {@link module:engine/model/schema~Schema#isLimit limit element} containing the entire | |||
* selection or the root otherwise. | |||
* | |||
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selection | |||
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selectionOrRangeOrPosition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to update the type.
@@ -542,7 +542,7 @@ describe( 'downcast-selection-converters', () => { | |||
it( 'should add a class to the selected table cell', () => { | |||
test( | |||
// table tr#0 |td#0, table tr#0 td#0| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the comments up to date now?
@@ -551,9 +551,9 @@ describe( 'downcast-selection-converters', () => { | |||
it( 'should not be used if selection contains more than just a table cell', () => { | |||
test( | |||
// table tr td#1, table tr#2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here too?
tests/model/schema.js
Outdated
schema.extend( 'article', { isLimit: true } ); | ||
schema.extend( 'section', { isLimit: true } ); | ||
|
||
setData( model, '<div><section><article><paragraph>foo[]bar</paragraph></article></section></div>' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you set a selection anywhere if you're testing if this method works with a range?
You can use parse()
here, btw.
tests/model/schema.js
Outdated
@@ -1101,7 +1123,13 @@ describe( 'Schema', () => { | |||
} | |||
} ); | |||
|
|||
setData( model, '<p>foo<img />bar</p>' ); | |||
// Parse data string to model. | |||
const parsedResult = setData._parse( '[<p>foo<img />bar</p>]', model.schema, { context: [ root.name ] } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the public export.
tests/model/schema.js
Outdated
|
||
// Set parsed model data to prevent selection post-fixer from running. | ||
model.change( writer => { | ||
writer.insert( parsedResult.model, root ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure you need to insert that into the model. Perhaps it works without that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know how to do that - AFAICS it's easiest to just use writer.insert( node, root)
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method we're testing there schema.getValidRanges()
doesn't need to work on on a piece of a tree which is in the model. At least – I think that doesn't need that. It should be able to work on a detached tree.
OTOH, we can leave it with the insert()
and test it just like it will be used most of the time (in an attached tree).
@@ -621,7 +621,7 @@ describe( 'DataController utils', () => { | |||
deleteContent( model, selection ); | |||
|
|||
expect( getData( model, { rootName: 'bodyRoot' } ) ) | |||
.to.equal( '[<paragraph>x</paragraph>]<paragraph></paragraph><paragraph>z</paragraph>' ); | |||
.to.equal( '<paragraph>[x]</paragraph><paragraph></paragraph><paragraph>z</paragraph>' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can change the initial selection too, so this test is less confusing (we're not checking the sel post-fixer here).
tests/model/utils/deletecontent.js
Outdated
new Position( doc.getRoot( 'bodyRoot' ), [ 1 ] ), | ||
new Position( doc.getRoot( 'bodyRoot' ), [ 2 ] ) | ||
); | ||
writer.setSelection( range ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to set that selection. deleteContent()
can work with any selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it'd be good if you changed it in the setData()
call above to something irrelevant to this test to not create a confusion.
tests/model/utils/deletecontent.js
Outdated
new Position( doc.getRoot( 'bodyRoot' ), [ 1 ] ), | ||
new Position( doc.getRoot( 'bodyRoot' ), [ 3 ] ) | ||
); | ||
writer.setSelection( range ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as above– no need to set it. Also, it'd be good if you changed it in the setData()
call above to something irrelevant to this test to not create a confusion.
@Reinmar the fix is on the branch already. |
🎉 |
@Mgsy As this looks like finished could you check how the selection on widgets behaves now? Also this one fixes selection on images in firefox: https://github.com/ckeditor/ckeditor5-engine/issues/1440 (not the flashing part though - only the selection problems). Also on FF the multi-range selection that is fixed (ie on widgets) should be merge into one big (https://github.com/ckeditor/ckeditor5-engine/issues/1440#issuecomment-402653775). |
@Mgsy you mean that it blinks? If so I don't think we could do anything in particular in this PR as this is a bit different issue (cc @Reinmar) - we need to stop selection updates somehow do prevent this. I think that a follow-up is needed here.
It looks OK (selecting whole table) - could you describe what you mean here since I've might be missing something from the GIF. |
Agree with @jodator regarding the first part – we need to extract this to a separate ticket. |
Yep, that is what I meant - selecting the whole table. As this behaviour is expected, the whole fix looks good to me 👌 |
Suggested merge commit message (convention)
Other: Fixing selection that cross limit node boundaries. Closes ckeditor/ckeditor5#4360.
Other: The
schema.getLimitElement()
method now accepts alsoRange
andPosition
as a parameter.Additional information
This PR comes with some fixes in other repos all branches are available at https://github.com/ckeditor/ckeditor5/tree/%23t/ckeditor5-engine/1436: