Open Bug 904846 Opened 11 years ago Updated 17 hours ago

Caret positioning is incorrect in an empty contenteditable element with ::before pseudo

Categories

(Core :: Layout: Block and Inline, defect, P2)

Product:
Core
Component:
Layout: Block and Inline
Version:
23 Branch
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
ASSIGNED
Project Flags:
Webcompat Priority P2
Tracking Flags:
Tracking Status
firefox61 --- wontfix
firefox62 --- fix-optional
firefox63 --- affected

People

(Reporter: andyearnshaw, Assigned: emilio)

References

Details

Attachments

(6 files, 3 obsolete files)

contenteditable-before-focus.html
413 bytes, text/html
Details
fix v2
6.36 KB, patch
MatsPalmgren_bugz
: review+
dbaron
: review+
Details | Diff | Splinter Review
Caret position affected by the presence of a ::before pseudo element
7.49 KB, image/png
Details
Test-case that still repros the issue.
363 bytes, text/html
Details
WIP: Bug 904846 - Fix baseline of empty text nodes.
48 bytes, text/x-phabricator-request
Details | Review
WIP: Bug 904846 - Fix baseline of empty text nodes.
48 bytes, text/x-phabricator-request
Details | Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1588.0 Safari/537.36

Steps to reproduce:

Create a single element, with the contenteditable attribute set to "true".  Add a ::before pseudo-element rule for the element, with a character in the content property value or use a fixed width.  In the rendered page, focus the editable element, paying attention to the position of the caret before typing the first character.


Actual results:

The caret is initially positioned to the leftmost point of the element.  When the first character is typed, the position of the character will be placed after the pseudo element, causing the caret to jump to the correct position.

Note that Chrome 30 dev (perhaps WebKit) also suffers from this problem, but Opera positions the caret perfectly.


Expected results:

The caret should initially be positioned after the pseudo-element.  Pseudo-elements aren't an editable part of the content and so it is confusing to see the caret positioned before the pseudo element when any typing places content after the pseudo element.

Once the element has content, it's not possible to get the caret back to its original (incorrect) position.
Attachment #789822 - Attachment mime type: text/plain → text/html
Confirmed with 2013-08-19-03-02-05-mozilla-central-firefox-26.0a1.en-US.linux-x86_64
Component: Untriaged → Editor
Product: Firefox → Core
I've confirmed with "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0"

Here's a JSFiddle: http://jsfiddle.net/rgthree/zTj8q/
(In reply to rgthree from comment #2)
> I've confirmed with "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0)
> Gecko/20100101 Firefox/23.0"
> 
> Here's a JSFiddle: http://jsfiddle.net/rgthree/zTj8q/

Actually, my issue in the jsfiddle linked above is very similar, except when the pseudo content (::before or ::after) is positioned absolutely, the caret is ~10px above the contenteditable until the user starts typing. I'm going to assume it's the same bug as here.
(In reply to comment #3)
> (In reply to rgthree from comment #2)
> > I've confirmed with "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0)
> > Gecko/20100101 Firefox/23.0"
> > 
> > Here's a JSFiddle: http://jsfiddle.net/rgthree/zTj8q/
> 
> Actually, my issue in the jsfiddle linked above is very similar, except when
> the pseudo content (::before or ::after) is positioned absolutely, the caret is
> ~10px above the contenteditable until the user starts typing. I'm going to
> assume it's the same bug as here.

In general we don't handle generated content (such as ::before and ::after) very well at all.
Here’s one more JSFiddle that demonstrates this same issue along with a very closely related issue regarding the use of [contenteditable] with a ::before pseudoelement:

http://jsfiddle.net/m5zDU/

Follow the instructions there and you’ll see that it’s pretty easy to accidentally move the input caret into and within the pseudoelement’s text.

Happy to file it as a separate issue... just let me know.
How is this still unconfirmed? Almost a year now...
I've had numerous problems with the content editable divs, with both chrome and firefox, but at least chrome has finally fixed this issue.
Hi, we see problems with this bug as well. You can look at our live demos at http://www.cortical.io/context-browser.html Does anybody here have a work around, short of getting a real fix?

Screenshot: https://www.evernote.com/shard/s128/sh/79590667-2fea-4929-a874-8345a8eaaaff/615fa9af0042929aa3c46ce1b99374b9/deep/0/safari-firefox-difference.png
Status: UNCONFIRMED → NEW
Ever confirmed: true
This isn't an editor issue.  This is a layout problem.  The 'inline-block' bit is key to this.  If you change it to just inline the testcase works.
Component: Editor → Layout: Block and Inline
Attached patch (Probably wrong) Patch (obsolete) — — Splinter Review 
The issue here is that nsBlockFrame passes through calls to GetCaretBaseline to its children, while nsInlineFrame does not.  If you use inline you get an nsInlineFrame, but if you use inline-block you get an nsBlockFrame.  The underlying text frame has not been reflowed so it doesn't have any metric yet, so if we pass the call through to the text frame we get a bogus baseline value.  Making nsBlockFrame not pass the call through if the line is inline fixes this issue, but I don't claim it's the correct fix.

Some guidance would be appreciated here.  I'm a long way from home.
Attachment #8481880 - Flags: feedback?(ehsan.akhgari)
Attachment #8481880 - Flags: feedback?(bzbarsky)
Comment on attachment 8481880 [details] [diff] [review]
(Probably wrong) Patch

Review of attachment 8481880 [details] [diff] [review]:
-----------------------------------------------------------------

I think the right thing to do would be to pass this down to the child even if it's an inline frame.  The problem might be the fact that the text frame doesn't have the right metrics.  You said something about it not having been reflown?  That's totally unexpected, I think nsCaret should not call GetCaretBaseline on any frame before a reflow has been performed.  Is that what's really happening?

Note that I would expect the reflow to happen, and since this is an empty text frame I would expect this code <http://hg.mozilla.org/mozilla-central/annotate/c360f3d1c00d/layout/generic/nsTextFrame.cpp#l8037> to compute the correct metrics for the frame.  Does that not happen here?
Attachment #8481880 - Flags: feedback?(ehsan.akhgari) → feedback-
Comment on attachment 8481880 [details] [diff] [review]
(Probably wrong) Patch

So I poked at this a little more and we're bailing out of nsTextFrame::ReflowText at http://hg.mozilla.org/mozilla-central/annotate/c360f3d1c00d/layout/generic/nsTextFrame.cpp#l7799 because there's no text.  That means we never get to http://hg.mozilla.org/mozilla-central/annotate/c360f3d1c00d/layout/generic/nsTextFrame.cpp#l8036 where we get the ascent data we need for GetCaretBaseline.
Attachment #8481880 - Attachment is obsolete: true
Attachment #8481880 - Flags: feedback?(bzbarsky)
Flags: needinfo?(roc)
Right, I agree. We need to add some code to ClearMetrics that obtains a real ascent. It would basically duplicate the code from PropertyProvider::InitFontGroupAndFontMetrics to get the nsFontMetrics.
Flags: needinfo?(roc)
I'll take this.
Assignee: nobody → roc
Fixing the textframe metrics gets the vertical position right but that isn't enough.

The other part of the bug is that the horizontal position is wrong. Since the block has no normal content to associate the caret with, we set the caret to be associated with offset 0 in the <div>'s block frame. The horizontal position of the caret is then computed by nsFrame::GetPointFromOffset, which just returns the left edge of the frame's content-rect. What we really want here is the position on the block's first line after any ::before content.

But that only works if the ::before content is inline outside. If it was display:block then we'd want the caret position to be below the ::before content, on an empty line that doesn't otherwise exist.

So there's actually a large can of worms here to fix this bug properly. In fact it's not at all clear what do in cases like
  <style>div::before { content"hello"; display:block; background:yellow; }</style>
  <div style="border:2px solid black"></div>
The intuitive idea "draw the caret where text would be inserted" doesn't work in cases like that where there's nowhere for the caret to go. We either have to change the layout to make room for the caret or draw it somewhere that looks broken.
Comment on attachment 8483223 [details] [diff] [review]
fix vertical position

Review of attachment 8483223 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine for the vertical metrics... are you going to try and do something about horizontal position here as well, or spin off a separate bug for that? As you note, there are some examples where the "right" answer is not at all clear, but it'd be nice to fix the inline-::before case, at least.
Attachment #8483223 - Flags: review?(jfkthame) → review+
I'm not going to do anything about the horizontal metrics for now. The patch here at least makes us behave the same as Chrome, which is a definite improvement. I'm unenthusiastic about trying to fix the inline case on its own without a broader plan.
Fair enough, but let's have a new bug on file for that, then.
Attached patch fix v2 — — Splinter Review 
To fix test regressions, we have to change nsLineLayout::IsZeroBSize to ignore collapsed-away text frames with non-zero height.

There's also a test that checks the height of an empty text node via getClientRects. I think changing this test is OK; I'm not aware of any spec that says the height must be zero.
Attachment #8483223 - Attachment is obsolete: true
Attachment #8486313 - Flags: review?(mats)
Comment on attachment 8486313 [details] [diff] [review]
fix v2

LGTM, but dbaron should probably have a look as well.
Attachment #8486313 - Flags: review?(mats)
Attachment #8486313 - Flags: review?(dbaron)
Attachment #8486313 - Flags: review+
Comment on attachment 8486313 [details] [diff] [review]
fix v2

I think this will change the quirks mode inline box model handling when line-height is normal, for cases like:

<p><span style="background:yellow"><span style="font-size: 80%">hello </span> </span></p>

(In quirks mode, the yellow background should be smaller than it is in standards mode.  And note the space before both </span>s.)

Looking at the code from:
https://hg.mozilla.org/mozilla-central/file/30920bcb070a/layout/generic/nsLineLayout.cpp#l2016

I think we'll hit canUpdate = true (because vertical-align is not top or bottom, and line-height is normal) and then hit:

  2044         if (blockStart < minBCoord) minBCoord = blockStart;
  2045         if (blockEnd > maxBCoord) maxBCoord = blockEnd;

This, in turn, will change the inline sizing adjustment further down.


If my theory that the patch breaks that case is correct, could you:
 * add a reftest for that case, and
 * fix it by making lines 2044-2045 (quoted above) conditional on not being an empty text frame (just like your check in IsZeroBSize

r=dbaron with that
Attachment #8486313 - Flags: review?(dbaron) → review+
I don't think that theory is correct. For the whitespace-only textframe (or a completely empty text frame in its place, if you adjust the test that way), canUpdate will not be set, because PFD_ISNONWHITESPACETEXTFRAME will not be set, because TEXT_HAS_NONCOLLAPSED_CHARACTERS will not be set along any path that calls nsTextFrame::ClearMetrics. Since canUpdate is not set.

Do you agree?
Flags: needinfo?(dbaron)
Ah, right; I think I'd misread the conditions around setting canUpdate.  Sounds good.
Flags: needinfo?(dbaron)
ready to land?
Flags: needinfo?(roc)
Probably, I just have a list of higher-priority stuff to deal with.
Flags: needinfo?(roc)
Any update on landing the resolution to Firefox ? (last message was 8 months ago)
Flags: needinfo?(roc)
There were test failures last time I ran this through try.
Flags: needinfo?(roc)
focus in contentEditable empty inline element,  cursor position error
5 years later and this bug persists.
use this function with index = 0

`
function moveCaretTo(element, index) {
   const range = document.createRange();
   const selection = window.getSelection();

   range.setStart(element, index);
   range.collapse(true);
   selection.removeAllRanges();
   selection.addRange(range);
}
`
Interesting that selection works and range doesn't. This allowed me to figure out what's going on. Apparently the startNode seems to be off by 1, falling into the node following the contenteditable. The workaround is to go to the previous element and re-focus.

Proof of concept showcasing both the bug and workaround:
https://codepen.io/cheng-lee/pen/LBVvOv
David, do you want to take a look or maybe find someone who could fix this in 63? It's clearly been around a while, but now we have this suggestion for a workaround.
status-firefox61: --- → wontfix
status-firefox62: --- → fix-optional
status-firefox63: --- → affected
Flags: needinfo?(dbaron)
While it would be good to get this landed, I don't see any reason to prioritize it right now.
Flags: needinfo?(dbaron)
I also don't see why the existence of a workaround increases the bug's priority.
It should be prioritized right now because it's affecting a lot of my work, and the work of others, and the existence of a hacky javascript "workaround" is not acceptable.  We use contenteditable divs in a lot of our projects, including our main control panel (Froala Editor).
The workaround doesn't work.
My caretproblems in firefox was gone as I added a white-space: pre-wrap to my contenteditable element.
@kontakt I can confirm this workaround works in Firefox v63 (64bit, Windows 7 Pro)

I used this CSS to only affect white space wrapping on *empty contenteditable elements:

[contenteditable="true"]:empty {
    white-space: pre-wrap;
}
See Also: → 1510942
HTML:

```
<div contenteditable="true" placeholder="Enter text here here here here here here here here..."></div>
```

CSS:

```
[contenteditable=true]:empty::before{
  content: attr(placeholder);
  display: block;
  overflow: hidden;
  text-overflow: ellipsis;
  white-space: nowrap;
}

div[contenteditable=true] {
  border: 1px dashed #AAA;
  width: 290px;
  padding: 5px;
  white-space: pre-wrap;
}
```

https://codepen.io/anon/pen/BvNpYw
I was hoping that the workaround would work for my use case (simulating an _ellipsis'd_ `::placeholder` in a contenteditable `<div>`) but it did not. (https://bugzilla.mozilla.org/attachment.cgi?id=9030512)

HTML:

```
<div contenteditable="true" placeholder="Enter text here here here here here here here here..."></div>
```

CSS:

```
[contenteditable=true]:empty::before{
  content: attr(placeholder);
  display: block;
  overflow: hidden;
  text-overflow: ellipsis;
  white-space: nowrap;
}

div[contenteditable=true] {
  border: 1px dashed #AAA;
  width: 290px;
  padding: 5px;
  white-space: pre-wrap;
}
```



https://codepen.io/anon/pen/BvNpYw

@Steven Luscher I found a work using display: flex on the contenteditable element:

https://codepen.io/anon/pen/omqGgL

It may have some side-effects though which I haven't thought about yet.

So on webcompat we got this testcase.
https://jsfiddle.net/7r13ebp8/

1. Open link and click on text field.

2. See where caret is positioned.

In Chrome, it is at the start of placeholder text, on the same line.
In Firefox, it is above, in top-left corner. But when you start typing something, your text appears on the same line. Or if you remove placeholder text, then caret is on the same line too.

The interop stories is not great, but there's no unique scenario for sure. I will repeat my comment from the webcompat issue.
https://github.com/webcompat/web-bugs/issues/48056#issuecomment-590129491

And this is in Safari… which has the same behavior than firefox.

So it's not obvious there is a webcompat issue.

but we could argue that it is more logical to put the caret at the beginning of the placeholder.

When we start typing the line, safari has also a caret which goes from the top of the box to the baseline, while firefox and chrome have a caret which is has big has the characters.

Another difference is if you remove the text after typing it.

  • In firefox, the area stays blank with small cursor blinking at the start of the line. If unfocused the line stays blank, caret disappears.
  • In chrome, the caret blinks, and the placeholder text reappears, when unfocused, just the caret disappears.
  • in safari, same thing than firefox, but with a caret from top of box to the baseline.

While I'm looking at this code... This fixes the case described in
comment 46.

Assignee: nobody → emilio
Status: NEW → ASSIGNED
Depends on: 1634743

Comment on attachment 9145036 [details]
Bug 904846 - Fix caret baseline on a line with only inlines to account for line-height properly. r=jfkthame

Revision D73465 was moved to bug 1634743. Setting attachment 9145036 [details] to obsolete.

Attachment #9145036 - Attachment is obsolete: true
Webcompat Priority: --- → ?
Webcompat Priority: ? → P3
Severity: normal → S3

The severity field for this bug is relatively low, S3. However, the bug has 18 votes.
:emilio, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(emilio)

S3 is probably fair here.

Flags: needinfo?(emilio)

(In reply to Steven Luscher from comment #44)

I was hoping that the workaround would work for my use case (simulating an
ellipsis'd ::placeholder in a contenteditable <div>) but it did not.
(https://bugzilla.mozilla.org/attachment.cgi?id=9030512)

HTML:

<div contenteditable="true" placeholder="Enter text here here here here here
here here here..."></div>

CSS:

[contenteditable=true]:empty::before{
  content: attr(placeholder);
  display: block;
  overflow: hidden;
  text-overflow: ellipsis;
  white-space: nowrap;
}

div[contenteditable=true] {
  border: 1px dashed #AAA;
  width: 290px;
  padding: 5px;
  white-space: pre-wrap;
}

https://codepen.io/anon/pen/BvNpYw

FWIW, changing overflow:hidden to overflow:clip will fix the caret position - tho its not clear why, according to the mdn doc:

Specifying a value other than visible (the default) or clip for overflow creates a new block formatting context. This is necessary for technical reasons; if a float intersects with a scrolling element, it would forcibly rewrap the content after each scroll step, leading to a slow scrolling experience.

I would imagine it might be related to the formatting context somehow...

As we don't know of any actual sites being affected by this, only codepens, I'm going to remove the webcompat priority for now.

Webcompat Priority: P3 → ---

(In reply to Thomas Wisniewski [:twisniewski] from comment #52)

As we don't know of any actual sites being affected by this, only codepens, I'm going to remove the webcompat priority for now.

I've spent the last couple of days baffled about a particular bug in a private project that is causing content written inside a ContentEditable code block to be written in reverse.

By pure luck this particular 11 year old bug report was bumped up 2 days ago and I just realised this is the root cause of my issue as it makes the caret get stuck on the leftmost position which causes the input to be reversed.

Imagine my dissapointment when i realised this issue (which is now so old that you could register it to middle school) was only bumped up because its priority has been removed. I implore you to please take cases similar to mine and number of votes on this bug report to consideration and re-evaluate the severity and priority of this issue.

Input text being reversed because of this bug.

(In reply to Thomas Wisniewski [:twisniewski] from comment #52)

As we don't know of any actual sites being affected by this, only codepens, I'm going to remove the webcompat priority for now.

Tons of websites/web-apps affected. Every contentEditable based editor is affected, and there are so many web components relying on contentEditable "wysiwyg"-like abilities. My own famous Tagify component which is used by tons of people is affected as well: https://github.com/yairEO/tagify

Many SaaS projects are behind a pay-wall and or are aimed for B2B only and are heavily relied on complex web components (unlike simple text-based websites) and this bug probably affect quite of the SaaS products (I've worked on many by many clients of mine)

Thanks for the insight folks, it's exactly what we need to know to help prioritize these kinds of bug fixes. I'll bump the priority back up now that we know it's definitely causing more pain than we were aware of.

Emilio, would you mind taking another look at this?

Flags: needinfo?(emilio)
Priority: -- → P2
Webcompat Priority: --- → P2

(In reply to Ata Hakcil from comment #53)

By pure luck this particular 11 year old bug report was bumped up 2 days ago and I just realised this is the root cause of my issue as it makes the caret get stuck on the leftmost position which causes the input to be reversed.

WDYM with "causes the input to be reversed"? This is only about the caret position. If you have a repro for that I'm happy to poke.

In any case I think the right thing to do is looking at whether https://hg.mozilla.org/try/rev/6716fc2c18434337eb6c153a380c9ffccc6b6986 still fixes the issue, and whether we can get it landed.

Thanks for updating the priority!

(In reply to Emilio Cobos Álvarez (:emilio) from comment #56)

(In reply to Ata Hakcil from comment #53)

By pure luck this particular 11 year old bug report was bumped up 2 days ago and I just realised this is the root cause of my issue as it makes the caret get stuck on the leftmost position which causes the input to be reversed.

WDYM with "causes the input to be reversed"? This is only about the caret position. If you have a repro for that I'm happy to poke.

We have a ContentEditable code block as the main component of an editor page. There is an event handler that reads the code block content until the caret and may inject additional content to the contentEditable code block such as autocomplete suggestions at the caret position.

Because of the nature of this component and the dynamic contents inside, we have to prevent the default event handler for keyboard input and insert the new character at the caret position ourselves.

Caret being stuck at the leftmost position means when we try to read the code block contents until the caret, we always read empty string, and it also makes the characters typed by the user to be added to the beginning of the block where caret is stuck.

https://treeherder.mozilla.org/jobs?repo=try&revision=00570165f1d0893c25db1bf1b5453188fc291da4 is the rebased patch again.

(In reply to Ata Hakcil from comment #57)

We have a ContentEditable code block as the main component of an editor page. There is an event handler that reads the code block content until the caret and may inject additional content to the contentEditable code block such as autocomplete suggestions at the caret position.

Because of the nature of this component and the dynamic contents inside, we have to prevent the default event handler for keyboard input and insert the new character at the caret position ourselves.

Caret being stuck at the leftmost position means when we try to read the code block contents until the caret, we always read empty string, and it also makes the characters typed by the user to be added to the beginning of the block where caret is stuck.

That seems totally unrelated to this bug, assuming I'm understanding correctly. Presumably you're reading the caret position using Selection.focusNode / Selection.focusOffset or so? That seems nothing to do with this bug, this bug is about the caret being mispositioned at paint time. If you could build a test-case for that issue and file a new bug, it'd be appreciated. Feel free to cc me, post it here.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #58)

https://treeherder.mozilla.org/jobs?repo=try&revision=00570165f1d0893c25db1bf1b5453188fc291da4 is the rebased patch again.

(In reply to Ata Hakcil from comment #57)

We have a ContentEditable code block as the main component of an editor page. There is an event handler that reads the code block content until the caret and may inject additional content to the contentEditable code block such as autocomplete suggestions at the caret position.

Because of the nature of this component and the dynamic contents inside, we have to prevent the default event handler for keyboard input and insert the new character at the caret position ourselves.

Caret being stuck at the leftmost position means when we try to read the code block contents until the caret, we always read empty string, and it also makes the characters typed by the user to be added to the beginning of the block where caret is stuck.

That seems totally unrelated to this bug, assuming I'm understanding correctly. Presumably you're reading the caret position using Selection.focusNode / Selection.focusOffset or so? That seems nothing to do with this bug, this bug is about the caret being mispositioned at paint time. If you could build a test-case for that issue and file a new bug, it'd be appreciated. Feel free to cc me, post it here.

Unfortunately I can't come up with a clean POC to demonstrate this as I can't get everything to go precisely wrong with just vanilla JS the way it is going on my current project, but I've taken the following screen recording from a more minimal environment that shows (unless I'm completely misreading what is going on in the background) that my issue is also related to caret being mispositioned at the paint time in same way, although I'm not 100% certain.

POC Video
poc.svelte

When I have time, I'll check if the suggested solution also fixes this problem and if not I'll isolate the issue further and create a new bug report.

The behavior of the test-cases on the bug is identical to chromium now. That's because empty text nodes on ::before were suppressed in bug 1865668.

Flags: needinfo?(emilio)
See Also: → 1865668, 1855583
Attachment #9397423 - Attachment mime type: text/plain → text/html

This passes try and includes a test, but the test fails.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: