Custom keyboard navigation through tables #304

niegowski · 2020-04-14T19:51:56Z

Suggested merge commit message (convention)

Other: Custom keyboard navigation through tables. Closes ckeditor/ckeditor5#3267. Closes ckeditor/ckeditor5#3286.

Additional information

# Conflicts: # src/tableediting.js

…fixes.

…e in a table cell.

…ed handling of widgets at beginning/end of table cell. Code cleanup and added inline comments.

coveralls · 2020-04-14T19:57:02Z

Coverage remained the same at 100.0% when pulling 92d1567 on i/3267 into 15016bf on master.

oleq · 2020-04-15T10:46:43Z

I think the fact that I can navigate across the lines downwards using the down arrow key but using the arrow up key moves the caret to the first position in the cell is a bug.

oleq · 2020-04-15T11:59:10Z

☝️ I see that it works for multiple blocks, one after another, but in the screencast its a single block with wrapped content.

oleq · 2020-04-15T12:00:44Z

~~In a case where the widget is the only child of the cell, there's a lockdown. Arrow keys won't let me navigate to neighboring cells.~~

~~Same, if it's the first/last child (up/down arrows).~~

My bad, I switched to another branch in the meanwhile.

oleq

The feature looks good to me. I only had some small concerns regarding implementation. Let's move ahead with this feature.

src/tablenavigation.js

oleq · 2020-04-15T13:29:31Z

This one also looks like a bug (arrow down should always navigate to the image and the cell below):

…handle hidden image captions.

niegowski · 2020-04-15T18:34:38Z

This one also looks like a bug (arrow down should always navigate to the image and the cell below):

This should be fixed now in eebd6f2.

niegowski · 2020-04-16T13:12:50Z

cc @ckeditor/qa-team Can you have a look at this? Navigation over table cells should be as intuitive as possible. Note that on the first/last text lines in a cell the caret is moved to first/last position in that cell (it's expected behavior). It's using DOM client rects so we should test it in different browsers.

…d from outside.

Mgsy · 2020-04-17T06:59:19Z

Firefox only

If a cell contains a blockquote, pressing Arrow down makes the selection jumps to the end of the blockquote, instead of a line below.

Mgsy · 2020-04-17T08:26:14Z

We've tested it in all browsers and besides the issue above, it works fine 👍

…aragraph when next paragraph is single-line.

…ests.

niegowski · 2020-04-20T17:43:40Z

Firefox only

If a cell contains a blockquote, pressing Arrow down makes the selection jumps to the end of the blockquote, instead of a line below.

This is how Firefox handles overflow: hidden for blockquote. It works that way even without tables. We use overflow: hidden there to trigger "Block formatting context" for blockquote to handle positioning next to floating widgets.

oleq

Some tests are failing on the CI.

oleq

Mostly concerns regarding docs.

src/table.js

src/tablenavigation.js

oleq · 2020-04-21T15:24:51Z

src/tablenavigation.js

+	}
+
+	/**
+	 * Returns a range from beginning/end of range up to selection closest position.


This probably needs rephrasing ("a range from the beginning of range") because honestly I don't understand what it does without digging into the code.

src/tablenavigation.js

oleq · 2020-04-21T15:30:03Z

src/tablenavigation.js

+	}
+
+	/**
+	 * Checks if the `modelRange` renders to a single line in the DOM.


That's probably one of the key methods around here. It deserves more documentation, especially that model ranges do not work at the DOM level. You should explain that you're checking out a DOM range corresponding to the model range and then you analyze its DOMRects to figure out whether the DOM range visually wraps to the next line.

Co-Authored-By: Aleksander Nowodzinski <a.nowodzinski@cksource.com>

oleq

Minor remarks concerning tests.

tests/tablenavigation.js

oleq · 2020-04-22T11:21:38Z

tests/tablenavigation.js

@@ -0,0 +1,2277 @@
+import { getCode } from '@ckeditor/ckeditor5-utils/src/keyboard';
+import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';


We try to follow a convention in which named imports usually land after default imports.

Also, we try (at least to some extent) to group imports (external packages in one group (image, horizontal line), in-package imports (table, table editing) in another, global helpers (global, env) in another).

This should probably be explained in https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/framework/guides/contributing/code-style.html (cc @Reinmar).

tests/tablenavigation.js

oleq · 2020-04-22T11:51:59Z

tests/tablenavigation.js

+					} );
+				} );
+
+				describe( 'contains image widget with caption and selection inside the caption', () => {


What contains that image? TCs suggest this is about a cell where the initial selection is but it is not clear from the description. It could as well describe a place the selection should be anchored to after the navigation.

Table cell contains image.

jodator

OK I'll stop here.

The feature works great and for me the issues which I'll report below are minor and can be follow ups.

As for the code:

I really don't like the ck-hidden check in the view. For now I think that we could live with this but I'd rather see if we cannot detect this case differently. What if we change view class? And that kind of question - I feel that this might be a bigger issue as how to reflect such things in the model.
Some if clause removals - just to have them nicer to read.
I've went through the _handleArrowKeys() method - my feeling is that some checks were an outcome of a long-living code that evolved with each edge case. I found some improvements and simplifications - those might be minor things really. I only wonder if the checks shouldn't be revisited with a "fresh" eye to spot other possible cases to handle differently - especially to remove the dependency on a view (in the ck-hidden case - the calculating DOM Rects is a pretty neat idea and we don't have any other option.
Don't import from model in features. We have API for that™.
The _isSingleLineRange needs better comments as current one were hard to dig into. However I don't have an idea how to improve them now.
I didn't read the tests so I hope @oleq found something there :D

src/table.js

jodator · 2020-04-22T07:02:22Z

tests/table.js

 	it( 'requires TableEditing, TableUI, TableSelection, TableClipboard, and Widget', () => {
-		expect( Table.requires ).to.deep.equal( [ TableEditing, TableUI, TableSelection, TableClipboard, Widget ] );
+		expect( Table.requires ).to.deep.equal( [ TableEditing, TableUI, TableSelection, TableClipboard, TableNavigation, Widget ] );
 	} );


Test name is wrong ;)

If you wish you could split it to 6 separate tests (each for required plugin). But changing test name is enough.

src/tablenavigation.js

jodator · 2020-04-22T13:57:27Z

src/tablenavigation.js

+		case keyCodes.arrowleft: return isLtrContent ? 'left' : 'right';
+		case keyCodes.arrowright: return isLtrContent ? 'right' : 'left';
+		case keyCodes.arrowup: return 'up';
+		case keyCodes.arrowdown: return 'down';


Please format it so each return is in new line (code style consistency with existing code).

jodator · 2020-04-22T14:40:12Z

And I have and inconsistency in the behaviour. Basically the feature works like the Google docs but - I don't find it OK but maybe it's just me.

So Google docs navigating with arrow (gets "trapped"):

So Google docs navigating with TAB (as in CKEditor - navigate on the same row):

Browsers (Chrome & Firefox) with arrows (similar to TAB action):

Similarly to browser, the Libre office follows the TAB behaviour pattern for arrows:

So as long we think that having different behaviour for TAB is OK (so as in GDocs) then it is a minor issue.

cc @oleq .

Co-Authored-By: Maciej <jodator@jodator.net>

jodator

LGTM now 👍.

The issues were addressed and the navigation itself works great.

CC @Reinmar I'd go with this in current iteration. We can also close handling shift ckeditor/ckeditor5#6641.

jodator · 2020-04-23T09:48:31Z

The test that fails is from Firefox. I don't see what is causing this in this PR so I'm merging it and try fix CI later.

niegowski added 8 commits April 8, 2020 22:23

Added custom keyboard navigation through tables.

b3c1a0d

Merge branch 'master' into i/3267

99b1207

# Conflicts: # src/tableediting.js

Added some code comments and optimisation of up/down at cell start/end.

e5ec2f0

Custom table navigation extracted to the separate plugin. Navigation …

7fb91e0

…fixes.

Updated tab key tests (moved to TableNavigation plugin).

cfd0110

Optimised algorithm for finding if selection is in the first/last lin…

9373f5c

…e in a table cell.

Table navigation using 'keydown' event's instead of keystrokes. Add…

379e3f7

…ed handling of widgets at beginning/end of table cell. Code cleanup and added inline comments.

Using modifySelection to find navigable selection positions.

8116068

niegowski requested a review from oleq April 14, 2020 19:53

Merge branch 'master' into i/3267

4b62a7e

oleq suggested changes Apr 15, 2020

View reviewed changes

src/tablenavigation.js Outdated Show resolved Hide resolved

src/tablenavigation.js Outdated Show resolved Hide resolved

src/tablenavigation.js Show resolved Hide resolved

src/tablenavigation.js Show resolved Hide resolved

src/tablenavigation.js Show resolved Hide resolved

Fixed isSingleLineRange (missing rect values rounding).

c508001

niegowski added 2 commits April 15, 2020 17:14

Simplified isSingleLineRange implementation.

9c6a8af

Changed _getNearestTextPosition to _getNearestVisibleTextPosition to …

eebd6f2

…handle hidden image captions.

jodator mentioned this pull request Apr 16, 2020

Editor crashes when pressing tab in a table cell with selection on an image ckeditor/ckeditor5#6607

Closed

Added tests for navigation through table cells when those are selecte…

05368fe

…d from outside.

niegowski added 5 commits April 17, 2020 19:32

Added more tests for custom keyboard navigation through tables.

26d7453

Updated tests for custom keyboard navigation through tables.

29dca14

Fixed edge case of the selection at the position next to the end of p…

b1f6706

…aragraph when next paragraph is single-line.

Added handling of navigation next/on object elements and additional t…

d4f1972

…ests.

Added tests for "dual" positions in text.

8c35a7e

Merge branch 'master' into i/3267

20b41a6

oleq suggested changes Apr 21, 2020

View reviewed changes

niegowski added 3 commits April 21, 2020 10:44

Tweaking visual test.

973170a

Tweaking visual test.

3795e37

Disabled unstable test on Gecko.

af89be5

oleq mentioned this pull request Apr 21, 2020

Table navigation using keyboard should not alter the native Shift+Arrow behavior ckeditor/ckeditor5#6641

Closed

oleq suggested changes Apr 21, 2020

View reviewed changes

niegowski and others added 3 commits April 22, 2020 11:12

Updated JSDocs.

85b4dea

Apply suggestions from code review

c9b7e5f

Co-Authored-By: Aleksander Nowodzinski <a.nowodzinski@cksource.com>

Updated JSDocs.

6dd7b32

niegowski requested a review from oleq April 22, 2020 09:25

oleq suggested changes Apr 22, 2020

View reviewed changes

niegowski and others added 3 commits April 22, 2020 13:29

Added missing license header.

c0497e6

Organized imports.

1879a5c

Tests: Improved test case naming in TableNavigation tests.

83d09d6

oleq reviewed Apr 22, 2020

View reviewed changes

tests/tablenavigation.js Show resolved Hide resolved

oleq reviewed Apr 22, 2020

View reviewed changes

jodator suggested changes Apr 22, 2020

View reviewed changes

niegowski and others added 4 commits April 22, 2020 16:57

Apply suggestions from code review

e3fe69f

Co-Authored-By: Maciej <jodator@jodator.net>

Apply suggestions from code review.

5287e15

Apply suggestions from code review.

344e515

Update commend wording.

9760311

jodator approved these changes Apr 23, 2020

View reviewed changes

Merge branch 'master' into i/3267

92d1567

jodator merged commit d480c6d into master Apr 23, 2020

jodator deleted the i/3267 branch April 23, 2020 09:50

jodator mentioned this pull request Apr 24, 2020

Table keyboard navigation - wrapping to next row invalid for row-spanned cells ckeditor/ckeditor5#6668

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom keyboard navigation through tables #304

Custom keyboard navigation through tables #304

niegowski commented Apr 14, 2020 •

edited

Loading

coveralls commented Apr 14, 2020 •

edited

Loading

oleq commented Apr 15, 2020

oleq commented Apr 15, 2020

oleq commented Apr 15, 2020 •

edited

Loading

oleq left a comment

oleq commented Apr 15, 2020

niegowski commented Apr 15, 2020

niegowski commented Apr 16, 2020

Mgsy commented Apr 17, 2020 •

edited

Loading

Mgsy commented Apr 17, 2020

niegowski commented Apr 20, 2020

Firefox only

oleq left a comment

oleq left a comment

oleq Apr 21, 2020

oleq Apr 21, 2020

oleq left a comment

oleq Apr 22, 2020 •

edited

Loading

oleq Apr 22, 2020

niegowski Apr 22, 2020

jodator left a comment

jodator Apr 22, 2020

jodator Apr 22, 2020

jodator commented Apr 22, 2020

jodator left a comment

jodator commented Apr 23, 2020

		@@ -0,0 +1,2277 @@
		import { getCode } from '@ckeditor/ckeditor5-utils/src/keyboard';
		import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';

Custom keyboard navigation through tables #304

Custom keyboard navigation through tables #304

Conversation

niegowski commented Apr 14, 2020 • edited Loading

Suggested merge commit message (convention)

Additional information

coveralls commented Apr 14, 2020 • edited Loading

oleq commented Apr 15, 2020

oleq commented Apr 15, 2020

oleq commented Apr 15, 2020 • edited Loading

oleq left a comment

Choose a reason for hiding this comment

oleq commented Apr 15, 2020

niegowski commented Apr 15, 2020

niegowski commented Apr 16, 2020

Mgsy commented Apr 17, 2020 • edited Loading

Firefox only

Mgsy commented Apr 17, 2020

niegowski commented Apr 20, 2020

Firefox only

oleq left a comment

Choose a reason for hiding this comment

oleq left a comment

Choose a reason for hiding this comment

oleq Apr 21, 2020

Choose a reason for hiding this comment

oleq Apr 21, 2020

Choose a reason for hiding this comment

oleq left a comment

Choose a reason for hiding this comment

oleq Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

oleq Apr 22, 2020

Choose a reason for hiding this comment

niegowski Apr 22, 2020

Choose a reason for hiding this comment

jodator left a comment

Choose a reason for hiding this comment

jodator Apr 22, 2020

Choose a reason for hiding this comment

jodator Apr 22, 2020

Choose a reason for hiding this comment

jodator commented Apr 22, 2020

jodator left a comment

Choose a reason for hiding this comment

jodator commented Apr 23, 2020

niegowski commented Apr 14, 2020 •

edited

Loading

coveralls commented Apr 14, 2020 •

edited

Loading

oleq commented Apr 15, 2020 •

edited

Loading

Mgsy commented Apr 17, 2020 •

edited

Loading

oleq Apr 22, 2020 •

edited

Loading