Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#10714 closed Bug (fixed)

[PR#82][iOS] On iPad touch listeners prevents CKEditor selection

Reported by: mbeck Owned by: pjasiun
Priority: Normal Milestone: CKEditor 4.4.1
Component: General Version: 4.3.3
Keywords: iOS HasPatch Cc: toni.rudolf@…

Description (last modified by Reinmar)

On an iPad using a Safari or Chrome browser, selection does not work when a jQuery $("body").on("touchstart") event listener is used.

To reproduce the issue CKEditor must use iframe for wysiwygarea and CKEditor container must have some offset (​https://bugs.webkit.org/show_bug.cgi?id=128924) from the document node.

Any touch listener on any ckeditor iframe parent will cause this issue on iOS also floating panels become not clickable, because focus leaves iframes before click handling happens (blur happens before click and panel hides)

Attachments (2)

ipad-touchstart-bug.html (1005 bytes) - added by mbeck 4 years ago.
10714.patch (3.0 KB) - added by artygus 3 years ago.

Download all attachments as: .zip

Change History (19)

Changed 4 years ago by mbeck

comment:1 Changed 4 years ago by j.swiderski

  • Keywords iOS added; ios ipad safari selection removed

comment:2 Changed 3 years ago by toooni

  • Cc toni.rudolf@… added

comment:3 Changed 3 years ago by artygus

  • Version changed from 4.2 to 4.3.3 (GitHub - master)

To reproduce the issue CKEditor must use iframe for wysiwygarea and CKEditor container must have some offset (https://bugs.webkit.org/show_bug.cgi?id=128924) from the document node.

Any touch listener on any ckeditor iframe parent will cause this issue on iOS also floating panels become not clickable, because focus leaves iframes before click handling happens (blur happens before click and panel hides)

Last edited 3 years ago by artygus (previous) (diff)

Changed 3 years ago by artygus

comment:4 Changed 3 years ago by artygus

  • Keywords HasPatch added
  • Summary changed from IOS: On iPad a jQuery $("body").on("touchstart") listener prevents CKEditor selection to IOS: On iPad touch listeners prevents CKEditor selection

comment:5 Changed 3 years ago by j.swiderski

  • Summary changed from IOS: On iPad touch listeners prevents CKEditor selection to [iOS] On iPad touch listeners prevents CKEditor selection

comment:6 Changed 3 years ago by Reinmar

  • Milestone set to CKEditor 4.4.1
  • Summary changed from [iOS] On iPad touch listeners prevents CKEditor selection to [PR#82][iOS] On iPad touch listeners prevents CKEditor selection

There was a pull request submitted: https://github.com/ckeditor/ckeditor-dev/pull/82

We'll review it for the 4.4.1.

comment:7 Changed 3 years ago by pjasiun

  • Owner set to pjasiun
  • Status changed from new to assigned

comment:8 Changed 3 years ago by pjasiun

  • Status changed from assigned to review

Code looks good and seems to work fine. I tested it and in fact it fixed the bug. On Chrome@Android this issue does not exist. I picked your commit and added one with code style fixes.

Changes in t/10714.

comment:9 Changed 3 years ago by Reinmar

Before we accept this patch, I would like to understand what it does. It fixes some unclear or buggy browser behaviour, so perhaps to understand it it's enough if you explain what's the issue.

comment:10 Changed 3 years ago by toooni

the issue is that if you have anywhere in your javascript a listener on a touchstart event ($("body").on("touchstart")) the ckeditor does not work anymore.. you can't move the cursor with your finger in ios.. it's just stuck at the first position..

comment:11 Changed 3 years ago by Reinmar

That's a "trigger". I'm really curious what's the reason of the issue - in other words - why is the caret stuck if you have this listener. Because this a mysterious thing.

AFAIK, that isn't the only issue fixed by this patch. For example, quoting the pull request:

fix for selection troubles and undesired floating panel hides, tested with iOS 6 and 7

comment:12 Changed 3 years ago by Reinmar

PS. comment:9 was mainly to @pjasiun. But if anyone knows the reasons, I will gladly hear them.

comment:13 Changed 3 years ago by pjasiun

After a while of investigation I found out that:

  1. the bug exists also if I use native JS instead of jQ:
    document.body.addEventListener("touchstart", function( env ) {} );
    
  2. the bug does not exist in inline editor,
  3. everything seems to be fine if there is no buttons in the toolbar,
  4. debugging on iPad is awful.

I will continue debugging on Monday.

comment:14 Changed 3 years ago by artygus

@pjasiun, sorry i've been away on vacations

  1. yes, the problem exists only with iframes (CKEditor uses iframe by default)
  2. yup
  3. iframe must have an offset from the document body (or html element, i'm not sure) - i filled the bug to webkit dev team - https://bugs.webkit.org/show_bug.cgi?id=128924
  4. you can use ios-webkit-debug proxy tool if you're on linux, or port for win32 https://github.com/artygus/ios-webkit-debug-proxy-win32

and about the floating panel hides, e.g. font size - whenever you touch anything in the iframe the blur happens first and triggers the panel to hide, i.e no scrolling occurs (only if we have (1), i.e. any of iframe parents has a touch event bound)

comment:15 Changed 3 years ago by Reinmar

  • Description modified (diff)

comment:16 Changed 3 years ago by Reinmar

  • Resolution set to fixed
  • Status changed from review to closed

Thanks Arty! I've made a final review and some changes (see git:001bbb4f) - i.e. to make no changes on other than iOS envs. There are numbers of ugly issues regarding focus in panels already fixed by CKEditor and I don't want to touch that.

As for the rest - great work on a really nasty bug.

I merged the branch to master with git:823d821.

comment:17 Changed 3 years ago by artygus

floating panel part relates to blur issues https://bugs.webkit.org/show_bug.cgi?id=133044

Last edited 3 years ago by artygus (previous) (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy