#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)
Change History (56)
comment:1 Changed 17 years ago by
Priority: | Normal → High |
---|
comment:2 Changed 17 years ago by
comment:3 Changed 17 years ago by
Keywords: | Confirmed added |
---|
comment:9 Changed 15 years ago by
Keywords: | WebKit added |
---|
comment:13 Changed 15 years ago by
Keywords: | IBM added |
---|
comment:14 Changed 15 years ago by
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 14 years ago by
Cc: | Satya Minnekanti Damian joek added |
---|
#5331 has been marked as dup
comment:18 Changed 14 years ago by
Keywords: | CantFix added |
---|
comment:19 Changed 14 years ago by
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:21 Changed 14 years ago by
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 14 years ago by
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 14 years ago by
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.
- 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)
- 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.
- 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 14 years ago by
Someone with editing privileges, please correct my typo: s/causual/casual
Thanks!
Changed 14 years ago by
Attachment: | 1272.patch added |
---|
comment:27 Changed 14 years ago by
Milestone: | → CKEditor 3.5.1 |
---|---|
Owner: | set to Frederico Caldeira Knabben |
Status: | confirmed → review |
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 14 years ago by
Status: | review → review_failed |
---|
TC1:
- Click to have cursor blinks inside document;
- Click on 'New Page' button;
- Actual Result: Cursor disappears.
TC2:
- Put cursor at the end of one line of text;
- Press enter key to open new paragraph;
- Press backspace key;
- Actual Result: Cursor not moved.
TC3:
- Press bold button on collapsed selection;
- Type one word;
- Press Undo button;
- Actual Result: Cursor disappears.
- Now make cursor blinks by clicking back to cursor position;
- press backspace key;
- Actual Result: Cursor not moved.
Changed 14 years ago by
Attachment: | 1272_2.patch added |
---|
comment:29 Changed 14 years ago by
Propose another option that doesn't require the "zero-width-space" hack, with the following limitation which are considered to be less critical:
- Upon opening style, elements path bar are not updated;
- Unable to line-break or pasting content upon opening style without typing anything.
comment:30 Changed 14 years ago by
Status: | review_failed → 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 14 years ago by
Attachment: | 1272_3.patch added |
---|
comment:31 Changed 14 years ago by
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:
- Type a single char in the middle of some text.
- Undo once.
The selection will be wrong.
comment:32 Changed 14 years ago by
Milestone: | CKEditor 3.5.1 → CKEditor 3.5.2 |
---|
Ops... forgot to postpone it.
comment:33 Changed 14 years ago by
Cc: | jamcunni@… added |
---|
Changed 14 years ago by
Attachment: | 1272_4.patch added |
---|
Changed 14 years ago by
Attachment: | 1272_5.patch added |
---|
comment:35 Changed 14 years ago by
The color buttons were still not working with the previous patch. The new one fixes it.
comment:36 Changed 14 years ago by
Status: | review → review_failed |
---|
Quite close, check this TC:
- Blank the page with "New Page";
- Press "Bold" then "Undo" Actual Result: One new line added (filler created after bogus br).
Changed 14 years ago by
Attachment: | 1272_6.patch added |
---|
comment:37 Changed 14 years ago by
Status: | review_failed → 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 14 years ago by
Status: | review → 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:
- Open multiple instances (replacebycode)
- Bold on first, press tab to focus the next;
- 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:
- Blank the editor, type "some" then "Shift-Enter";
- 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 Changed 14 years ago by
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 14 years ago by
Attachment: | 1272_7.patch added |
---|
comment:40 Changed 14 years ago by
Status: | review_failed → 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 14 years ago by
Keywords: | CantFix HasPatch removed |
---|---|
Status: | review → review_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 14 years ago by
Also note that a small issue that "contentDomUnload" could be safely used instead of introducing new "beforeSetMode".
Changed 14 years ago by
Attachment: | 1272_8.patch added |
---|
comment:43 Changed 14 years ago by
Status: | review_failed → 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:
- CTRL+B to start Bold.
- Do not type anything and switch to Source.
- 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 14 years ago by
Status: | review_passed → review |
---|
Ops, small mistake when asking for review.
comment:45 follow-up: 47 Changed 14 years ago by
Status: | review → review_passed |
---|
Be sure the following spots are addressed before celebration:
- #6577 is regressed;
- Var leaked at L1287.
comment:46 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
We did it! Fixed with [6461].
WebKit bug for it:
http://bugs.webkit.org/show_bug.cgi?id=15256