Opened 7 years ago

Closed 7 years ago

#6192 closed Bug (fixed)

Webkit: editor.focus() fails if no selection ranges exist

Reported by: Jude Allred Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.4.1
Component: General Version: 3.4.1
Keywords: WebKit Cc:

Description (last modified by Frederico Caldeira Knabben)

This causes insertion into the editor to fail.

Repro steps:

  1. use Chrome
  2. go to ckeditor.com/demo
  3. without focusing the editor, attempt to insert a smiley
  4. smiley insertion fails. [expected] a smily is inserted.

Ditto for tables, attachments, and all other insertables.

I've written and tested a fix. It's attached as an hg patch file.

Attachments (5)

webkit focus.patch (1.3 KB) - added by Jude Allred 7 years ago.
6192.patch (1.2 KB) - added by Tobiasz Cudnik 7 years ago.
6192_2.patch (783 bytes) - added by Tobiasz Cudnik 7 years ago.
6192_3.patch (1.1 KB) - added by Tobiasz Cudnik 7 years ago.
6192_4.patch (1.3 KB) - added by Frederico Caldeira Knabben 7 years ago.

Download all attachments as: .zip

Change History (21)

Changed 7 years ago by Jude Allred

Attachment: webkit focus.patch added

comment:1 Changed 7 years ago by Jude Allred

Note: The patch is meant to be applied to wysiwygarea/plugin.js

comment:2 Changed 7 years ago by Krzysztof Studnik

Keywords: Webkit added
Status: newconfirmed
Version: 3.4.1 (SVN - trunk)

Bug confirmed on Chrome build 6.0.495.0

comment:3 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4.1

We're definitely having some issue with WebKit selection after [5785]. We have also #6179 and #6178 as well. We need to find a fix for it sooner than later.

comment:4 Changed 7 years ago by Sa'ar Zac Elias

Keywords: HasPatch added

comment:5 Changed 7 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: confirmedassigned

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6192.patch added

comment:6 Changed 7 years ago by Tobiasz Cudnik

Status: assignedreview

comment:7 Changed 7 years ago by Frederico Caldeira Knabben

Description: modified (diff)

comment:8 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

Some notes:

  1. Style:
BAD --> if ( editor.getSelection().getRanges().length === 0 )
GOOD -> if ( !editor.getSelection().getRanges().length )
  1. Considering that the selection is being retrieved in the if(), it should be instead cached in a variable to be reused inside of it.
  1. Strange stuff:
// What's this???
$( doc.$.body ).children()[ 0 ]

Considering the third issue, which should definitely through an error, I'm assuming that if block has never been entered, because I have no errors. So, what's the TC to make it enter there?

Other than the mysterious functions, what if body has no children?

comment:9 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

It seems this issue can be fixed by only focusing HTML element once.

I've split part of the patch (an IF block) to the #6153. All 3 issues of previous patch have been fixed.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6192_2.patch added

comment:10 Changed 7 years ago by Tobiasz Cudnik

When typing #6153, i was thinking about #6178, sorry.

comment:11 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed
  • There is no need to initialize the wasFocused variable.
  • When loading the editor (or switching back from source) with long contents, if you scroll down with the mousewhell and click in a paragraph at the bottom, the caret will be placed there, but the contents will scroll up.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6192_3.patch added

comment:12 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

Third patch fixes scrolling issue with similar way pasting does.

Unfortunately i wasn't able to prepare reduced TC for this issue, as contenteditable iframe is always focused properly, including when iframe element is generated dynamically. It seems this issue relays on our complex focus logic.

comment:13 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: WebKit added; Webkit HasPatch removed
Status: reviewreview_failed

Not been able to have a reduced tc for this browser issue represents a failure on understanding the root of the problem, not being able to solve it properly.

I've made some investigation on this, opening a WebKit bug for it:
https://bugs.webkit.org/show_bug.cgi?id=45686

Based on that, I've found out that the fix here was taking the wrong path. I'll provide something different.

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 6192_4.patch added

comment:14 Changed 7 years ago by Frederico Caldeira Knabben

Owner: changed from Tobiasz Cudnik to Frederico Caldeira Knabben
Status: review_failedreview

comment:15 Changed 7 years ago by Tobiasz Cudnik

Status: reviewreview_passed

Your patch is much better than mine for this issue, although we still need the previous one to fix #6153.

comment:16 Changed 7 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Fixed with [5882].

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy