t/1301: The bindTwoStepCaretToAttribute should not fail in more complex cases #1406

oleq · 2018-04-16T12:17:55Z

Suggested merge commit message (convention)

Fix: The bindTwoStepCaretToAttribute behavioral helper should not fail in more complex cases. Closes ckeditor/ckeditor5#4277. Closes ckeditor/ckeditor5#4305. Closes ckeditor/ckeditor5#937. Closes ckeditor/ckeditor5#922. Closes ckeditor/ckeditor5#946.

Additional information

Note: A thorough testing required before merging. @Mgsy?

…nrelated attributes inside the main attribute.

…e newest API.

…l changes.

…tribute module.

oleq · 2018-04-16T12:20:50Z

I wonder if we should have a ticket test for every single issue mentioned here 🤔Wouldn't it be an overkill?

coveralls · 2018-04-16T12:26:02Z

Coverage remained the same at 100.0% when pulling 4f6d9d9 on t/1301 into b54f26a on master.

Mgsy · 2018-04-16T12:40:18Z

Steps to reproduce

Open the article sample.
Create new link.
Put the caret at the end of the link.
Press Arrow right to leave the link.
Activate bold and type one character.
Press Arrow left few times.

Current result

The caret is stuck.

GIF

Click

It's a regression.

Edit: This scenario could be simplified. I've updated it.

Mgsy · 2018-04-17T08:20:08Z

Steps to reproduce

Open the article sample.
Put the caret at the end of the Paragraph word.
Open the link balloon, type one character, i.e. x and add it.
Put the caret at the end of the link.
Press Arrow left.

Current result

The two-step caret movement doesn't activate and the selection is outside of the link.

GIF

Click

Reinmar · 2018-04-17T10:04:03Z

@scofalik, could you review this?

scofalik · 2018-04-17T10:34:37Z

AFAIK @oleq is fighting with a nasty bug ATM.

scofalik · 2018-04-17T13:01:36Z

src/utils/bindtwostepcarettoattribute.js

+		//
+		//		<paragraph><$text attribute>{}bar</$text>baz</paragraph>
+		//
+		if ( position.isAtStart && this._hasAttribute ) {


I am not sure what exactly is this case here, but the comment makes me worried, that there's an error here.

Basically, Position#isAtStart will not return true if you are at the "beginning of an attribute". Or, more precisely, when a position is at the same offset when a Text node starts. So, in this example:

<paragraph>foo<$text linkHref="foo.html">{}link</$text>bar</paragraph>

Position#isAtStart returns false.

If you meant "at the beginning of an element", please fix the comment.

Bug in docs, OFC.

scofalik · 2018-04-17T13:04:37Z

src/utils/bindtwostepcarettoattribute.js

+		if ( isBetweenDifferentValues( position, attribute ) && this._hasAttribute ) {
+			this._preventCaretMovement( data );
+			this._removeSelectionAttribute();
+		} else {


Is this else needed here?

scofalik · 2018-04-17T13:14:09Z

src/utils/bindtwostepcarettoattribute.js

+
+		// DON'T ENGAGE 2-SCM if gravity is already overridden. It means that we just entered
+		//
+		// 		<paragraph>foo{}<$text attribute>bar</$text>baz</paragraph>


Is this comment right? Shouldn't it be {}bar?

scofalik · 2018-04-17T13:19:29Z

src/utils/bindtwostepcarettoattribute.js

+		//
+		// 		<paragraph><$text attribute="1">foo</$text>{}<$text attribute="2">bar</$text></paragraph>
+		//
+		if ( isBetweenDifferentValues( position, attribute ) && this._hasAttribute ) {


Wouldn't this condition be met in this case:

<paragraph><$text attribute="1">foo</$text><$text attribute="2">{}bar</$text>

Okay I guess that in this scenario gravity is already overridden, so it would return earlier. If you think it is worthy to left a note about this, please do so. If not, it's fine.

oleq · 2018-04-17T13:28:19Z

Open the article sample.
Put the caret at the end of the Paragraph word.
Open the link balloon, type one character, i.e. x and add it.
Put the caret at the end of the link.
Press Arrow left.

It's not a bug. Setting selection using the mouse in a boundary situation (link is first/last in the block) will always put it inside the link. So no 2-SCM then. You have to use the arrow right keystroke to activate it.

scofalik · 2018-04-17T13:44:09Z

src/utils/bindtwostepcarettoattribute.js

 	const isAttrInNext = nextNode ? nextNode.hasAttribute( attribute ) : false;
 	const isAttrInPrev = prevNode ? prevNode.hasAttribute( attribute ) : false;

-	if ( isAttrInNext && isAttrInPrev && nextNode.getAttributeKeys( attribute ) !== prevNode.getAttribute( attribute ) ) {
+	if ( ( !isAttrInPrev && isAttrInNext ) || isBetweenDifferentValues( position, attribute ) ) {


I am not sure if I like isBetweenDifferentValues here, because it repeats some code from before. Maybe

isAttrInNext && ( !isAttrInPrev || prevNode.getAttribute( attribute ) != nextNode.getAttribute( attribute ) ) ?

scofalik · 2018-04-17T13:44:32Z

src/utils/bindtwostepcarettoattribute.js

+	const isAttrInNext = nextNode ? nextNode.hasAttribute( attribute ) : false;
+	const isAttrInPrev = prevNode ? prevNode.hasAttribute( attribute ) : false;
+
+	if ( ( isAttrInPrev && !isAttrInNext ) || isBetweenDifferentValues( position, attribute ) ) {


Same as above.

scofalik · 2018-04-17T14:05:32Z

src/utils/bindtwostepcarettoattribute.js

+			//
+			if ( isBetweenDifferentValues( position, attribute ) && this._hasAttribute ) {
+				this._preventCaretMovement( data );
+				this._restoreGravity();


I wonder whether the selection should be restored in case. I guess there's a difference when this would be your model:

<paragraph><$text attribute="1" bold="true">foo</$text><$text attribute="2">{}bar</$text></paragraph>

Do we want the bold or not.

I don't say that current solution is incorrect, I just wonder.

In this case

<$text attribute="1" bold="true">foo</$text><$text attribute="2">{}bar</$text>

←: the selection has bold="true" only (2-SCM active for attribute). It's a typing between two attribute boundaries of a different value case.

←: the selection has attribute="1" bold="true"

If the selection lost bold="true" in the first ← step I'd personally find it confusing. Is that what you wanted to discuss?

I wonder if the selection should have bold after the first left-arrow press.

But this is nothing critical.

scofalik · 2018-04-17T14:10:35Z

src/utils/bindtwostepcarettoattribute.js

-			// If we are here we need to check if caret is a one character before the text with attribute bound
-			// `foo<a>bar</a>b{}iz` or `foo<a>b{}ar</a>biz`.
-			const nextPosition = position.getShiftedBy( -1 );
+			// DON'T ENGAGE 2-SCM if gravity is already overridden. It means that we have already entered


Outdated comment?

scofalik · 2018-04-17T14:11:32Z

src/utils/bindtwostepcarettoattribute.js

+				if ( this._hasAttribute ) {
+					this._removeSelectionAttribute();
+
+					return;


Unnecessary return

scofalik · 2018-04-17T14:12:18Z

src/utils/bindtwostepcarettoattribute.js

+			//
+			// 		<paragraph>{}<$text attribute>bar</$text></paragraph>
+			//
+			if ( position.isAtStart && isAtBoundary( position, attribute ) ) {


I am not sure if isAtBoundary is needed here if we check this._hasAttribute anyway.

BTW. wouldn't using isAtStartBoundary be more precise and a tiny little bit faster?

scofalik · 2018-04-17T14:17:59Z

src/utils/bindtwostepcarettoattribute.js

+	 * @private
+	 */
+	_preventCaretMovement( data ) {
+		data.preventDefault();


Did you give it a thought to also cancel the event? I wonder if canceling the event wouldn't be correct. Of course, it works now as it is, so we don't have to change it, but IDK. We kind of cancel the event by handling it in quite a custom way so IDK. Both canceling and not canceling may create issues in future.

You can verify how important this is by checking how this works next to widgets which also try to handle left/right arrow keys ;)

PS. I think we should cancel the event and make sure that 2scm handles it.

Yes, you're right. The 2SCM doesn't work around widgets like image. But to get it working 2 things are neccessary:

evt.stop()

changing 2SCM keydown listener priority to highest because the Widget plugin already uses high :/

I love the second point. Can we use priorities.high + 1? Does on() allow specifying number still? I think it does because it should be using priorities.get().

Well, that's what priorities are for!

scofalik · 2018-04-17T15:34:18Z

Review

I finished reviewing the code state as it is now. AFAIK @oleq is still fighting with some bugs but there is progress in this matter. After the bugs are fixed we could merge this PR.

This saying, in my opinion, this solution is too complicated at this point. I think there are two causes for that:

@oleq inherited this code from @oskarwrobel and there was not much time to rethink the architecture of this solution.
We had too few cases on our hands.

As much as I am for merging this PR (since it will make the whole feature more stable) I also think that we should discuss this feature once again, having the full view and discovering some cases that we didn't think about at the beginning.

Proposal

Personally, I think that we might be better dropping "override/restore" gravity as it brings a lot of complications. Dropping this solution and managing attributes directly through writer API may simplify this feature. Additionally, we will get rid of weird situations, where gravity overriding might do more than we'd like to, for example:

<$text linkHref="foo.html">foo{}</$text><$text bold="true">bar</$text>

If we override gravity to the ride, we start typing bold. Is it correct? Maybe - I am not saying it is not. But it is more mess and potential problems.

This feature seems like it shouldn't really be that difficult, though I guess that we made it too complicated by using wrong tool (gravity) to approach it.

Finally, even though we use selection gravity, we still use writer API to directly add or remove attributes from the selection anyway!

I have a feeling that there are just three things that should be taken into consideration:

Does the selection has the attribute?
Will the selection "leave the attribute"?
Will the selection "enter the attribute"?

Which leads to those three scenarios:

Has attribute + is leaving -> Remove the attribute, prevent caret move.
Doesn't have attribute + is entering -> Add the attribute, prevent caret move.
If approaching from the right side, remove the attribute when approaching the attribute (because the gravity is to the left). This, however, is tricky*.

How would it work for different cases?

Between attributes:

<$text attribute="1">fo{}o</$text><$text attribute="2">bar</$text>

-> not leaving or entering
<$text attribute="1">foo{}</$text><$text attribute="2">bar</$text>

-> has attribute + is leaving, remove attribute, prevent caret move
<$text attribute="1">foo</$text>{}<$text attribute="2">bar</$text>

-> does not have attribute + is entering, add attribute, prevent caret move
<$text attribute="1">foo</$text><$text attribute="2">{}bar</$text>

-> not leaving or entering
<$text attribute="1">foo</$text><$text attribute="2">b{}ar</$text>

<- not leaving or entering
<$text attribute="1">foo</$text><$text attribute="2">{}bar</$text>

<- has attribute + is leaving, remove attribute, prevent caret move
<$text attribute="1">foo</$text>{}<$text attribute="2">bar</$text>

<- does not have attribute + is entering, add attribute, prevent caret move
<$text attribute="1">foo{}</$text><$text attribute="2">bar</$text>

<- not leaving or entering
<$text attribute="1">fo{}o</$text><$text attribute="2">bar</$text>

Inside multiple attributes using 2SCM:
The trick is to handle each attribute separately and cancel handling if one of them was handled. So the keydown event should be stopped after 2SCM is used.

<$text a="1" b="2">fo{}o</$text>bar

-> not leaving or entering
<$text a="1" b="2">foo{}</$text>bar

-> has attribute a + is leaving, remove attribute, prevent caret move
<$text a="1" b="2">foo</$text><$text b="2">{}</$text>bar

-> has attribute b + is leaving, remove attribute, prevent caret move
<$text a="1" b="2">foo</$text>{}bar

-> not leaving or entering
<$text a="1" b="2">foo</$text>b{}ar

The beginning of an element:

<paragraph><$text a="1">f{}oo</$text></paragraph>

<- not leaving or entering
<paragraph><$text a="1">{}foo</$text></paragraph>

<- has attribute + is leaving, remove attribute, prevent caret move
<paragraph>{}<$text a="1">foo</$text></paragraph>

The end of an element:

<paragraph><$text a="1">fo{}o</$text></paragraph>

-> not leaving or entering
<paragraph><$text a="1">foo{}</$text></paragraph>

-> has attribute + is leaving, remove attribute, prevent caret move
<paragraph><$text a="1">foo</$text>{}</paragraph>

Approaching from the right side:

<paragraph><$text a="1">foo</$text>b{}ar</paragraph>

<- approaching from the right side, remove attribute a, remove attribute b
<paragraph><$text a="1" b="2">foo</$text>{}bar</paragraph>

<- does not have attribute a + is entering, add attribute, prevent caret move
<paragraph><$text a="1" b="2">foo</$text><$text a="1">{}</$text>bar</paragraph>

<- does not have attribute b + is entering, add attribute, prevent caret move
<paragraph><$text a="1" b="2">foo{}</$text>bar</paragraph>

<- not leaving or entering
<paragraph><$text a="1" b="2">fo{}o</$text>bar</paragraph>

Other questions regarding the solution

How to check "is leaving" and "is entering"?

This seems easy. We already check if the selection is at attribute boundary so we would just add checking if a proper key was pressed.

Clearing attributes set by 2SCM?

There's a question about clearing attributes set by 2SCM. However, maybe we don't need to do anything?

When the selection is moved through API (keypress, mouse click, directChange == true) then the attributes are cleared anyway. This means that we don't need to care about additional clearing.

* - However this messes up our solution when the caret approaches element from the right because the attribute would be removed first and then selection would clear that. So for that case, we would need more additional handling. I believe it is doable, though.

When the selection is moved because of changes to the model tree (changes in background, collaboration, directChange == false) we don't want those changes to affect selection anyway. The only exception would be if the selection is "removed" or the attribute is removed/changed. This should be doable too.

scofalik · 2018-04-17T15:57:32Z

Now, as I am thinking about the solution I described, I got an idea. Maybe we could implement gravity managing as an additional separate feature. It would be much simpler. Basically, if a user clicks left arrow key, gravity is set to right, and when a user clicks right arrow key, gravity is set to right.

This would cover two things:

We would not have the described problem in "approach from the right side" scenario.
We (maybe) would give users a better UX for attributes for which we don't want a 2SCM.

How would it work?

fo{}o<$text bold="true">bold</$text>bar

-> keep gravity to the left
foo{}<$text bold="true">bold</$text>bar

-> keep gravity to the left
foo<$text bold="true">b{}old</$text>bar

<- keep gravity to the right
foo<$text bold="true">{}bold</$text>bar

<- keep gravity to the right
fo{}o<$text bold="true">bold</$text>bar

The gravity would be also reset on change:range with directChange set to true.

This is just an idea, I didn't think it through and I didn't think what problems it may bring. Also what problems it may bring in conjunction with 2SCM (although it would fix one issue :)).

… caret movement.

…with the highest priority.

…called if the two-step called movement engages.

scofalik · 2018-04-18T13:01:07Z

src/utils/bindtwostepcarettoattribute.js

-		return true;
-	}
+	const { nodeBefore, nodeAfter } = position;
+	const isAttrBefore = nodeAfter ? nodeAfter.hasAttribute( attribute ) : false;


I find those names confusing now. isAttrBefore checks nodeAfter. What does isAttrBefore mean? If anything, I'd suggest isBeforeAttr.

My bad. I changed it automatically and somehow make a mistake.

scofalik · 2018-04-18T13:01:16Z

src/utils/bindtwostepcarettoattribute.js

-		return true;
-	}
+	const { nodeBefore, nodeAfter } = position;
+	const isAttrBefore = nodeAfter ? nodeAfter.hasAttribute( attribute ) : false;


scofalik · 2018-04-18T13:01:30Z

src/utils/bindtwostepcarettoattribute.js

-	const isAttrInNext = nextNode ? nextNode.hasAttribute( attribute ) : false;
-	const isAttrInPrev = prevNode ? prevNode.hasAttribute( attribute ) : false;
+	const { nodeBefore, nodeAfter } = position;
+	const isAttrBefore = nodeAfter ? nodeAfter.hasAttribute( attribute ) : false;


scofalik · 2018-04-18T13:04:58Z

src/utils/bindtwostepcarettoattribute.js

@@ -380,6 +397,8 @@ class TwoStepCaretHandler {
 			if ( position.isAtStart ) {
 				if ( this._hasSelectionAttribute ) {
 					this._removeSelectionAttribute();


Why aren't we preventing caret move in this case? 🤔

Would that matter in this case? It's a start of the block anyway and we're moving left.

Doesn't it matter in case of <paragraph>foo</paragraph><paragraph><$text a="1">{}foo</$text></paragraph>? Won't we get to the previous paragraph if we won't stop caret movement?

I tested this in manual test and for some reason it works 🤷‍♂️

Hm, you're right. It should be prevented. But the funny thing is that it works. As long as the keydown event is stopped.

scofalik · 2018-04-18T14:15:00Z

Things I found in manual test (http://localhost:8125/ckeditor5-engine/tests/manual/two-step-caret.html):

[]biz after first right-arrow keypress becomes []biz and then additional keypress does nothing.
Minor thing, in one scenario a button on toolbar flickers:
This scenario from manual test fails, however I don't know how important it is, really:

Attributes set explicit
Press right arrow once again
selection should be at the same position
underline button should be not selected
(*) bold button should stay selected

I don't like that there's no intermediate step between barbiz.
Filler bug is back:
You were supposed to merge http://localhost:8125/ckeditor5-engine/tests/manual/tickets/1301/1.html with http://localhost:8125/ckeditor5-engine/tests/manual/two-step-caret.html
I miss an image in http://localhost:8125/ckeditor5-engine/tests/manual/two-step-caret.html test to see how 2SCM works with widgets.

… the boundary of an attribute should engage the two-step caret movement.

oleq · 2018-04-18T14:45:39Z

[]biz after first right-arrow keypress becomes []biz and then additional keypress does nothing.

I'm unable to reproduce it.

Minor thing, in one scenario a button on toolbar flickers:

It's because the engine sets attributes and we remove them :/

This scenario from manual test fails, however I don't know how important it is, really:

We sacrificed this when choosing _refreshAttributes() over _updateAttributes in LiveSelection.

I don't like that there's no intermediate step between barbiz.

Me too. But I'm afraid there's no easy way to do that. Different bindTwoStepCaretToAttribute "instances" would need to talk to each other to do that. ATM when we leave (arrow right) bar[]biz we prevent the movement and override gravity (u attribute handler does that). To create that additional "empty" step we'd need to remove the selection attribute (i) too at the same time. And it's up to the i attribute handler. It would need to know what happened in the u handler to approach the situation properly.

…aret movement.

Reinmar · 2018-04-18T15:28:19Z

Reinmar · 2018-04-18T15:33:58Z

OK, the above is not caused by 2scm.

…tribute-preceded-by-some-text-at-the-end-of-the-block-case.

oleq added 9 commits April 16, 2018 13:55

Tests: Refactoring in bindTwoStepCaretToAttribute tests.

6d7a74d

Fix: The two step caret movement should not engage when approaching u…

c301696

…nrelated attributes inside the main attribute.

Code refactoring.

e460049

Tests/Docs: Updated Writer and DocumentSelection docs and tests to th…

c12b045

…e newest API.

Tests: Added a test to verify 2-SCM acts properly when facing externa…

ebb407f

…l changes.

Tests: Added tests to verify 2-SCM with multiple attributes.

86e3e34

Docs: Extended documentation in the engine/utils/bindtwostepcarettoat…

16a7220

…tribute module.

Tests: Added an additional test case.

447750f

Tests: Added an additional tests case.

88caaad

oleq requested review from scofalik and Mgsy April 16, 2018 12:20

Reinmar requested review from scofalik and removed request for scofalik April 17, 2018 10:06

scofalik reviewed Apr 17, 2018

View reviewed changes

scofalik added the status:review- label Apr 17, 2018

oleq added 9 commits April 17, 2018 18:32

Fix: The filler logic in the renderer should not interfere the 2-step…

e1c02a0

… caret movement.

Docs and code refactoring.

991a96f

Code refactoring.

763f32a

Code refactoring.

bff08fb

Docs: Fixed outdated docs in the bindTwoStepCaretToAttribute helper.

5be31ef

Code refactoring in the bindTwoStepCaretToAttribute helper.

411f952

Code refactoring in the bindTwoStepCaretToAttribute helper.

0c1d47b

Fix: The bindTwoStepCaretToAttribute helper should listen to keydown …

dc66333

…with the highest priority.

Fix: Any further view.document#keydown event listeners should not be …

4f6d9d9

…called if the two-step called movement engages.

scofalik reviewed Apr 18, 2018

View reviewed changes

oleq added 2 commits April 18, 2018 15:31

Merge branch 'master' into t/1301

649f76d

Code refactoring: Fixed confusing variable names in helpers.

24f8e2f

Fix: Pressing arrow left/right at the start/end of the block which is…

e89e69d

… the boundary of an attribute should engage the two-step caret movement.

oleq added 2 commits April 18, 2018 17:13

Fix: Jumping over the filler should not be hindered by the two-step c…

d32625f

…aret movement.

Docs: Fixed typos in TwoStepCaretHandler docs.

8f964bd

Fix: The two-step caret movement could handle the single-character-at…

446edb9

…tribute-preceded-by-some-text-at-the-end-of-the-block-case.

scofalik merged commit f0fd2d8 into master Apr 18, 2018

scofalik deleted the t/1301 branch April 18, 2018 16:06

scofalik mentioned this pull request Oct 9, 2019

Rethink two-step caret movement ckeditor/ckeditor5#4341

Closed

t/1301: The bindTwoStepCaretToAttribute should not fail in more complex cases #1406

t/1301: The bindTwoStepCaretToAttribute should not fail in more complex cases #1406

Conversation

oleq commented Apr 16, 2018 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

oleq commented Apr 16, 2018

coveralls commented Apr 16, 2018 • edited Loading

Mgsy commented Apr 16, 2018 • edited Loading

Steps to reproduce

Current result

GIF

Mgsy commented Apr 17, 2018

Steps to reproduce

Current result

GIF

Reinmar commented Apr 17, 2018

scofalik commented Apr 17, 2018

scofalik Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleq commented Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik commented Apr 17, 2018 • edited Loading

Review

Proposal

How would it work for different cases?

Other questions regarding the solution

scofalik commented Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik commented Apr 18, 2018

oleq commented Apr 18, 2018 • edited Loading

Reinmar commented Apr 18, 2018

Reinmar commented Apr 18, 2018

oleq commented Apr 16, 2018 •

edited by pomek

Loading

coveralls commented Apr 16, 2018 •

edited

Loading

Mgsy commented Apr 16, 2018 •

edited

Loading

scofalik Apr 17, 2018 •

edited

Loading

oleq commented Apr 17, 2018 •

edited

Loading

scofalik Apr 17, 2018 •

edited

Loading

scofalik commented Apr 17, 2018 •

edited

Loading

scofalik commented Apr 17, 2018 •

edited

Loading

oleq commented Apr 18, 2018 •

edited

Loading