Bug 15256

Created attachment 16346 [details] TC

Sorry... small correction... for correctness. Today, the caret is actually placed "before" the <b></b> in the above example, not after it. So we are having the following result: Some |<b></b>text.

<rdar://problem/5610785>

This problem doesn't only occur for empty text nodes - even if the node is not empty, selection position is adjusted to start before its container. I think I have a workaround that might work. It involves zero-width space: [<b>&#x200b;</b>] ... range.setStart( el.firstChild, 1 ) ; range.setEnd( el.firstChild, 1 ) ; This positions the insertion point after the invisible space, and inside the text child node. Obviously, the space needs to be removed when the user starts typing, or clicks somewhere else - otherwise, arrow navigation will not work incorrectly, stopping at this position twice. Please let us know if this works for you.

(In reply to comment #4) > Obviously, the space needs to be removed when the user starts typing, or clicks > somewhere else - otherwise, arrow navigation will not work incorrectly, > stopping at this position twice. I've tried something similar before, but the above thing is the big problem. It is quite difficult to provide a stable cleanup mechanism in JavaScript, to remove the zero-width space text node. There are too many combinations to consider, like keyboard navigation, mouse click, API calls, clipboard pasting, etc... it would be easy to have zero-width space chars laying around, causing an undesired behavior. At the end, the cleanup code would be so huge that it would be overkill for a JS app. This is why we need the browser to handle it properly.

I tried to quickly fix this today, but the problem is much deeper than just the addRange() function - it's the underlying selection object that enforces "valid" positions. It's clear to me that it should be redesigned, but that may take a while.

Created attachment 17529 [details] automated test case (incomplete) This tests that ranges round-trip properly, but it is not enough to fix this bug - editing commands should avoid validating selections in some situations, too.

Is this a regression from Safari 2.0.4 or any released (non-beta) version since then?

This is not a regression - but it looks like a major limitation without a viable workaround.

Marking this bug as critical for FCKeditor.

See also: bug 6350.

See also: bug 21785, bug 20242, bug 20129.

See also bug #23189

What is happening with this bug?

Is there a reason this bug is now 2+ years old without action? Please target this for patching.

I would also like to request a redesign of the selection/caret behaviour, or e.g. to extend the default behaviour with CSS hints to allow you to control possible caret placements yourself. There are many cases where you would want to use inline elements, but still allow the caret to distinguish between being inside and outside the element in question.

Also calling for a fix. I've hit this bug many times, and have tried all sorts of hacks to get around it (e.g. appending text nodes on keypress, zero-width spaces etc).

Just FYI... three years passed, and this issue is still highly critical for CKEditor. Unfortunately we're not able to code a fix for it. Is there anything else we could do to have someone involved on fixing it?

We're opening a bounty for a definitive fix to this issue: https://lists.webkit.org/pipermail/webkit-dev/2010-September/014251.html I really hope we'll have success with it.

(In reply to comment #19) > We're opening a bounty for a definitive fix to this issue: > https://lists.webkit.org/pipermail/webkit-dev/2010-September/014251.html > > I really hope we'll have success with it. The problem with this bug is that resolving this bug requires our rewriting much of the existing editing code. And that isn't easy by no means. Could you explain how this bug is impacting CKEditor? It seems like you're trying to apply styles to a selection? I'd like to know more context of the problem you're encountering so that we might be able to come up with an alternative solution to your problem.

There is now a bounty on this bug: <https://lists.webkit.org/pipermail/webkit-dev/2010-September/014251.html>

(In reply to comment #20) > Could you explain how this bug is impacting CKEditor? It seems like you're trying to apply styles to a selection? That's exactly the problem. The detail is that we're talking about a "collapsed" selection. I mean, just have the caret blinking inside the editor, hit CTRL+B for Bold (bug: nothing is happening at this point) and type some text. There will be no style applied. Just to give you more details, here you have the code being executed: 1st part, this small if block. Note that even if we're using an internal range manipulation class, it works much like the DOM range implementation of WebKit: http://dev.ckeditor.com/browser/CKEditor/trunk/_source/plugins/styles/plugin.js?rev=5847#L360 2nd part - it's a bit more complicated, but basically we're transforming our internal range representation in a native WebKit range and selecting it: http://dev.ckeditor.com/browser/CKEditor/trunk/_source/plugins/selection/plugin.js?rev=5812#L1040 The attached TC reflects the above exactly.

(In reply to comment #22) > (In reply to comment #20) > > Could you explain how this bug is impacting CKEditor? It seems like you're trying to apply styles to a selection? > > That's exactly the problem. The detail is that we're talking about a "collapsed" selection. I mean, just have the caret blinking inside the editor, hit CTRL+B for Bold (bug: nothing is happening at this point) and type some text. There will be no style applied. Could you call document.execCommand('bold', false, null) instead of manually adding bold? We have typing style internally, and bold should be preserved when user starts typing. I bet other browsers would support this as well. We also have applyStyle command internally, which allows you to apply arbitrary CSS style on the selected region. It's used by bold, italic, etc... as well as for style preservation. We could expose either if that'll benefit you and other web developers.

By either I meant typing style and applyStyle command.

(In reply to comment #23) > Could you call document.execCommand('bold', false, null) Well, that's the way we have it in FCKeditor 1.0... we were just using the standard execCommand stuff. But, later we've found limitations in this approach. Let me give you more info. The very first issue is being able to produce the same output with all browsers. For the bold command, you have some browsers producing <b>, while others use <strong>. Depending on the setting, you can even have <span style="font-weight: bold;">, or the even worst <span class="Apple-style-span" style="font-weight: bold;">. Consider that we're talking just about the bold command, but almost all commands have different implementation among browsers. The other important factor is that CKEditor is extremely flexible, when considering styles. One can decide how to exactly apply styles. For bold, for example, you can easily choose to output <b>, <strong>, <span style="font-weight: bold;">, <span class="myBold">, or any other tag you want. So far, we were just talking about bold... but you must also consider custom styles, which doesn't have their execCommand equivalent. It has been always my opinion: execCommand is one of the worst things one could have for styling. It's badly designed and limited. It's a pity to see it's making its space in the HTML5 standards. The future doesn't look better because of it. --- > We also have applyStyle command internally, which allows you to apply arbitrary CSS style on the selected region. It's used by bold, italic, etc... as well as for style preservation. We could expose either if that'll benefit you and other web developers. Is this command that flexible, so you can apply a style that outputs not only <span style="..."> tags, but also things like <span class="xyz">, <b dir="rtl">, <font face="ABC">, etc.? In that case, it would be just perfect to workaround the problem, even if still not a real solution for the root of the problem.

I found a workaround, don't know if anybody found this before but instead of programmatically appending a zero-width space, you can do the same thing with the css3 content property in the after psuedo-selector of the elements you want to put the caret in. This has the advantage that the extra characters don't show up in the DOM and the user can't navigate the caret between it. So basically it doesn't need cleaning up. For this to work for any child element of your content editable element it would be something like this: #mycontenteditableelement *:after { content: '\200B'; } I didn't check completely, but I suspect this is a full workaround.

(In reply to comment #26) > #mycontenteditableelement *:after { > content: '\200B'; > } > > I didn't check completely, but I suspect this is a full workaround. I'll just comment that the support for after, before, first-letter, and other pseudo-selectors are poorly supported in the content editable area. In the worst case, WebKit crashes or reaches an inconsistent state. So I wouldn't recommend this.

(In reply to comment #27) > I'll just comment that the support for after, before, first-letter, and other pseudo-selectors are poorly supported in the content editable area. In the worst case, WebKit crashes or reaches an inconsistent state. So I wouldn't recommend this. Good to know. Do you or anybody else have any advice as to what to avoid specifically? In my limited use case (adding new <p>'s in response to keypress events and moving the caret inside the newly created <p>) I am seeing no unwanted side effects whatsoever with Safari 5.0.3.

While the (In reply to comment #28) > Good to know. Do you or anybody else have any advice as to what to avoid specifically? In my limited use case (adding new <p>'s in response to keypress events and moving the caret inside the newly created <p>) I am seeing no unwanted side effects whatsoever with Safari 5.0.3. Thanks for sharing your findings. While the provided hack didn't fix the issue in CKEditor (after a quick test, still to be investigated), I've noticed that it has broken the arrow navigation inside the editor. Basically, the RIGHT-ARROW key remained stuck when reaching the right boundary of any inline element (like a <strong>).

Any news on this. I keep running into this bug and variations of it. For example, it's impossible to put the caret at the beginning of ANY text node. This is quite a silly bug to have in WebKit for so long.

(In reply to comment #30) > Any news on this. I keep running into this bug and variations of it. For example, it's impossible to put the caret at the beginning of ANY text node. This is quite a silly bug to have in WebKit for so long. There's an active discussion on whatwg about VisiblePosition's canonicalization. Please respond on the thread if you're interested in this being fixed in the long term.

@Ryosuke Niwa - Do you have a link?

(In reply to comment #32) > @Ryosuke Niwa - Do you have a link? http://www.mail-archive.com/whatwg@lists.whatwg.org/msg27063.html

See also: bug 66630.

Any update on this?

*** Bug 10884 has been marked as a duplicate of this bug. ***

It is also, as far as I can tell, impossible to place the insertion point *between* elements. This makes a number of interesting programmatic uses of insertHTML impossible (unwrapping nodes, for example). For example, given an HTML block like this: <div contentEditable="true"><div id="scratchpad"></div></div> and code like this: document.getElementById("scratchpad").innerHTML = "<div id=\"foo\">blah</div><div id=\"bar\">blah</div>"; var sel = window.getSelection(); sel.removeAllRanges(); var range = document.createRange(); range.setStartAfter(document.getElementById("foo")); range.setEndAfter(document.getElementById("foo")); sel.addRange(range); document.execCommand("insertHTML", false, "<div id=\"baz\">-</div>"); One would expect this snippet to result in: <div id="foo">blah</div><div id="baz">-</div><div id="bar">blah</div> but instead, you get: <div id="foo">blah</div><div id="bar">-blah</div> I've tried every combination of set{Start|End}{After|Before|} that I can think of, and even things like setBaseAndExtent, modifying the selection object directly by extending it in either direction, etc. Nothing works.

Any progress on this bug? It is still reproducible in the JavaFX implementation of WebKit.

It seems that the same issue has been fixed in Blink - https://code.google.com/p/chromium/issues/detail?id=346613 Would it be possible to port it to Webkit? This issue requires from editor developers one of the most tricky hacks I've ever seen, so that would be a big relief if we could finally remove it.

Created attachment 238624 [details] Partial patch I tried my hand at merging in the diffs from Chrome/Blink. With these changes, WebKit seems to behave more like I'd expect when programmatically monkeying with the selection, but I haven't done extensive testing of it yet. Caveats: 1. The user interface behavior is still problematic. It's still impossible to click between element boundaries or at the beginning of elements. I hope to try to figure out what's going on there "real soon now". 2. I don't know your test system well enough to merge the changes to the test cases correctly. They might be close, or they might not be—I didn't bother running tests. 3. I'm not sure if you need to add or modify any copyright notices as a result of these changes. I'll leave that up to your discretion.

Note that I'll clean this patch up and resubmit in a day or two. The previous patch fixes only the programmatic behavior; the arrow key and mouse click caret behavior is still not quite right, and I'm trying to fix that while I'm at it.

Created attachment 238794 [details] A more complete patch Here's a more complete patch. This patch incorporates the Blink changes, plus additional tweaks to the visible position code and cursor rendering code so that cursors within empty elements show up in the right place, provided that the empty element has a ::before pseudoelement. (Note that if an element doesn't have both a ::before and ::after pseudoelement, it isn't visually distinct anyway, so although the selection gets stored and retrieved without modification, in practice, the cursor isn't really inside the element in any useful sense of the word.)

Unfortunately, I still can't place the caret between adjacent elements, so there's still at least one more subpart to this bug. I'll keep pounding on it. Also, there's a new block of code in RenderBox.cpp that needs to be nested under the enclosing if (box) to avoid a crash in the inspector. I'll update the patch with that fix after I get the caret-between-elements issue fixed.

Make that three more bugs left to fix in this patch: * Still can't put cursor between adjacent elements. * Crash if the user goes off the left edge of any line box. * Wraps around to the left if the user walks off the right end of the last line box in the editing range. *sigh*

Created attachment 238899 [details] Almost complete patch This patch fixes the aforementioned crasher bugs and adds the ability to place the editing cursor between adjacent inline elements. Still missing: placing editing cursor between adjacent block elements.

Created attachment 239395 [details] Final HTML editing caret patch Whew. I think I've fixed the last crasher bugs and caret positioning bugs (or at least the last layout bugs that I introduced). I also rewrote a bunch of the caret positioning code for block elements in a different way that's a lot cleaner. I've also updated it to be based on TOT as of a couple of days ago, which required inlining a bunch of recently removed macros. Hope this helps. If and when you take these fixes, you'll probably need to do two additional things: * Finish fixing up the text cases as needed. Although I attempted to merge the test case changes from Blink, I know nothing about how your test suite is designed (or theirs), so I had basically no idea what I was doing during that marge. As a result, the test case parts of this patch may be completely and utterly wrong. You have been warned. :-) * Check the original Blink patches to see if you need to introduce any new copyright lines into the modified files. I didn't notice any such additions in the failed patches as I applied them, but a second set of eyes couldn't hurt.

Created attachment 239396 [details] Slight cleanup to previous patch. Oops. I forgot to scan the previous patch for "#if 0" blocks and remove some junk code. Fixed.

Created attachment 239652 [details] Patch for one more bug This additional patch fixes an edge case that can cause crashes.

Created attachment 239657 [details] Patch to prevent deletion of empty element This patch prevents deletion of an empty element with visible boundaries after you delete its contents.

Created attachment 239665 [details] Rework of patch to prevent deletion of empty element Turns out that previous patch caused failures later, for two reasons: 1. In some cases, the editing code was inserting a placeholder <br> tag to avoid the element becoming empty, but that isn't needed or desirable if the editing boundaries are already distinct. 2. Deleting that <br> tag caused a crash because the merge code wasn't expecting to ever encounter a situation where such a node still existed, and tried to merge it with its parent. This fix corrects those additional bugs.

Created attachment 239689 [details] Fix for an additional case where cursor can't be placed after the last child element In VisiblePosition::canonicalPosition(), the code first checks to see if the upstream or downstream candidate is a match. However, in some circumstances, the upstream/downstream methods return a Position that is set to "after all children" rather than a Position whose numerical offset is actually after the last child. This patch to isCandidate() makes it correctly detect and handle those Position objects.

Created attachment 239746 [details] Fix for an occasional hang when walking off the left edge of certain block elements Walking off the left edge of a block element that contains *only* a single text node caused a hang, because obtaining the inline box and offset for the paragraph returned the node that we already visited, thus making it impossible to reach the position to the left of such an element. Worse, it resulted in an infinite loop. This patch shortcuts that infinite loop by checking to see if the position to the left of the element is a candidate right after checking to see if the position to the left of the text node is a candidate.

Created attachment 239747 [details] Update to occasional hang patch. The previous patch, "239746: Fix for an occasional hang when walking off the left edge of certain block elements", fixed a hang in leftVisuallyDistinctCandidate() when moving left from the left bound of an element containing only text. A similar problem (without the hang) occurs on the right end; specifically the cursor jumped from the right end of the text node to the left end. This updated version of that patch adds a similar fix to rightVisuallyDistinctCandidate().

@Piotrek Koszuliński: I lost 3 days working this bug around in my editor. It's incredibly hard to implement selection if you can't place cursor to an empty element or at its beginning : https://bugs.webkit.org/show_bug.cgi?id=23189 I had to explicitly place it to the prev EOL in zillion variations which showed up to be a super hard thing to do...

@Jakub Liska: I know. Workaround for this bug is a real killer. That's why I'm keeping fingers crossed for David. Hopefully, soon we'll be able to remove this hack and automatically fix many issues which it creates.

I haven't personally seen any new crashers or other misbehavior since the last set of patches I submitted, but I'm spending most of my time on unrelated projects, so that's no guarantee. :-) I'd encourage you to try out the attached patches and see how things behave for you. Shout in this bug if you find additional edge cases, crasher bugs, etc., and I'll take a look at them (if I can reproduce them). Two areas where I would not be too surprised to hear about bugs include right-to-left content and ::before/::after pseudoelements that contain images.

Created attachment 243323 [details] Adds proper highlighting for ::before and ::after pseudo-elements. This patch enables highlighting of ::before and ::after pseudo-elements when they appear within a selection. This patch includes (and thus supersedes) the previous patches. (The pile of patches was getting way too big.) This patch also reverts part of bug #32123 that caused massive over-selection or under-selection when a pseudo-element contained no text nodes. Note that these selection changes are still quite preliminary. They work for me when using limited test content, but have not been tested against large swaths of web content.

Created attachment 243610 [details] Patch for an edge case where command-right or command-left could cause a crash

Created attachment 243617 [details] Fix for another crashing edge case Note that for some bizarre reason, this latest patch breaks the WebKitLegacy build for me with an inability to find the "PseudoElement.h" header. I'm not exactly sure why such a critical WebCore header would be missing in that framework's build environment, and the problem is probably specific to my machine's rather broken build configuration. Either way, I have no need to build that framework for my purposes, so I'm going to ignore it for the time being.

I am able to reproduce partially this bug where in Safari 15.5 and Chrome Canary 104, the caret is inside [] but if I type something, it is not bold. In Firefox Nightly 103, it is doing as specified of expected result of test case (inside [] and typing bold). If it is not intended expected results or require spec change, please do accordingly. If my testing is flawed, please retest accordingly. Thanks!

I am able to reproduce partially this bug where in Safari 15.5 and Chrome Canary 104, the caret is inside [] but if I type something, it is not bold. In Firefox Nightly 103, it is doing as specified of expected result of test case (inside [] and typing bold). If it is not intended expected results or require spec change, please do accordingly. If my testing is flawed, please retest accordingly. Thanks!

It seems like this *still* isn't fixed. I'm making a wysiwyg editor, and not having to hack around this would be useful.

Pull request: https://github.com/WebKit/WebKit/pull/10503

I know the underlying issues has not been fixed, but I see posters trying to create an editor. I have found a way to get around this issue in this use case. The trick is to reverse the sequence of operations. It is tempting, when a user clicks the "bold" button, to create a <b> element then allow the user to type into it. Instead, just a set a flag to indicate "bold" mode. Then intercept the keydown event. If the bold flag is set, insert the <b> tag along with the first keystroke into the document and place the cursor on it. Then kill the bold flag. You will succeed in setting the cursor because the <b> element is not empty. Hook the onselectionchange event to knock down the flag. Note this is not a hack - it is actually a better way to do it IMHO. If the user clicks "bold" and then moves the selection, there will be no orphan <b> element in the document to try and remove.

Unfortunately, my use case is for an XML editor, and that would result in a document that doesn't conform to the DTD because of missing element nesting if you save prior to hitting a key, not to mention that it would would be really confusing for a button to do nothing, and then for the entire layout of parts of the page to change when you hit the next key because the newly created element is, for example, centered left to right.

I tested this bug in Safari 26.2 with the following code with document.designMode = "on"; in the console and it behaves correctly with Some<b>|</b>text as a result of clicking the button. <!doctype html> <html> <body> <p id="p">Some <b id="b"></b>text.</p> <button onclick="placeCaret()">Place caret inside &lt;b&gt;</button> <script> function placeCaret() { const b = document.getElementById("b"); const range = document.createRange(); const sel = window.getSelection(); range.setStart(b, 0); range.setEnd(b, 0); sel.removeAllRanges(); sel.addRange(range); } </script> </body> </html>

The text is not bold and should be.