Opened 10 years ago

Closed 6 years ago

Last modified 6 years ago

#1272 closed Bug (fixed)

Safari : It is not possible to apply styles to collapsed selection

Reported by: Frederico Caldeira Knabben Owned by: Frederico Caldeira Knabben
Priority: Must have (possibly next milestone) Milestone: CKEditor 3.5.3
Component: Core : Styles Version: SVN (FCKeditor) - Retired
Keywords: IBM WebKit Cc: Satya Minnekanti, Damian, joek, Jude, Allred, lnagpal@…, jamcunni@…

Description

For ticket #253, the required feature has been created for FF, IE and Opera. We haven't found a feasible way to have this feature on Safari, so more investigation is needed.

Attachments (9)

webkitFix.patch (5.4 KB) - added by Jude Allred 7 years ago.
A 90% fix for this bug
1272.patch (4.8 KB) - added by Frederico Caldeira Knabben 7 years ago.
1272_2.patch (10.2 KB) - added by Garry Yao 7 years ago.
1272_3.patch (4.9 KB) - added by Frederico Caldeira Knabben 7 years ago.
1272_4.patch (6.5 KB) - added by Frederico Caldeira Knabben 6 years ago.
1272_5.patch (6.6 KB) - added by Frederico Caldeira Knabben 6 years ago.
1272_6.patch (10.4 KB) - added by Frederico Caldeira Knabben 6 years ago.
1272_7.patch (10.3 KB) - added by Frederico Caldeira Knabben 6 years ago.
1272_8.patch (10.9 KB) - added by Frederico Caldeira Knabben 6 years ago.

Download all attachments as: .zip

Change History (56)

comment:1 Changed 10 years ago by Frederico Caldeira Knabben

Priority: NormalHigh

comment:2 Changed 10 years ago by Frederico Caldeira Knabben

comment:3 Changed 9 years ago by Wojciech Olchawa

Keywords: Confirmed added

comment:4 Changed 9 years ago by Frederico Caldeira Knabben

#2553 has been marked as DUP

comment:5 Changed 9 years ago by Frederico Caldeira Knabben

#1939 has been marked as DUP

comment:6 Changed 8 years ago by Frederico Caldeira Knabben

#3708 has been marked as DUP

comment:7 Changed 8 years ago by Frederico Caldeira Knabben

#4071 has been marked as DUP

comment:8 Changed 8 years ago by Frederico Caldeira Knabben

#4070 has been marked as DUP

comment:9 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: WebKit added

comment:10 Changed 8 years ago by Frederico Caldeira Knabben

#4747 has been marked as DUP

comment:11 Changed 8 years ago by Frederico Caldeira Knabben

#4807 has been marked as DUP.

comment:12 Changed 8 years ago by Damian

What version of CKEditor is this fix targeted for?

comment:13 Changed 8 years ago by Damian

Keywords: IBM added

comment:14 in reply to:  12 Changed 8 years ago by Frederico Caldeira Knabben

Replying to damo:

What version of CKEditor is this fix targeted for?

Please look at the Milestone field. We have not targeted it to any future release.

This is one of our most wanted bugs to get fixed in WebKit. The fact is that it looks like we don't have much to do here and the fix depends totally on Apple. I've contact them a few times in the past about it, but so far we had no evolution on this (two years trying).

Maybe others may also try contacting them about this issue.

comment:15 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Cc: Satya Minnekanti Damian joek added

#5331 has been marked as dup

comment:16 Changed 7 years ago by Frederico Caldeira Knabben

#5929 has been marked as DUP.

comment:17 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Cc: Jude Allred added

#5982 has been marked as dup

comment:18 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: CantFix added

comment:19 Changed 7 years ago by Frederico Caldeira Knabben

We're opening a bounty for a fix at the WebKit side:
https://lists.webkit.org/pipermail/webkit-dev/2010-September/014251.html

I really hope it'll work this time. Let's stay watching it.

comment:20 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Cc: lnagpal@… added

#1272 has been marked as dup

Changed 7 years ago by Jude Allred

Attachment: webkitFix.patch added

A 90% fix for this bug

comment:21 Changed 7 years ago by Jude Allred

I attached a patch which may be useful to some of you in addressing this bug. I'm not sure I'd advocate including it in CKEditor core, and it would certainly need to be modified to conform to CKEditor's style guide, however It can be useful as a workaround. The technique is based on some patches that others here have submitted.

comment:22 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: HasPatch added

Hi Jude Allred

Thanks for the patch. Can you explain why do you say that it's 90%?
Having 90% is better than 0%, but it would save some time testing the patch to check what it's supposed to do and what is missing :-)

comment:23 Changed 7 years ago by Jude Allred

Sure :-)

As you know, Webkit has some issues with applying formatting to empty selections. This patch gets around that by intercepting the code path which would apply formatting to an empty selection and injecting a zero-width nonbreaking space, selecting it, applying the formatting, and then (if it hasn't already been overwritten) removing the zero-width nonbreaking space.

The zero-width nonbreaking space idea came from a patch submitted in Ticket #3708, however I've modified it heavily.

Since the selection is no longer empty, it works! That's the easy part of the patch.. the tricky part is the code which cleans up the injected "bogus nodes", since there are countless ways to move a selection and we certainly don't want our editor littered with the zero-width spaces.

So, I built this with the goal of making the quirks as minimal as possible. In causual use, most people don't notice, which is great! In fact, since deploying this fix for our FogBugz users in August, the only complaints we've received about style behavior in Webkit have come from our own QA team. Here's the shortlist of edge cases which aren't addressed by this patch. There are basically infinite edge cases, so chasing them didn't seem worth it.

  1. Toggling a style off will make the cursor "disappear" due to the changed selection. This can be disconcerting, but functions fine. (the cursor is placed correctly, it's just highlighting a zero-width element, so it's invisible)
  1. Safari only: Zero width non-breaking spaces aren't actually zero width... they seem to have sub-pixel width. This means that some operations can make it appear as though the cursor is moving tiny amounts.
  1. If you toggle off a style and then your next action is to press the space bar followed by a character, the character will still have the style. Note that toggling on styles works just fine, it's the off + space combination which misbehaves.

I'm sure there are other unknown edge cases. These bother me, but I assessed them to be much nicer to live with than the original bug.

This code is live in the FogBugz wiki editor-- you can play with it there if you like, just create a free trial.

Hope this was helpful,

  • Jude

comment:24 Changed 7 years ago by Jude Allred

Someone with editing privileges, please correct my typo: s/causual/casual

Thanks!

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

#6914 is a dup.

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

#6933 is yet another dup.

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 1272.patch added

comment:27 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.5.1
Owner: set to Frederico Caldeira Knabben
Status: confirmedreview

I'm proposing a simple workaround solution for it, using the zero-width-space hack. It looks like it solves the problem with any noticeable drawback.

I'm tentatively targeting this one to the 3.5.1, even if it's already frozen for testing.

comment:28 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

TC1:

  1. Click to have cursor blinks inside document;
  2. Click on 'New Page' button;
  • Actual Result: Cursor disappears.

TC2:

  1. Put cursor at the end of one line of text;
  2. Press enter key to open new paragraph;
  3. Press backspace key;
  • Actual Result: Cursor not moved.

TC3:

  1. Press bold button on collapsed selection;
  2. Type one word;
  3. Press Undo button;
  • Actual Result: Cursor disappears.
  1. Now make cursor blinks by clicking back to cursor position;
  2. press backspace key;
  • Actual Result: Cursor not moved.

Changed 7 years ago by Garry Yao

Attachment: 1272_2.patch added

comment:29 Changed 7 years ago by Garry Yao

Propose another option that doesn't require the "zero-width-space" hack, with the following limitation which are considered to be less critical:

  1. Upon opening style, elements path bar are not updated;
  2. Unable to line-break or pasting content upon opening style without typing anything.

comment:30 Changed 7 years ago by Frederico Caldeira Knabben

Status: review_failedassigned

As the first patch has R-, I'm postponing it so we can work on this with more time.

After looking at patch 2, I would instead go for the simpler approach of patch 1.

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 1272_3.patch added

comment:31 Changed 7 years ago by Frederico Caldeira Knabben

I'm adding a new patch just to keep track on the development. It fixes Garry's TC2. Other are still pending.

I'm adding a new TC:

  1. Type a single char in the middle of some text.
  2. Undo once.

The selection will be wrong.

comment:32 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.5.1CKEditor 3.5.2

Ops... forgot to postpone it.

comment:33 Changed 6 years ago by James Cunningham

Cc: jamcunni@… added

Changed 6 years ago by Frederico Caldeira Knabben

Attachment: 1272_4.patch added

comment:34 Changed 6 years ago by Frederico Caldeira Knabben

Status: assignedreview

New patch that fixes all previous TCs.

Changed 6 years ago by Frederico Caldeira Knabben

Attachment: 1272_5.patch added

comment:35 Changed 6 years ago by Frederico Caldeira Knabben

The color buttons were still not working with the previous patch. The new one fixes it.

comment:36 Changed 6 years ago by Garry Yao

Status: reviewreview_failed

Quite close, check this TC:

  1. Blank the page with "New Page";
  2. Press "Bold" then "Undo" Actual Result: One new line added (filler created after bogus br).

Changed 6 years ago by Frederico Caldeira Knabben

Attachment: 1272_6.patch added

comment:37 Changed 6 years ago by Frederico Caldeira Knabben

Status: review_failedreview

That issue is due to a bug on CKEDITOR.dom.node#getAddress. Fixing it involved a small re-factoring of it and getIndex. Please review these changes with care.

comment:38 Changed 6 years ago by Garry Yao

Status: reviewreview_failed

One step further, while I found the following issues:

The filler node is not properly scoped (being scoped as global) which cause the following trouble:

  1. Open multiple instances (replacebycode)
  2. Bold on first, press tab to focus the next;
  3. Bold on second, press "Shift+Tab" to focus back;
    • Actual Result: Cursor stops blinking in first.

Clean on "Backspace" and "Arrow Left" sometimes not enough, check this case:

  1. Blank the editor, type "some" then "Shift-Enter";
  2. Bold then type "bold", press "Arrow Up" then "Arrow Right"
    • Actual Result: Cursor trapped by filler;

No filler cleanup on editor output (getData);

Filler should get created (for collapsed selection) only when necessary (parent empty inline element).

comment:39 in reply to:  38 Changed 6 years ago by Frederico Caldeira Knabben

Replying to garry.yao:

Filler should get created (for collapsed selection) only when necessary (parent empty inline element).

That was my original implementation, but I've met a case where the caret position changed. I can't recall the case though. I think it's related to style removal or maybe the ENTER key. It should not be a problem to leave it always on though, as long as it doesn't bring issues to the editor. (I know, it's a matter of reducing the risks)

Changed 6 years ago by Frederico Caldeira Knabben

Attachment: 1272_7.patch added

comment:40 Changed 6 years ago by Frederico Caldeira Knabben

Status: review_failedreview

The new patch changes things a little bit, so it's possible to handle more than one editor instance, keeping the implementation still relatively simple.

comment:41 Changed 6 years ago by Garry Yao

Keywords: CantFix HasPatch removed
Status: reviewreview_failed

Almost there! Note that we can't simply remove the originally chunk from selection::select ( if it's intended to be changed, it should be ported to selection::selectRanges as well).

comment:42 Changed 6 years ago by Garry Yao

Also note that a small issue that "contentDomUnload" could be safely used instead of introducing new "beforeSetMode".

Changed 6 years ago by Frederico Caldeira Knabben

Attachment: 1272_8.patch added

comment:43 Changed 6 years ago by Frederico Caldeira Knabben

Status: review_failedreview_passed

I've moved the missing bits of select to selectRanges with this new patch.

As for "contentDomUnload" it didn't work. This is the TC that "beforeSetMode" fixes:

  1. CTRL+B to start Bold.
  2. Do not type anything and switch to Source.
  3. There must not be an empty <strong></strong> in the source code (not really empty).

In any case, "beforeSetMode" didn't sound like a bad thing to have.

comment:44 Changed 6 years ago by Frederico Caldeira Knabben

Status: review_passedreview

Ops, small mistake when asking for review.

comment:45 Changed 6 years ago by Garry Yao

Status: reviewreview_passed

Be sure the following spots are addressed before celebration:

  1. #6577 is regressed;
  2. Var leaked at L1287.

comment:46 Changed 6 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

We did it! Fixed with [6461].

comment:47 in reply to:  45 Changed 6 years ago by Frederico Caldeira Knabben

Replying to garry.yao:

  1. #6577 is regressed;

In the final commit, I've added the same check that has been removed from select to selectRanges. I was not able to reproduce #6577 though, so I cannot confirm if it fixes anything. A clear TC file is missing on that ticket. It doesn't hurt anyway.

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