t/1439: Firefox should visually move the caret to the new line after a soft break #1582
Conversation
tests/view/renderer.js
Outdated
| @@ -1714,6 +1715,74 @@ describe( 'Renderer', () => { | |||
| expect( domRoot.innerHTML ).to.equal( '<ul><li>Foo<ul><li><b>Bar</b><i>Baz</i></li></ul></li></ul>' ); | |||
| } ); | |||
|
|
|||
| // #1439 | |||
| it( 'should force–refresh the selection in FF after a soft break to nudge the caret (non-gecko)', () => { | |||
There was a problem hiding this comment.
Shouldn't the test name be something like "it should not force-refresh the selection after a soft break in non-gecko browsers" and the other one negated?
There was a problem hiding this comment.
Also, I am not that sure if we need such a test and if we can test it properly. I mean, the test works but checking if addRange was called is a bit dangerous. If we ever use it in the normal selection setting process the test will start failing and we will have to remove it anyway. I would be fine with manual test only, I guess.
There was a problem hiding this comment.
Agree with @scofalik on changing test names to something like it should not force-refresh the selection after a soft break in non-gecko browsers and it should force-refresh the selection after a soft break in gecko browsers.
As for not having unit test, we probably need some to have 100% cc (unless this code is already covered by some not directly related tests). Still it is a visual issue so can't be really tested reasonable with unit test. Anyway, I'm ok with leaving the test as is or just removing it.
There was a problem hiding this comment.
I renamed the tests. I'm for keeping them, well at least for the CC.
tests/view/renderer.js
Outdated
| @@ -1714,6 +1715,74 @@ describe( 'Renderer', () => { | |||
| expect( domRoot.innerHTML ).to.equal( '<ul><li>Foo<ul><li><b>Bar</b><i>Baz</i></li></ul></li></ul>' ); | |||
| } ); | |||
|
|
|||
| // #1439 | |||
| it( 'should force–refresh the selection in FF after a soft break to nudge the caret (non-gecko)', () => { | |||
There was a problem hiding this comment.
Agree with @scofalik on changing test names to something like it should not force-refresh the selection after a soft break in non-gecko browsers and it should force-refresh the selection after a soft break in gecko browsers.
As for not having unit test, we probably need some to have 100% cc (unless this code is already covered by some not directly related tests). Still it is a visual issue so can't be really tested reasonable with unit test. Anyway, I'm ok with leaving the test as is or just removing it.
src/view/renderer.js
Outdated
| // To stay on the safe side, the fix being as specific as possible, it targets only the | ||
| // selection which is at the very end of the element and preceded by <br />. | ||
| if ( childAtOffset && childAtOffset.tagName == 'BR' ) { | ||
| domSelection.addRange( domSelection.getRangeAt( 0 ) ); |
There was a problem hiding this comment.
I had a concern about domSelection.addRange if adding another range will not result in selection having two ranges (as they are the same it doesn't makes sense probably) but it seems there is one range after this call 👍
There was a problem hiding this comment.
I was also worried about that but as you said, there's only one range after this call.
Anyway, let's make sure I didn't break FF with this branch. @Mgsy, can you help us?
|
@oleq could you resolve CI and coverage issues? |
|
I think the coverage decreased because this PR requires a branch in the utils, which hadn't been merged yet at that point. |
src/view/renderer.js
Outdated
| // which happens a lot when using the soft line break, the browser fails to (visually) move the | ||
| // caret to the new line. A quick fix is as simple as force–refreshing the selection with the same range. | ||
| if ( env.isGecko ) { | ||
| const parent = focus.parent; |
There was a problem hiding this comment.
I'll move the entire body of this condition to another function because it looks bad that there's a return inside it. What if someone would like to add another such hack after this one? We'll need to refactor it anyway.
Suggested merge commit message (convention)
Fix: Firefox should visually move the caret to the new line after a soft break. Closes ckeditor/ckeditor5#4363.
Additional information
In a F2F talk with @scofalik we decided to keep browser quirks in the engine rather in features (like
ShiftEnterCommand).The entire fix is just
domSelection.addRange( domSelection.getRangeAt( 0 ) );BTW ;-)