Rect.getDomRangeRects doesn't support ranges inside the text nodes

[ma2ciek]

Running documentView.scrollToTheSelection when selection is inside some text nodes throws the following Error:

Error: Uncaught TypeError: range.startContainer.getBoundingClientRect is not a function.

[Reinmar]

And what's the actual test case? How can a user reproduce this?

[oleq]

I suppose the issue is about this line. What is interesting, though, is why the algorithm fell into this condition. It (most certainly) shouldn't unless you're using Safari.

Are you sure the range returned by this.selection.getFirstRange() makes sense in DOM?

[Reinmar]

This happened in tests, so that might've been some special, not really realistic case. But yes, it might happen in Safari.

[Reinmar]

Anyway, let's fix this quickly because we simply might've missed cases when it occurs when testing manually.

[oleq]

As it turned out in a F2F communication with @ma2ciek, the test that was the source of the issue didn't add the editor UI (editor.ui.view.element) to the DOM. Because of this, all the elements/ranges/text nodes/etc. weren't rendered at all and hence, they didn't have clientRects/boundingClientRect and the algorithm fell into the "quirks mode" trying to do its best.

What we can do now is to:

• ignore the issue because in some sort of way it's invalid (and throws anyway)
• validate the range, making sure its containers are in DOM but it sounds like an overkill

[Reinmar]

Oh, so the error is that text nodes didn't have bounding client rect because they weren't added to DOM?

This is a bug in a test obviously, but I think that we need to secure the scroll* methods anyway. Throwing errors from such tools will not be very useful (especially for tests). I'd rather make them log a warning and abort. WDYT?

[oleq]

I'd rather make them log a warning and abort. WDYT?

We can check this in the Rect#constructor by node.ownerDocument.body.contains( node ).

diff --git a/src/dom/rect.js b/src/dom/rect.js
index 49bc2aa..060baa1 100644
--- a/src/dom/rect.js
+++ b/src/dom/rect.js
@@ -11,6 +11,7 @@ import global from './global';
 import isRange from './isrange';
 import isElement from '../lib/lodash/isElement';
 import getBorderWidths from './getborderwidths';
+import log from '../log';
 
 /**
  * A helper class representing a `ClientRect` object, e.g. value returned by
@@ -60,6 +61,19 @@ export default class Rect {
 			enumerable: false
 		} );
 
+		if ( isElement( source ) || isRange( source ) ) {
+			const relatedNode = isRange( source ) ? source.startContainer : source;
+
+			if ( !relatedNode.ownerDocument.body.contains( relatedNode ) ) {
+				/**
+				 * TODO
+				 *
+				 * @error rect-source-not-in-dom
+				 */
+				log.warn( 'rect-source-not-in-dom: The source of this rect does not belong to any rendered DOM tree.', source );
+			}
+		}
+
 		if ( isElement( source ) ) {
 			copyRectProperties( this, source.getBoundingClientRect() );
 		} else if ( isRange( source ) ) {

[Reinmar]

I'd avoid checking if the DOM contains anything. That will be another problem when adding support for iframes. Can't we handle situation when there's a problem with getClientBoundingRect? We might log a warning immediately and set rect to 0,0,0,0

[oleq]

relatedNode.ownerDocument.body.contains will not cause troubles in case of the iframe because ownerDocument points to the right Document.

[Reinmar]

That's true. Sorry, I was prejudiced regarding checking that :D. So, it seems ok.

But will the change you proposed above prevent the code from throwing? Or do we need something more anyway?

[oleq]

It will throw. But the warning will give you the reason why.

Or... we can throw our own error instead of log.warn.

[Reinmar]

Hm... I initially thought about setting the rect to 0,0,0,0 so there's no crash and warning just in case (so you know that you did something incorrect). Overengineered? Is it better to just throw our error?

[oleq]

A rect with 0,0,0,0 will cause bugs in other parts of the code base anyway. I'd just throw at the very beginning and be done with it.