Added method 'getClientRects()' to CKEDITOR.dom.range #1499

engineering-this · 2018-01-25T10:13:04Z

What is the purpose of this pull request?

New feature

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
This feature will be used in my upcoming PR on balloonPanel and this usage will be covered in manual tests.

What changes did you make?

I've added getClientRects method which uses native method with same name, also implemented fallback for browsers that doesn't support it.

Closes #1498

mlewand · 2018-01-25T15:42:05Z

core/dom/range.js

+		 * Returns Array of rects from dom elements contained within current ranges.
+		 *
+		 * @since 4.9.0
+		 * @private


I don't think we need to mark this method as a private. Note that private members are usually prefixed with an underscore.

mlewand · 2018-01-25T15:42:28Z

core/dom/range.js

+		/**
+		 * Returns Array of rects from dom elements contained within current ranges.
+		 *
+		 * @since 4.9.0


There are no docs on returned type.

mlewand · 2018-01-25T15:43:01Z

core/dom/range.js

@@ -2818,6 +2818,69 @@ CKEDITOR.dom.range = function( root ) {
 			reference.remove();
 		},

+		/**
+		 * Returns Array of rects from dom elements contained within current ranges.


rects -> rectangles
Array -> array (you're not using it as a type, but a regular word)
dom -> DOM

I'm not sure if this summary describes well enough what this method does.

It returns rectangles of areas covered by ranges, not by elements contained by the range (element could be contained partially, and based on this description one could think that despite that fact returned rectangles will contain the entire element, which is not true).

mlewand · 2018-01-25T15:53:50Z

core/dom/range.js

+					rects = [ start.getClientRect(), start.getClientRect() ];
+					start.remove();
+				}
+				rect.right = Math.max( rects[ 0].right, rects[ 1 ].right );


Code style issue, missing space before bracket. same in next line.

mlewand · 2018-01-25T15:59:08Z

tests/core/dom/range/getclientrects.js

+	var tests = {
+		'test paragraphs': function() {
+			var bot = this.editorBot,
+			editor = this.editor;


Wrong indentation.

mlewand · 2018-01-25T16:03:21Z

tests/core/dom/range/getclientrects.js

+( function() {
+	'use strict';
+
+	bender.editor = {};


You don't really need to use editor here. Actually you can work on regular HTML elements. Just go with:

var rng = new CKEDITOR.dom.range( CKEDITOR.document );

and play with your range.

mlewand · 2018-01-25T16:04:30Z

core/dom/range.js

+			}
+
+			if ( this.document.$.getSelection !== undefined ) {
+				var sel = this.document.$.getSelection(),


You're working on a global selection here, which is not the same as your context selection.

Just imagine that you create a new range in memory in order to use it later or whatever, but you do no actually select it. E.g.

var rng = new CKEDITOR.dom.range( CKEDITOR.document ) rng.setStart( … ); rng.setEnd( … ); var rects = rng.getClientRects();

👆 this case needs to be included in unit tests.

I can see also a design flaw in our Range implementation which will make your task harder - AFAICT we're not keeping a reference to the native Range object.

If that's the case your only solution for this will be to temporarily create a new range, use it to probe what you need to check and then destroy it.

Also there is a need to handle our fake selection - however we could extract it into a separate ticket. But please familiarize your self with this aspect already (talk to other devs if you need a help with that).

engineering-this · 2018-01-29T12:14:12Z

core/dom/range.js

+				rect.bottom = Math.max( rects[ 0 ].bottom, rects[ 1 ].bottom );
+				rect.left = Math.min( rects[ 0 ].left, rects[ 1 ].left );
+				rect.top = Math.min( rects[ 0 ].top, rects[ 1 ].top );
+				rect.width = Math.abs( rects[ 0 ].left - rects[ 1 ].left );


I am thinking if I should use extra tabulations to improve readability. i.e:

rect.bottom = Math.max( rects[ 0 ].bottom, rects[ 1 ].bottom ); rect.left = Math.min( rects[ 0 ].left, rects[ 1 ].left ); rect.top = Math.min( rects[ 0 ].top, rects[ 1 ].top ); rect.width = Math.abs( rects[ 0 ].left - rects[ 1 ].left ); rect.height = rect.bottom - rect.top;

Late answer but we are not using this kind of whitespace alignment. Reason is that it is just hard to maintain as you change one of these 5 lines, you need to modify 4 other.

It's fine the way it currently is 👍

mlewand

One TC is way too little.

There are cases that are not covered, for instance, a selection where there are multiple rectangles returned, for instance selection like that:

Also please, rebase the branch onto latest 4.10.0 branch. Make sure to move the changelog entry.

mlewand · 2018-03-09T14:10:25Z

tests/core/dom/range/getclientrects.js

+				var listToCompare = getListToCompare( range );
+				rectList.forEach( function( item, index ) {
+					for ( var key in item ) {
+						if ( typeof item[ key ] === 'number' ) {


Be more explicit about asserted properties, for instance you could simply whitelist them in some array.

mlewand · 2018-03-09T14:12:33Z

tests/core/dom/range/getclientrects.js

+				rectList.forEach( function( item, index ) {
+					for ( var key in item ) {
+						if ( typeof item[ key ] === 'number' ) {
+							assert.isTrue( item[ key ] === listToCompare[ index ][ key ] );


This is not a proper way to assert primitive values. Assertion message will not be useful at all if the assertion fails.

It:

will not contain any useful expected, actual value (will only say that was expecting true, while false given)

will not contain any useful information on what number caused the fail. Was it top, left etc? This information should be included in a short assertion message.

mlewand · 2018-03-09T14:13:34Z

tests/core/dom/range/getclientrects.js

+			// This assert is for IE8, but it should pass on newer browsers.
+			var keys = [ 'width', 'height', 'top', 'bottom', 'left', 'right' ];
+			CKEDITOR.tools.array.forEach( keys, function( key ) {
+				assert.isTrue( rectList[ 0 ][ key ] >= 0 );


Not really a solid assertion for IE8. Can't we write a better assertion here? Something with real data?

mlewand · 2018-03-09T14:17:49Z

core/dom/range.js

+
+			if ( this.document.$.getSelection !== undefined ) {
+				var range = document.createRange(),
+					rectList,


Uninited vars should be declared last.

mlewand · 2018-03-09T14:18:07Z

core/dom/range.js

+				var range = document.createRange(),
+					rectList,
+					rectArray = [];
+				range.setStart( this.startContainer.$, this.startOffset );


It would be nice to put one line of space here, before this line.

mlewand · 2018-03-09T14:24:51Z

core/dom/range.js

+				return rect;
+			}
+
+			if ( this.document.$.getSelection !== undefined ) {


Rather than executing this code every time function getClientRects is called, it make sense to do it only once like it's done in other core mentods: https://github.com/ckeditor/ckeditor-dev/blob/4fa6edd/core/dom/element.js#L1073-L1116

mlewand · 2018-03-16T10:15:36Z

Sorry, I've removed the next branch before rebasing all the PRs that were based on it. This PR is now rebased onto latest major (4.10.0).

Comandeer · 2018-03-30T14:41:14Z

Please also refer to my comments from #1627 (review).

mlewand

I was concerned with the result returned in Internet Explorer 8 (using this polyfill):

Note that returned rectangle is marked with a red border.

Given the fact that there's no easy way to retrieve all selection markup, I think that the solution you took is the most optimal one. Return a rectangle between the boundary points. This will work great e.g. with context menu and will work just fine if we are to use it with something like balloon toolbar.

However this needs to be well documented, so in API docs provide an information what kind of result is returned for browsers that do not support this feature. So please add a proper docs on it.

I don't like the fact that tests are not precise, and have some tolerance. In addition to that the tests could be made more easy to read/understand in general.

I gave it some testing, and what we can make selections on something that has a fixed size, and then compare the returned rectangle(s). Now intuitively you'd want to use a monospace font for that purpose, however browser do not use fixed width for monospace fonts (shame). So from there the only alternative solution is some fixed size inline content, like images or simply spans with a fixed width.

Lastly during the testing I have witnessed that IE 11 (and possibly others) have a (upstream) bug that will not return rectangle for image selected:

This is something that we could work around with a feature in this function but that's outside of a scope here - please create a followup ticket for htat.

mlewand · 2018-04-04T13:02:46Z

CHANGES.md

@@ -8,6 +8,10 @@ Fixed Issues:
 * [#1458](https://github.com/ckeditor/ckeditor-dev/issues/1458): [Edge] Fixed: After blurring editor it takes 2 clicks to focus a widget.
 * [#1034](https://github.com/ckeditor/ckeditor-dev/issues/1034): Fixed: JAWS leaves forms mode after pressing <kbd>Enter</kbd> key in an inline CKEditor instance.

+API changes:
+
+* [#1498](https://github.com/ckeditor/ckeditor-dev/issues/1498) : Added new method 'getClientRects()' to CKEDITOR.dom.range, which returns list of rects for each selected element.


Move changelog entry to the new target release (4.10.0).

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

core/dom/range.js

@@ -2818,6 +2818,83 @@ CKEDITOR.dom.range = function( root ) {
 			reference.remove();
 		},

+		/**
+		 * Returns array with rectangles of areas covered by ranges. For each DOM element within range there is one rectangle, but it represents only part of element which is within range, not whole element.


This line is too long. It looks that you had linter disabled, as otherwise it would prevent the commit.

Please split it into a bit narrower text.

Also this description is not clear, please rephrase it as simply as you can. Feel free to ask Anna for help - this method needs to be understandable.

Also it is very important to describe the fallback for browsers that do not support range.getClientRects() natively. I mean describe what area of selection is used.

It would be nice to use images, eventually ascii art to present this information as it might be hard to describe it with words only 🙂

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

core/dom/range.js

+		/**
+		 * Returns array with rectangles of areas covered by ranges. For each DOM element within range there is one rectangle, but it represents only part of element which is within range, not whole element.
+		 *
+		 * @since 4.9.0


Please update the target, it's now 4.10.0.

mlewand · 2018-04-04T14:06:36Z

tests/core/dom/range/getclientrects.js

+		'test inline element': function() {
+			setupTest( getHtmlForTest( 'span', 'test' ), true );
+		},
+		'test multi-line inline element': function() {


It's multiline rather than multi-line (I believe the later is correct too, but we're not using it in our codebase).

mlewand · 2018-04-05T09:26:45Z

core/dom/range.js

+		 * @returns {Array}
+		 */
+		getClientRects: ( function() {
+			// Extending empty object wth rect, to prevent inheriting from DOMRect, same approach as in CKEDITOR.dom.element.getClientRect().


"wth" -> with

mlewand · 2018-04-05T09:29:13Z

core/dom/range.js

+
+			if ( this.document.getSelection !== undefined ) {
+				return function() {
+					var range = document.createRange(),


How about a word on why are we duplicating the range? I'm missing short comment with rationale here.

mlewand · 2018-04-05T09:30:17Z

core/dom/range.js

+					rectList = range.getClientRects();
+					range.detach();
+
+					for ( var i = 0; i < rectList.length; i++ ) {


You could simplify this significantly by using CKEDITOR.tools.array.map( range.getClientRects(), … ).

mlewand · 2018-04-05T09:33:06Z

core/dom/range.js

+					start.remove();
+				}
+
+				rect.right = Math.max( rects[ 0 ].right, rects[ 1 ].right );


Instead of assigning rect like that, how about simply returning with

return { top: Math.min( rects[ 0 ].top, rects[ 1 ].top ), … };

That way you'll have one variable less in the scope and you'll get rid of these assignments.

mlewand · 2018-04-05T11:14:12Z

Also a manual test would help a lot, I have pushed a branch showing how easier unit tests could be achieved and that contains a nice base for manual test. You can find it on branch 1498_tests.

mlewand · 2018-04-06T15:46:53Z

Also since #1866 got into the major branch, please mark the returned value as an array of CKEDITOR.dom.rect.

mlewand

Checked just couple of things in docs, it needs further corrections before it could be further reviewed.

mlewand · 2018-04-10T12:20:08Z

core/dom/range.js

+		 *
+		 * In the following example:
+		 *
+		 *        <p><span>first { span</span><span> second text</span></p>


Use tabs for indentation instead of spaces.

Use triple backticks for code samples, see ckeditor/ckeditor4-docs#131

Also mark the code listing language for proper syntax highlighting.

mlewand · 2018-04-10T12:52:53Z

core/dom/range.js

+		 *             └   ┘
+		 *
+		 * @since 4.10.0
+		 * @returns {Array} Returns an array of {@link CKEDITOR.dom.rect}


This is not an optimal way of marking returned array. It should be marked as @returns {Type[]}.

mlewand · 2018-04-10T12:55:57Z

core/dom/range.js

+		 * Brackets represent the beginning and the end of the selection.
+		 *
+		 * Returned rectangles would be represented by areas like below:
+		 *         text [ text ] [ text text ]


That doesn't render correctly in our docs. Be sure to always build docs and preview it before you publish something.

engineering-this · 2018-04-10T14:14:40Z

core/dom/range.js

+		 *
+		 * ```html
+		 *  <p><span>first { span</span><span>second span</span></p>
+		 *  <p><span>very long } span</span></p>


For some reason JSDuck wont create codeblock here if I don't put two spaces or two tabulations here. Couldn't fix that even with help from @msamsel.

mlewand · 2018-04-10T14:29:25Z

core/dom/range.js

+		 *      └   ┘
+		 * ```
+		 * @since 4.10.0
+		 * @returns {CKEDITOR.dom.rect[]} Returns an array of {@link CKEDITOR.dom.rect}


Returns an array of {@link CKEDITOR.dom.rect} does not bring any value here, this informaiton is already expressed by @returns {CKEDITOR.dom.rect[]} so it's OK to remove it.

The same thing is repeated at the beginning of description for this method.

mlewand · 2018-04-11T07:08:08Z

core/dom/range.js

@@ -2818,6 +2818,114 @@ CKEDITOR.dom.range = function( root ) {
 			reference.remove();
 		},

+		/**
+		 * Returns an array of {@link CKEDITOR.dom.rect} elements that are represented as rectangles which are covered by ranges.
+		 * For each DOM element within the selection range there is only one rectangle.


For each DOM element within the selection range there is only one rectangle.

I don't feel like going though spec of (native browser) range.getClientRects() but I believe it's not guaranteed to return a rectangle for each DOM element. For instance Edge does not do that.

Let's be more generic about that, saying that it's a rect list describing the selection.

mlewand · 2018-04-11T07:11:53Z

core/dom/range.js

+		 * Returned rectangles would be represented by areas like below:
+		 *
+		 * ```
+		 * first [ span ] [ second span ]


You should be more strict about spacing in this example. I know that you wanted to do that for readability, but it's important to use spaces as they're used in your input HTML.

For instance there's no space between "span" and "second".

Again the returned result will differ accross the browsers, as some of them might unify (simplify/optimize) returned rectangles. That should be somehow marked as a note.

mlewand · 2018-04-11T07:31:17Z

core/dom/range.js

+		 *
+		 * Where each pair of brackets represents one rectangle.
+		 *
+		 * In Internet Explorer 8 this method will return an array containing only one rectangle which would start in the top left hand corner of the selection and end in the bottom right hand corner.


We should also mention why is this the case. That's because IE8 has no implementation of range.getClientRects().

mlewand · 2018-04-11T07:40:47Z

core/dom/range.js

+		 *      ┌   ┐
+		 * first span second span
+		 * very long span
+		 *      └   ┘


To be honest I find it very difficult to provide accurate ASCI art here. The one propsed by you implies like there's area bove "span" and below "long".

I came with something like that:

+-----+ first| span| second span very |long |span +-----+

Which has similar issue and is still far from decent in my opinion.

Instead how about linking an image (I guess that would be a first image in our API docs, I'm so excited). We could use Cloud Services for hosting the image, contact me on how to perform that.

mlewand · 2018-04-11T08:50:59Z

tests/core/dom/range/getclientrects.js

+				this.playground = doc.getById( 'playground' );
+			},
+
+			'test simple single line selection': function() {


Am I missing something or is there only one unit test? There are much more cases to be covered.

Edge is squashing rects into one per line. Now I'm not sure which is better: write extra assertions for Edge, or skip tests with more than one element selected? What is your thought on that @mlewand

We want to have proper testing for each environment, so we can't skip any browser. I think there are going to be three correct results:

resutls returned by modern browsers

results returned by Edge and IE

results returned by polyfilled getClientRects (IE8)

There was a simplification possible, where you could simply dynamically remove rect.x / rect.y property from each expected results for IE/Edge browsers rather than duplicating all the fixtures. Other than that, changed variable naming, transformed triple ternary operator etc.

mlewand

LGTM

uladzimir-miadzinski · 2019-02-22T11:06:37Z

can you also fix bug with setStart and setEnd methods of CKEDITOR.dom.range?
when offset bigger than content length, I have an error

zone.js:199 Uncaught DOMException: Failed to execute 'setStart' on 'Range': The offset 1 is larger than the node's length (0).
    at push.../node_modules/ckeditor/ckeditor.js.window.CKEDITOR.window.CKEDITOR.dom.CKEDITOR.dom.range.getClientRects

i got this error in 4.11.2
and I think the fix can be like that:

            if (this.startContainer.$.length < this.startOffset) {
                this.startOffset = this.startContainer.$.length;
            }

            if (this.endContainer.$.length < this.endOffset) {
                this.endOffset = this.endContainer.$.length;
            }

mlewand · 2019-02-25T13:19:46Z

@medinsky this is a request unrelated to this PR. Feel free to request a feature request like that. I'll mention though that it shows that there might be a problem somewhere underneath which should be fixed.

In other words that would be fixing a symptoms not the root cause of the problem.

mlewand suggested changes Jan 25, 2018

View reviewed changes

engineering-this commented Jan 29, 2018

View reviewed changes

engineering-this requested a review from mlewand January 29, 2018 12:16

engineering-this mentioned this pull request Jan 30, 2018

Added possibility to attach balloon panel to selection instead of el… #1526

Merged

2 tasks

engineering-this mentioned this pull request Feb 8, 2018

Fixed: Context menu is incorrectly positioned when opened with Shift-F10 #1627

Merged

2 tasks

mlewand suggested changes Mar 9, 2018

View reviewed changes

engineering-this force-pushed the t/1498 branch from 4fa6edd to 306b2e0 Compare March 12, 2018 16:37

engineering-this changed the base branch from major to next March 12, 2018 16:37

engineering-this requested a review from mlewand March 12, 2018 16:41

mlewand closed this Mar 16, 2018

mlewand changed the base branch from next to major March 16, 2018 10:15

mlewand reopened this Mar 16, 2018

mlewand suggested changes Apr 5, 2018

View reviewed changes

mlewand mentioned this pull request Apr 6, 2018

Create an abstract API doc type for DOM rectangle #1866

Closed

engineering-this force-pushed the t/1498 branch 5 times, most recently from 01babb0 to b5c3628 Compare April 10, 2018 08:29

engineering-this requested a review from mlewand April 10, 2018 10:08

mlewand suggested changes Apr 10, 2018

View reviewed changes

engineering-this commented Apr 10, 2018

View reviewed changes

engineering-this requested a review from mlewand April 10, 2018 14:15

mlewand suggested changes Apr 11, 2018

View reviewed changes

engineering-this force-pushed the t/1498 branch from 3f15c75 to 4fae2a6 Compare April 12, 2018 11:17

engineering-this requested a review from mlewand April 12, 2018 11:22

engineering-this and others added 17 commits April 17, 2018 11:57

Reworked tests, added more test cases, codestyle

83bb842

Changelog entry after rebase

03bbd96

typo

be55050

Refactoring

2df6f2d

Updated tests

cad2f6c

Updated docs, test descriptions, polishing

5bdcc64

Fixed docs entry

986773d

Renamed manual test

1de8b63

Added test cases

2e4ad0a

Updated docs

df5fab8

Refactoring

f18869f

Refactoring tests

9f68b76

Changed test steps description

78417da

Docs: added some spacing to range.getClientRects API docs.

5e15d81

Fixed test on iOS

b20da6f

Fixed codestyle

3fa8da5

engineering-this force-pushed the t/1498 branch 2 times, most recently from 8fb441f to cb39233 Compare April 17, 2018 10:16

engineering-this requested a review from mlewand April 17, 2018 10:24

engineering-this force-pushed the t/1498 branch from cb39233 to 0a0592c Compare April 17, 2018 10:27

Updated test notes for IE8

dfa498b

engineering-this force-pushed the t/1498 branch from 0a0592c to dfa498b Compare April 17, 2018 10:28

engineering-this added 2 commits April 17, 2018 14:46

Fixed assertion testing only right

14690bc

Changed ticket number to link in test description

2fffd5f

mlewand approved these changes Apr 17, 2018

View reviewed changes

mlewand merged commit ea438fc into major Apr 17, 2018

mlewand mentioned this pull request Apr 17, 2018

Add method 'getClientRects()' to CKEDITOR.dom.range #1498

Closed

Added method 'getClientRects()' to CKEDITOR.dom.range #1499

Added method 'getClientRects()' to CKEDITOR.dom.range #1499

Conversation

engineering-this commented Jan 25, 2018 • edited

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

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

engineering-this Jan 29, 2018 • edited

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

Choose a reason for hiding this comment

mlewand commented Mar 16, 2018

Comandeer commented Mar 30, 2018

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

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 5, 2018

mlewand commented Apr 6, 2018

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

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

engineering-this Apr 12, 2018 • edited

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

uladzimir-miadzinski commented Feb 22, 2019 • edited

mlewand commented Feb 25, 2019

engineering-this commented Jan 25, 2018 •

edited

engineering-this Jan 29, 2018 •

edited

engineering-this Apr 12, 2018 •

edited

uladzimir-miadzinski commented Feb 22, 2019 •

edited