Fixed autocomplete view positioning #1732

jacekbogdanski · 2018-03-02T16:46:55Z

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

View positioning was based on window pane size and scroll position. Now it depends on absolute rectangle of the editor frame.

mlewand

Two things needs to be addressed before going into review:

https://github.com/ckeditor/ckeditor-dev/blob/13d35dcb74ba0412d2ae2cb2c08f66e3a06ff196/tests/plugins/autocomplete/view.js#L222 - Wouldn't a better name for it be caretRect rather than just rect?
https://github.com/ckeditor/ckeditor-dev/blob/13d35dcb74ba0412d2ae2cb2c08f66e3a06ff196/tests/plugins/autocomplete/view.js#L216 - this is absolute rectangle of what? The purpose of that option should be clear. It's hard to understand it otherwise.

mlewand · 2018-03-09T11:33:35Z

tests/plugins/autocomplete/manual/viewposition.md

+## Unexpected
+
+The view is not changing its position depending on space below and above a caret.
+


Please put just a single line ending here.

jacekbogdanski · 2018-03-09T13:22:27Z

I refactored some missing semicolons etc from test file (looks like my linter wasn't working). There may be changes not related to this issue but I think it's good idea to just fix it now so we don't forgot about it in other PRs.

mlewand

Generally looks OK, I'm missing divarea and inline editor instances in the manual test.

https://github.com/ckeditor/ckeditor-dev/blob/3e782b9/tests/plugins/autocomplete/view.js#L226 (see Fixed autocomplete view positioning #1732 (review))

My point about that line has not been addressed.

https://github.com/ckeditor/ckeditor-dev/blob/3e782b9/tests/plugins/autocomplete/view.js#L234

This is not a good idea:
- You do cleanup after assertion. Cleaning up is desired, but before the assertion. If your assertion fails it throws an exception, meaning no cleanup occurs.
- You increase a complexity while the code is synchronous.
After adding divarea plugin you'll see that it does not react to scroll. This should also be fixed as a part of this PR.

mlewand · 2018-03-20T15:34:29Z

plugins/autocomplete/plugin.js

-				editable = this.editor.editable(),
+			var editor = this.editor,
+				viewPanelHeight = this.element.getSize( 'height' ),
+				windowFrame = editor.window.getFrame(),


This variable is used only once. I think it can be safely inlined, without sacrificing the readability.

mlewand · 2018-03-22T10:56:26Z

plugins/autocomplete/plugin.js

+				editorViewportRect = editable.isInline() ? editable.getAbsoluteClientRect( editor ) : windowFrame.getAbsoluteClientRect( editor ),
+				// How much space is there for the panel above and below the specified rect.
+				spaceAbove = rect.top - editorViewportRect.top,
+				spaceBelow = rect.bottom - editorViewportRect.bottom,


Shouldn't the space below be computed by editorViewportRect.bottom - rect.bottom? Surprisingly I don't see any visible issues related to this value being computed incorrectly.

jacekbogdanski · 2018-03-26T10:11:31Z

About not addressed line here: https://github.com/ckeditor/ckeditor-dev/blob/3e782b9/tests/plugins/autocomplete/view.js#L226 (see #1732 (review))

I'm not sure what more could I change here? I already changed config.absoluteRect into config.editorViewportRect and renamed elementStub into getClientRectStub. Do you see any improvements which can I apply here?

mlewand · 2018-04-04T09:08:42Z

There's a bit of mess in commit history. We need to wait till #1727 lands in the major.

jacekbogdanski · 2018-04-18T10:48:37Z

I rebased it into the latest t/1703 so it contains #1727 changes.

jacekbogdanski · 2018-04-18T11:52:30Z

I changed the manual test for inline editor - it has long content but I wanted to make sure that the editor is at the end of the browser viewport so it could be property tested.

mlewand

I like the fact that with the new code base it became simpler (I mean the setPosition method).

Actually, regarding positioning I start to think that we should simply hide the autocomplite if it's outside of the viewable area (classic/divarea editor). But that should be extracted to a follow up ticket, for now it's good the way it is.

I found still three failing cases in the manual test. First one (mobile) is important and should be fixed, two others can be extracted to a separate issue.

Classic editor does not reposition autocomplete on iOS

Open manual test on Firefox/Edge.
Focus the classic editor.
Start typing @ to begin autocomplete.
Scroll the editor editable.

(Current) Expected

The autocomplete moves along.

Actual

Autocomplete remains statically.

Firefox/MSEdge edge case

Open manual test on Firefox/Edge.
Focus the classic editor.
Press ctrl + end.
Insert @ to begin autocomplete.

(Current) Expected

The autocomplete should appear within the editable.

Actual

It appears below the editable. It's enough to scroll down a bit again to make it appear correctly inside.

Additional notes

I'm saying currently expected because I think we should change/unify this behavior. But that should be done within a dedicated issue.

IE9 inline editor position fail

Open manual test on Firefox/Edge.
http://tests.ckeditor.test:1030/tests/plugins/autocomplete/manual/viewposition / Inline editor
Focus (last) inline editor.
Put the cared at the end of text and type @

Expected

View at the end of the text should be placed above a cursor.

(Works correctly on IE10, IE11, Edge)

Other things:

Manual test: Rather than putting wall of text in manual tests, you could just add a dummy paragraph/element with a large height.

Both tests: tests are not ignored on unsupported env (ie8).

mlewand · 2018-04-19T15:34:52Z

tests/plugins/autocomplete/manual/viewposition.html

+<style>
+	/* Swap margin with padding to prevent moving autocomplete panel outside editor. */
+	body {
+		margin-left: 0px;


This hack should not be needed for a proper positioning. 🙂

That's true. I will try to adjust caret position relative to the document body. I think it's an edge case where a user adds margin into body element but it could be probably fixed without huge work.

mlewand · 2018-04-19T15:35:15Z

tests/plugins/autocomplete/manual/viewposition.html

+initEditor( 'inline', 'editor3' );
+
+function initEditor( creator, name, extraPlugins ) {
+	var editor = CKEDITOR[ creator ]( name, {


This editor variable is not used anywhere.

mlewand · 2018-04-19T15:35:39Z

tests/plugins/autocomplete/manual/viewposition.html

+
+<script>
+
+initEditor( 'replace', 'editor1' );


Inconsistent indentation, see getTextTestCallback function.

By extracting function call to a function I don't think you saved too much here…

You'd be better with extracting common config, and making a regular CKEDITOR.replace / CKEDITOR.inline calls. For an example, check here: https://github.com/ckeditor/ckeditor-dev/blob/8182c89c2cdf55872c73ba0c834f516eb35335ca/tests/plugins/balloontoolbar/manual/focus.html#L59-L71

mlewand · 2018-04-19T15:46:00Z

tests/plugins/autocomplete/manual/viewposition.html

+
+<h2>Classic editor</h2>
+<div id="editor1" >
+	<p>Lorem ipsum dolor sit amet, consectetur adipisicing elit. Neque animi, iusto laudantium nobis itaque optio vitae asperiores, placeat magnam quo distinctio. Eaque fugiat quia maxime modi, quibusdam nostrum repellendus in?</p>


I changed the manual test for inline editor - it has long content but I wanted to make sure that the editor is at the end of the browser viewport so it could be property tested.

I see your reason, but you could achieve that in a simpler way. You could just put paragraphs that have a fixed height, say more that 1 times the height of the editable. I'll send a commit showing that in a minute.

jacekbogdanski · 2018-04-20T12:02:58Z

After some time fighting with margins on host body I gave up because I couldn't find an easy way to fix it. I tried to remove preset body position from caret rect but it wasn't working well with different cases. I'm not sure if this is an important issue so I left margin hack from #1732 (comment) unchanged.

I don't see much use cases where someone would like to change body margins. I think this issue exists for balloonpanel also. WDYT @mlewand? Can we live with it or should I spend more time on fixing this issue?

mlewand

I don't see much use cases where someone would like to change body margins.

I disagree, it's rare but it could definitely happen in the wild. Given the fact that it already occured is a pretty good tell.

I think this issue exists for balloonpanel also.

Does it? I think it was patched in #1201. Please check it and if it is still buggy then please report a bug.

I actually remembered that there was a similar issue in balloon panel however so I have checked it up how guys solved it. I saw the fix there but it didn't convincing to me (wasn't that clean) I recalled there's a offsetParent available and that's something we could use here. Hence the patch in 5ab276b.

I made a dirty commit, but that's something that should set you up. Please use it as a base for your work, ensure that it works correctly and cover this case with unit tests.

I also had some issues with TC names.

mlewand · 2018-04-20T13:07:04Z

tests/plugins/autocomplete/view.js

@@ -146,67 +161,52 @@
 		},

 		'test set position above inside absolute rect': function() {


I'm not a fan of this naming. Since we touched it in this PR, how about adjusting it to something clearer.

I have concerns regarding test case names. I'll illustrate what I see as an ASCII art:

+---------------------------------------------+ | | | editor viewport | | +--------------+ | | | | | | | view | | | | | | | +--------------+ | | █ | | | | | +---------------------------------------------+

The "above inside absolute rect" does not sound helpful.

"test position not enough space between the caret and bottom viewport"

Let's use ASCII art for TCs like that to make understanding it easier.

mlewand · 2018-04-20T13:10:58Z

tests/plugins/autocomplete/view.js

 			} );
+
+			assert.areEqual( '300px', view.element.getStyle( 'top' ) );
+			assert.areEqual( '100px', view.element.getStyle( 'left' ) );
 		},

 		'test set position below inside absolute rect': function() {


Name "test set position below inside absolute rect" in this TC is actually more accurate than the previous one.

+---------------------------------------------+ | | | editor viewport | | | | █ | | +--------------+ | | | | | | | view | | | | | | | +--------------+ | | | | | +---------------------------------------------+

I would go with something like: "test enough space under and above the caret"

And change the assertion:

assert.areEqual( '110px', view.element.getStyle( 'top' ) );

to

assert.areEqual( '110px', view.element.getStyle( 'top' ), 'View is displayed below the caret' );

mlewand

There's a bug in a edge case related to position. Also few style changes are needed in tests.

mlewand · 2018-04-24T11:20:39Z

tests/plugins/autocomplete/view.js

 			} );
+
+			assert.areEqual( '110px', view.element.getStyle( 'top' ), 'View is displayed below the caret' );
+			assert.areEqual( '50px', view.element.getStyle( 'left' ) );
 		},

 		'test set position above outside absolute rect': function() {


You have fixed the "absolute rect" thing in two previous tests, but left it here unchanged. For this and next TC "absolute rect" has again no meaning (or it actually misleads the reader).

I have fixed one name, but since I see that there are other issues that will require another review bounce, please fix the other name.

Also add ASCII illustration to positioning tests that do not have it. It's really helpful for others (if done right).

mlewand · 2018-04-24T11:49:54Z

tests/plugins/autocomplete/view.js

 			} );
+
+			assert.areEqual( '300px', view.element.getStyle( 'top' ) );


This is not the expected result in case when selection is just below the bottom edge of viewport.

Please also add a case where the caret is 1px above the bottom viewport edge.

Also add a TC where the caret starts at 0, and where the caret is above the viewport.

mlewand · 2018-04-24T12:12:17Z

I have pushed a single commit with some changes that I have already been changing while reviewing.

jacekbogdanski · 2018-04-25T09:00:31Z

I moved view positioning tests into a separate file so it could be easily tested with different editors. Also, I simplified setPosition function so it doesn't contain multiple ifs. Instead, I'm recalculating view position for different cases so it's easier to read but adds additional performance impact (position is calculated more times). I think it's not worth to preoptimize this code and add complicated if-else conditions but would like to know your opinion @mlewand.

My changes also closes #1913 and #1912.

… autocomplete position.

jacekbogdanski · 2018-04-25T09:04:29Z

Rebased into the latest t/1703

mlewand

LGTM

mlewand · 2018-04-25T10:01:52Z

The only missing part there was ignoring tests on unsupported IE.

jacekbogdanski force-pushed the t/1703-3559 branch from 4ac477d to 13d35dc Compare March 2, 2018 17:10

mlewand self-requested a review March 9, 2018 11:01

mlewand suggested changes Mar 9, 2018

View reviewed changes

jacekbogdanski requested a review from mlewand March 9, 2018 13:17

jacekbogdanski force-pushed the t/1703-3559 branch from f0690b7 to 24ddab1 Compare March 13, 2018 10:41

mlewand force-pushed the t/1703-3559 branch from 24ddab1 to 3e782b9 Compare March 20, 2018 14:33

mlewand suggested changes Mar 22, 2018

View reviewed changes

jacekbogdanski force-pushed the t/1703-3559 branch from 3e782b9 to 9a579a3 Compare March 25, 2018 12:59

jacekbogdanski requested a review from mlewand March 26, 2018 10:11

jacekbogdanski force-pushed the t/1703 branch from bb2c852 to e69020f Compare April 18, 2018 09:43

jacekbogdanski force-pushed the t/1703-3559 branch from 2cba1c8 to 36e3426 Compare April 18, 2018 10:45

mlewand suggested changes Apr 19, 2018

View reviewed changes

jacekbogdanski requested a review from mlewand April 20, 2018 12:04

jacekbogdanski mentioned this pull request Apr 20, 2018

Hide autocomplete view if it's outside of the viewable area #1911

Open

mlewand suggested changes Apr 20, 2018

View reviewed changes

jacekbogdanski requested a review from mlewand April 24, 2018 07:44

mlewand suggested changes Apr 24, 2018

View reviewed changes

jacekbogdanski force-pushed the t/1703 branch from aa0b979 to 28a57f6 Compare April 24, 2018 12:12

jacekbogdanski requested a review from mlewand April 25, 2018 09:00

jacekbogdanski added 3 commits April 25, 2018 11:02

Fixed autocomplete view positioning.

b820de8

Tests fix and code refactor.

2e8eb87

Refactored tests.

0622ea5

Jacek Bogdański and others added 21 commits April 25, 2018 11:02

Fixed spaceBelow, small refactoring.

85eb4e3

Refactored tests.

3171aac

Added inline and divarea manual tests.

5b15b97

Small manual test change.

42b8e26

Fixed divarea positioning.

4b25016

Small test change.

568a19d

Tests: simplified fixtures markup.

20d0582

Refactored tests.

d1490f3

Ignored tests for ie8.

94c34c7

Removed fixed an issue where margin in parents would result in broken…

da81276

… autocomplete position.

Fixed view positioning relative to the host element.

ed25deb

Refactored tests.

c547e09

Fixed getCaretRect tests.

0f66871

Added unit tests for view.getCaretRect.

1542c0e

Typo fix.

efb080d

Fixed tests for IE9.

81d14ab

Tests: autocomplete positioning changed TC name, and adjusted comment.

a25c806

Fixed view positioning for bottom edge.

1b889ba

Fixed edge positioning for caret height.

479e4e9

Added inline tests.

a0bfb19

Moved view positioning tests into separate file.

db23436

jacekbogdanski force-pushed the t/1703-3559 branch from 25fcb96 to db23436 Compare April 25, 2018 09:03

Tests: ignore autocomplete positioning tests on IE8.

92e85c9

mlewand approved these changes Apr 25, 2018

View reviewed changes

mlewand merged commit dc9fc70 into t/1703 Apr 25, 2018

mlewand deleted the t/1703-3559 branch May 9, 2018 15:22

This was referenced May 9, 2018

Invalid autocomplete view position on Firefox/MSEdge #1912

Closed

Invalid autocomplete view position on IE9 (inline editor) #1913

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed autocomplete view positioning #1732

Fixed autocomplete view positioning #1732

jacekbogdanski commented Mar 2, 2018

mlewand left a comment

mlewand Mar 9, 2018

jacekbogdanski commented Mar 9, 2018 •

edited

Loading

mlewand left a comment

mlewand Mar 20, 2018

mlewand Mar 22, 2018

jacekbogdanski commented Mar 26, 2018

mlewand commented Apr 4, 2018

jacekbogdanski commented Apr 18, 2018

jacekbogdanski commented Apr 18, 2018

mlewand left a comment

mlewand Apr 19, 2018

jacekbogdanski Apr 20, 2018

mlewand Apr 19, 2018

mlewand Apr 19, 2018

mlewand Apr 19, 2018

mlewand Apr 19, 2018

jacekbogdanski commented Apr 20, 2018 •

edited

Loading

mlewand left a comment

mlewand Apr 20, 2018

mlewand Apr 20, 2018

mlewand Apr 20, 2018

mlewand left a comment

mlewand Apr 24, 2018

mlewand Apr 24, 2018

mlewand Apr 24, 2018

mlewand Apr 24, 2018

mlewand Apr 24, 2018

mlewand commented Apr 24, 2018

jacekbogdanski commented Apr 25, 2018

jacekbogdanski commented Apr 25, 2018

mlewand left a comment

mlewand commented Apr 25, 2018

		## Unexpected

		The view is not changing its position depending on space below and above a caret.

		@@ -146,67 +161,52 @@
		},

		'test set position above inside absolute rect': function() {

		} );

		assert.areEqual( '300px', view.element.getStyle( 'top' ) );

Fixed autocomplete view positioning #1732

Fixed autocomplete view positioning #1732

Conversation

jacekbogdanski commented Mar 2, 2018

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

mlewand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacekbogdanski commented Mar 9, 2018 • edited Loading

mlewand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacekbogdanski commented Mar 26, 2018

mlewand commented Apr 4, 2018

jacekbogdanski commented Apr 18, 2018

jacekbogdanski commented Apr 18, 2018

mlewand left a comment

Choose a reason for hiding this comment

Classic editor does not reposition autocomplete on iOS

(Current) Expected

Actual

Firefox/MSEdge edge case

(Current) Expected

Actual

Additional notes

IE9 inline editor position fail

Expected

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

jacekbogdanski commented Apr 20, 2018 • edited Loading

mlewand left a 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

mlewand left a 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

mlewand commented Apr 24, 2018

jacekbogdanski commented Apr 25, 2018

jacekbogdanski commented Apr 25, 2018

mlewand left a comment

Choose a reason for hiding this comment

mlewand commented Apr 25, 2018

jacekbogdanski commented Mar 9, 2018 •

edited

Loading

jacekbogdanski commented Apr 20, 2018 •

edited

Loading