Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Edge/FF] Exception after dragging the selection through the image #5416

Closed
Mgsy opened this issue Oct 2, 2017 · 9 comments · Fixed by ckeditor/ckeditor5-utils#213
Closed
Assignees
Labels
package:ui type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Mgsy
Copy link
Member

Mgsy commented Oct 2, 2017

Steps to reproduce

  1. Go to https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/examples/builds/balloon-editor.html.
  2. Start the selection from the end of the first header.
  3. Drag it down through the image.

Current result

  • The toolbar appears in the left corner of the browser.
  • There is the exception in the console.

Error

SCRIPT438: Object doesn't support property or method 'getBoundingClientRect'
rect.js (365,1)

GIF

bug_cke5

Other information

OS: Windows 10
Browser: Edge 16, Firefox
Release: 0.11.0

@ma2ciek
Copy link
Contributor

ma2ciek commented Oct 2, 2017

In FF the selection starts at the end of the text that's inside the header, so in the https://github.com/ckeditor/ckeditor5-utils/blob/ac06329334e9f981e10df18237a986b56a43112e/src/dom/rect.js#L365, the startContainer variable is the Text element.

@ma2ciek
Copy link
Contributor

ma2ciek commented Oct 2, 2017

( range.getClientRects() returns an empty array. )

@ma2ciek
Copy link
Contributor

ma2ciek commented Oct 2, 2017

Maybe it can be solved by using the parent element instead of the original element when the startContainer is a text node?

@Reinmar
Copy link
Member

Reinmar commented Oct 2, 2017

But why isn't this throwing in all browsers?

@ma2ciek ma2ciek self-assigned this Oct 2, 2017
@ma2ciek
Copy link
Contributor

ma2ciek commented Oct 2, 2017

It's interesting that the error doesn't occur in the manual tests: http://localhost:8125/ckeditor5-image/tests/manual/caption.html.

@ma2ciek
Copy link
Contributor

ma2ciek commented Oct 2, 2017

When the selection starts inside the header, its start moves to the caption during dragging down. When dragging up all works fine.

@ma2ciek
Copy link
Contributor

ma2ciek commented Oct 2, 2017

Actually, this is caused by the BalloonEditor which calculates the rect for its position. It can be reproduced here too http://localhost:8125/ckeditor5-editor-balloon/tests/manual/ballooneditor.html

@ma2ciek
Copy link
Contributor

ma2ciek commented Oct 2, 2017

It's connected to https://github.com/ckeditor/ckeditor5-engine/issues/1156. There's a moment before the crash when selection starts at the end of the text and ends in the main editable element just after the text node. So there's no native rectangle available from getClientRects method and startContainer of the range is a text node, which doesn't have a getBoundingClientRect method.

@oleq
Copy link
Member

oleq commented Oct 3, 2017

I briefly checked this issue and the problem is in this line https://github.com/ckeditor/ckeditor5-utils/blob/7211d0269bf3e3ab7586afa987785e3dcbbcf31c/src/dom/rect.js#L365.

The startContainer is a text node, and text nodes don't have such a method. However, using text's parent here in such situation will not fix the issue completely (it will not throw, though).

Generally speaking, the client rects interface in Firefox is very experimental. Such DOM range:

<h>Foo[</h>
<img>
<p>Ba]r</p>

has 0 rects in getClientRects() and a rect full of zeros in getBoundingClientRect(). It's like it does not exist but... it renders. The API is totally broken.

I anticipated this situation a couple months ago and used a hack that retrieves the parent of the startContainer, calculates its rects and returns them as if they were range's rects.

ATM the best we can achieve is:

image

with the following fix:

diff --git a/src/dom/rect.js b/src/dom/rect.js
index 480452f..7ec5444 100644
--- a/src/dom/rect.js
+++ b/src/dom/rect.js
@@ -3,6 +3,8 @@
  * For licensing, see LICENSE.md.
  */
 
+/* global Node */
+
 /**
  * @module utils/dom/rect
  */
@@ -362,11 +364,17 @@ export default class Rect {
 		// instead and adjust rect's width to simulate the actual geometry of such range.
 		// https://github.com/ckeditor/ckeditor5-utils/issues/153
 		else {
-			const startContainerRect = new Rect( range.startContainer.getBoundingClientRect() );
-			startContainerRect.right = startContainerRect.left;
-			startContainerRect.width = 0;
+			let startContainer = range.startContainer;
+
+			if ( startContainer.nodeType == Node.TEXT_NODE ) {
+				startContainer = startContainer.parentNode;
+			}
+
+			const rect = new Rect( startContainer.getBoundingClientRect() );
+			rect.right = rect.left;
+			rect.width = 0;
 
-			rects.push( startContainerRect );
+			rects.push( rect );
 		}
 
 		return rects;

Later on, we could also consider endContainer, offsets and direction of the range to make it smarter. But considering the market share of FF, it's not gonna be so soon.

oleq referenced this issue in ckeditor/ckeditor5-utils Dec 19, 2017
Fix: `Rect.getDomRangeRects()` should not throw if the provided DOM range starts in a text node. Closes ckeditor/ckeditor5-ui#317.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-ui Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:ui labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ui type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants