t/173: Created various DOM utilities to allow the view document scroll to the selection #174

oleq · 2017-08-01T14:10:34Z

Suggested merge commit message (convention)

Feature: Created various DOM utilities to allow the view document scroll to the selection. Closes ckeditor/ckeditor5#4966.

BREAKING CHANGE: The static Rect.getViewportRect method has been removed. Use new Rect( window ) instead.

…clusion of borders and scrollbars.

…arget helpers.

…ports in scroll.js.

… environment.

…e importing strategy has changed.

Reinmar · 2017-08-08T13:27:04Z

src/dom/getborderwidths.js

+import global from './global';
+
+/**
+ * Returns an object containing CSS border withs of a specified `HTMLElement`.


widths*

HTML element

Reinmar · 2017-08-08T13:29:01Z

src/dom/getborderwidths.js

+		left: computedStyles.borderLeftWidth
+	};
+
+	for ( const width in borderWidths ) {


Just use parseInt() above and return the object without iterating over it. That will be shorter and faster.

Reinmar · 2017-08-08T13:36:00Z

src/dom/rect.js

@@ -234,21 +247,63 @@ export default class Rect {
 	}

 	/**
+	 * Checks if all properties ({@link #top}, {@link #left}, {@link #right},
+	 * {@link #bottom}, {@link #width} and {@link #height}) are the same as in the other `Rect`.


are the same -> equal? properties are "the same" has a bit different meaning than "values of these properties equal ...". It's a detail and shouldn't be confusing, but I found the wording a bit unusual.

Reinmar · 2017-08-08T13:40:28Z

src/dom/rect.js

+	}
+
+	/**
+	 * Checks whether a `Rect` fully contains another `Rect` instance.


The same issue as everywhere else – I don't think that it makes sense to abuse preformatted text and type names. IMO, this makes reading the descriptions harder because the text is less heterogeneous. Plus, it's semantically incorrect as well because you're not checking Rect (type) here but a rect instance.

For me, it should be: "Checks whether this rect fully contains another rect instance."

The same applies to many other places in these docs.

cc @AnnaTomanek

Reinmar · 2017-08-08T13:45:26Z

src/dom/rect.js

+	contains( anotherRect ) {
+		const intersectRect = this.getIntersection( anotherRect );
+
+		if ( !intersectRect || !intersectRect.isEqual( anotherRect ) ) {


return intersectRect && intersectRect.isEqual( anotherRect );?

Reinmar · 2017-08-08T13:56:15Z

src/dom/rect.js

 	 */
-	static getViewportRect() {
+	static getViewportRect( options = {} ) {


Why does this method look like one big hack? There's something wrong with this method's constructor if you need to override stuff like _source and call private methods once an instance was initialised.

I removed the method and allowed new Rect( window ). See the updated docs.

Reinmar · 2017-08-08T14:03:06Z

src/dom/rect.js

+		rect._source = global.window;
+
+		if ( options.excludeScrollbars ) {
+			rect._excludeScrollbarsAndBorders();


Couldn't this be a public method? It'd make more sense to call it manually, rather than add the options param everywhere.

I made a public method out of this thing.

Reinmar · 2017-08-08T14:20:13Z

src/dom/scroll.js

+ * by keeping the `target` some distance from the edge of the viewport and thus making it easier to
+ * read or edit by the user.
+ */
+function scrollViewportToShowTarget( { target, viewportOffset = 0 } ) {


won't scrollViewportToShowTarget( { target } ) crash? It will leave viewportOffset set to undefined or not? I can never remember;|

It won't. See the tests and tutorials.

OK, the problems I remembered were with this notation: function x( a = { x: 1, y: 2 } ).

Reinmar · 2017-08-08T14:23:49Z

src/dom/scroll.js

+	//
+	// To prevent this, we're checking the rects moved by the viewportOffset to cover
+	// the upper/lower edge of the viewport.
+	const rects = [ targetShiftedUpRect, targetShiftedDownRect ];


Yyyy... Are you sure about this method's code? I don't want to analyse it :D

Yes. It's a pure magic 😄

Reinmar · 2017-08-08T14:48:54Z

src/dom/scroll.js

+	} while ( ( parent = parent.parentNode ) );
+}
+
+export default {


Why exporting this as an object? For testing purposes? Please explain it then. And in fact, it's better to expose these functions separately and then expose a test object with the two of them. So that object will be protected, while the functions will be nice and clean and exposed separately.

Yes. It's for testing purposes, e.g. to mock scrollAncestorsToShowTarget() and check if scrollViewportToShowTarget() always calls it first.

Without the mock, the test becomes ridiculously complex because both viewport and ancestors are to be scrolled and it's fairly obvious that scrollAncestorsToShowTarget() is already tested extensively in the other place.

OK, but you can still code this in a clearer way:

const module = { a, b }; export function a() { module.b(); } export function b() { ... } export { module as _test };

Thanks to the fact that a() uses b() through module you can still mock the stuff.

Reinmar · 2017-08-08T14:51:34Z

src/dom/scroll.js

+/* global Node */
+
+/**
+ * @module utils/dom/scroll


Do I see correctly that there are no tests for this module?

There are lots of them.

OK, they got folded so I missed them.

Reinmar · 2017-08-09T08:21:10Z

I love the manual test :D The description is unclear (it's too complicated – it's hard to figure out what you're actually supposed to do) but you can play with these elements in various ways.

I'd only do one change – make the squares borders' rounded. That will help in recognizing whether the entire square is visible. Something like 5px will be enough. You can also think about different style which might help. 1px border would also be good.

…ct and replaced it with new Rect( window ). Introduced a new instance method #excludeScrollbarsAndBorders.

…API.

…ng purposes.

Reinmar · 2017-08-15T16:38:35Z

src/dom/rect.js

 			enumerable: false
 		} );

 		if ( isElement( source ) ) {
 			copyRectProperties( this, source.getBoundingClientRect() );
 		} else if ( isRange( source ) ) {
 			copyRectProperties( this, Rect.getDomRangeRects( source )[ 0 ] );
+		} else if ( source === global.window ) {


What about other windows? Iframes will have their own. This deserves a followup.

PS. The next line is not optimal (you could use source).

Reinmar · 2017-08-15T16:40:02Z

src/dom/rect.js

+		let scrollBarWidth, scrollBarHeight;
+
+		if ( source === global.window ) {
+			scrollBarWidth = global.window.innerWidth - global.document.documentElement.clientWidth;


Just like I commented above – depending on the main window and document is unsafe.

Reinmar · 2017-08-15T16:40:41Z

src/dom/rect.js

+	 * @returns {Boolean} `true` if contains, `false` otherwise.
+	 */
+	contains( anotherRect ) {
+		const intersectRect = this.getIntersection( anotherRect );


What about two rects next to each other?

OK, nvmnd. I didn't ask :D

Reinmar · 2017-08-15T16:52:03Z

And here's the followup: https://github.com/ckeditor/ckeditor5-utils/issues/175.

oleq added 8 commits July 28, 2017 14:42

Implemented the getBorderWidths() DOM helper.

0ae9dba

Implemented Rect class methods #isEqual() and #contains(). Allowed ex…

c8eb083

…clusion of borders and scrollbars.

Used getBorderWidths() helper in getOptimalPosition().

ee26338

Tests: Added manual test for scrollViewportToShowTarget helper.

9a5f18e

Implemented the scrollViewportToShowTarget and scrollAncestorsToShowT…

fc98335

…arget helpers.

Exported an object containing methods instead of separate function ex…

db2a4e9

…ports in scroll.js.

Tests: Fixed broken getBorderWidths() test.

95a10f1

Tests: Made the tests immune to the border-width-* styles in the test…

5308898

… environment.

oleq mentioned this pull request Aug 1, 2017

t/660: Implemented ViewDocument#scrollToTheSelection method ckeditor/ckeditor5-engine#1057

Merged

oleq added 3 commits August 1, 2017 16:25

Tests: Fixed scrollViewportToShowTarget manual test because the modul…

844589f

…e importing strategy has changed.

Docs: Fixed docs errors in the Rect class.

6ca4246

Merge branch 'master' into t/173

b682284

Reinmar reviewed Aug 8, 2017

View reviewed changes

Kamil Piechaczek and others added 2 commits August 9, 2017 16:12

Upgraded tests to the latest Chai and Sinon version.

31a2475

Internal: Updated eslint-config-ckeditor5. [skip ci]

3869e1a

Reinmar added the status:review- label Aug 9, 2017

oleq added 3 commits August 10, 2017 11:15

Merge branch 'master' into t/173

382d38b

Tests: Updated stubs to the latest Sinon API.

7970957

Docs fixes and code refactoring in the getBorderWidths utility.

a70eb1f

oleq added 5 commits August 10, 2017 14:28

Code refactoring and docs improvements in the dom/rect module.

5bdcf64

Code refactoring in the Rect class: removed static Rect.getViewportRe…

dccccd8

…ct and replaced it with new Rect( window ). Introduced a new instance method #excludeScrollbarsAndBorders.

Aligned dom/position and dom/scroll modules to the latest Rect class …

11a9083

…API.

Made dom/scroll module export hlper functions and an object for testi…

32a3c61

…ng purposes.

Docs: Fixed JSDoc errors.

0087b21

Reinmar reviewed Aug 15, 2017

View reviewed changes

Reinmar added 2 commits August 15, 2017 18:44

Corrected API docs.

1aec85d

Updated manual test to match the API change.

154ec66

Reinmar merged commit b143e9c into master Aug 15, 2017

Reinmar deleted the t/173 branch August 15, 2017 16:50

Reinmar removed the status:review- label Aug 15, 2017

Reinmar mentioned this pull request Oct 9, 2019

Rect and scroll utilities should support multi window scenarios (aka iframe support) ckeditor/ckeditor5#4967

Closed

oleq mentioned this pull request May 18, 2020

JSDoc validator: Add Window as a valid type ckeditor/ckeditor5#7016

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

t/173: Created various DOM utilities to allow the view document scroll to the selection #174

t/173: Created various DOM utilities to allow the view document scroll to the selection #174

oleq commented Aug 1, 2017 •

edited by pomek

Loading

Reinmar Aug 8, 2017

Reinmar Aug 8, 2017

Reinmar Aug 8, 2017

Reinmar Aug 8, 2017

Reinmar Aug 8, 2017

Reinmar Aug 8, 2017

Reinmar Aug 8, 2017

oleq Aug 10, 2017

Reinmar Aug 8, 2017

oleq Aug 10, 2017

Reinmar Aug 8, 2017

oleq Aug 8, 2017

Reinmar Aug 9, 2017

Reinmar Aug 8, 2017

oleq Aug 10, 2017

Reinmar Aug 8, 2017

oleq Aug 8, 2017 •

edited

Loading

Reinmar Aug 9, 2017 •

edited

Loading

Reinmar Aug 8, 2017

oleq Aug 8, 2017

Reinmar Aug 9, 2017

Reinmar commented Aug 9, 2017

Reinmar Aug 15, 2017

Reinmar Aug 15, 2017

Reinmar Aug 15, 2017

Reinmar Aug 15, 2017

Reinmar commented Aug 15, 2017

t/173: Created various DOM utilities to allow the view document scroll to the selection #174

t/173: Created various DOM utilities to allow the view document scroll to the selection #174

Conversation

oleq commented Aug 1, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleq Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Reinmar Aug 9, 2017 • 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

Reinmar commented Aug 9, 2017

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

Reinmar commented Aug 15, 2017

oleq commented Aug 1, 2017 •

edited by pomek

Loading

oleq Aug 8, 2017 •

edited

Loading

Reinmar Aug 9, 2017 •

edited

Loading