Ticket #1272 (closed Bug: fixed)

Opened 7 years ago

Last modified 3 years ago

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

Reported by: fredck Owned by: fredck
Priority: High Milestone: CKEditor 3.5.3
Component: Core : Styles Version: SVN (FCKeditor) - Retired
Keywords: IBM WebKit Cc: satya, damo, 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

webkitFix.patch (5.4 KB) - added by Jude Allred 3 years ago.
A 90% fix for this bug
1272.patch (4.8 KB) - added by fredck 3 years ago.
1272_2.patch (10.2 KB) - added by garry.yao 3 years ago.
1272_3.patch (4.9 KB) - added by fredck 3 years ago.
1272_4.patch (6.5 KB) - added by fredck 3 years ago.
1272_5.patch (6.6 KB) - added by fredck 3 years ago.
1272_6.patch (10.4 KB) - added by fredck 3 years ago.
1272_7.patch (10.3 KB) - added by fredck 3 years ago.
1272_8.patch (10.9 KB) - added by fredck 3 years ago.

Change History

comment:1 Changed 7 years ago by fredck

  • Priority changed from Normal to High

comment:2 Changed 7 years ago by fredck

comment:3 Changed 6 years ago by w.olchawa

  • Keywords Confirmed added

comment:4 Changed 6 years ago by fredck

#2553 has been marked as DUP

comment:5 Changed 5 years ago by fredck

#1939 has been marked as DUP

comment:6 Changed 5 years ago by fredck

#3708 has been marked as DUP

comment:7 Changed 5 years ago by fredck

#4071 has been marked as DUP

comment:8 Changed 5 years ago by fredck

#4070 has been marked as DUP

comment:9 Changed 5 years ago by fredck

  • Keywords WebKit added

comment:10 Changed 4 years ago by fredck

#4747 has been marked as DUP

comment:11 Changed 4 years ago by fredck

#4807 has been marked as DUP.

comment:12 follow-up: ↓ 14 Changed 4 years ago by damo

What version of CKEditor is this fix targeted for?

comment:13 Changed 4 years ago by damo

  • Keywords IBM added

comment:14 in reply to: ↑ 12 Changed 4 years ago by fredck

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 4 years ago by alfonsoml

  • Cc satya, damo, joek added

#5331 has been marked as dup

comment:16 Changed 4 years ago by fredck

#5929 has been marked as DUP.

comment:17 Changed 4 years ago by alfonsoml

  • Cc Jude, Allred added

#5982 has been marked as dup

comment:18 Changed 4 years ago by fredck

  • Keywords CantFix added

comment:19 Changed 4 years ago by fredck

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 3 years ago by alfonsoml

  • Cc lnagpal@… added

#1272 has been marked as dup

Changed 3 years ago by Jude Allred

A 90% fix for this bug

comment:21 Changed 3 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 3 years ago by alfonsoml

  • 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 3 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 3 years ago by Jude Allred

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

Thanks!

comment:25 Changed 3 years ago by Saare

#6914 is a dup.

comment:26 Changed 3 years ago by Saare

#6933 is yet another dup.

Changed 3 years ago by fredck

comment:27 Changed 3 years ago by fredck

  • Status changed from confirmed to review
  • Owner set to fredck
  • Milestone set to CKEditor 3.5.1

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 3 years ago by garry.yao

  • Status changed from review to review_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 3 years ago by garry.yao

comment:29 Changed 3 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 3 years ago by fredck

  • Status changed from review_failed to assigned

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 3 years ago by fredck

comment:31 Changed 3 years ago by fredck

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 3 years ago by fredck

  • Milestone changed from CKEditor 3.5.1 to CKEditor 3.5.2

Ops... forgot to postpone it.

comment:33 Changed 3 years ago by jamescun

  • Cc jamcunni@… added

Changed 3 years ago by fredck

comment:34 Changed 3 years ago by fredck

  • Status changed from assigned to review

New patch that fixes all previous TCs.

Changed 3 years ago by fredck

comment:35 Changed 3 years ago by fredck

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

comment:36 Changed 3 years ago by garry.yao

  • Status changed from review to review_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 3 years ago by fredck

comment:37 Changed 3 years ago by fredck

  • Status changed from review_failed to review

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 follow-up: ↓ 39 Changed 3 years ago by garry.yao

  • Status changed from review to review_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 3 years ago by fredck

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 3 years ago by fredck

comment:40 Changed 3 years ago by fredck

  • Status changed from review_failed to review

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 3 years ago by garry.yao

  • Status changed from review to review_failed
  • Keywords CantFix HasPatch removed

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 3 years ago by garry.yao

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

Changed 3 years ago by fredck

comment:43 Changed 3 years ago by fredck

  • Status changed from review_failed to review_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 3 years ago by fredck

  • Status changed from review_passed to review

Ops, small mistake when asking for review.

comment:45 follow-up: ↓ 47 Changed 3 years ago by garry.yao

  • Status changed from review to review_passed

Be sure the following spots are addressed before celebration:

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

comment:46 Changed 3 years ago by fredck

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

We did it! Fixed with [6461].

comment:47 in reply to: ↑ 45 Changed 3 years ago by fredck

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 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy