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

[Frederico Caldeira Knabben]

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.

[Frederico Caldeira Knabben]

WebKit bug for it:

​http://bugs.webkit.org/show_bug.cgi?id=15256

[Frederico Caldeira Knabben]

#2553 has been marked as DUP

[Frederico Caldeira Knabben]

#1939 has been marked as DUP

[Frederico Caldeira Knabben]

#3708 has been marked as DUP

[Frederico Caldeira Knabben]

#4071 has been marked as DUP

[Frederico Caldeira Knabben]

#4070 has been marked as DUP

[Frederico Caldeira Knabben]

#4747 has been marked as DUP

[Frederico Caldeira Knabben]

#4807 has been marked as DUP.

[Damian]

What version of CKEditor is this fix targeted for?

[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.

[Alfonso Martínez de Lizarrondo]

#5331 has been marked as dup

[Frederico Caldeira Knabben]

#5929 has been marked as DUP.

[Alfonso Martínez de Lizarrondo]

#5982 has been marked as dup

[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.

[Alfonso Martínez de Lizarrondo]

#1272 has been marked as dup

[Jude Allred]

A 90% fix for this bug

[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.

[Alfonso Martínez de Lizarrondo]

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 :-)

[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)

2. 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.

3. 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

[Jude Allred]

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

Thanks!

[Sa'ar Zac Elias]

#6914 is a dup.

[Sa'ar Zac Elias]

#6933 is yet another dup.

[Frederico Caldeira Knabben]

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.

[Garry Yao]

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.

[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.

[Frederico Caldeira Knabben]

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.

[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.

[Frederico Caldeira Knabben]

Ops... forgot to postpone it.

[Frederico Caldeira Knabben]

New patch that fixes all previous TCs.

[Frederico Caldeira Knabben]

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

[Garry Yao]

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).

[Frederico Caldeira Knabben]

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.

[Garry Yao]

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).

[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)

[Frederico Caldeira Knabben]

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.

[Garry Yao]

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).

[Garry Yao]

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

[Frederico Caldeira Knabben]

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.

[Frederico Caldeira Knabben]

Ops, small mistake when asking for review.

[Garry Yao]

Be sure the following spots are addressed before celebration:

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

[Frederico Caldeira Knabben]

We did it! Fixed with [6461].

[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.