Open Bug 1615852 Opened 5 years ago Updated 2 months ago

broken `<br>` workaround in `<div contenteditable="true">` with bad side effects (also affects Google Sheets)

Categories

(Core :: DOM: Editor, defect, P3)

Product:
Core
Component:
DOM: Editor
Version:
72 Branch
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox75 --- affected
firefox76 --- affected

People

(Reporter: pavlix, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

1615852.html
872 bytes, text/html
Details

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

Firefox adds an unexpected <br> to an editable <div> while editing the content. It does so when you delete the content of an editable and the spurious <br> stays even when you add new content.

  1. Create an HTML document that contains a <div> element with contenteditable attribute set true.

  2. Edit the <div> and add some text.

  3. Delete all the text using a backspace.

  4. Now add some text again.

  5. Check the contents of the <div> using DOM inspector.

Note: Don't add any newlines when testing for the spurious <br> element using the test above. However, when finished with this scenario, please try also experimenting with multiple lines. Firefox adds <div> subelements and even things like <div><br></div>. There seems to be a whole class of bugs related to editable content.

Actual results:

The contents of the <div> is your new text and an additional <br> element. This causes issues in Google Sheets and might be a source of many issues in various other web applications.

Expected results:

The contents of the <div> should only be your new text.

You can also use the following code (e.g. in a local HTML file) to reproduce the issue and discover related issues.

<!DOCTYPE html>
<html>
<head>
    <style>
        #source { width: 10em; background-color: lightgreen; }
        #target { margin-top: 1em; background-color: lightgrey; }
    </style>
</head>
<body>
    <div id="source" contenteditable="true">xxx</div>
    <div id="target"></div>

    <script>
        source = document.getElementById("source");
        target = document.getElementById("target");

        function display() {
            target.innerText = source.innerHTML;
        }

        source.addEventListener("input", display);
        display();
    </script>
</body>
</html>

With this code I observe and reliably reproduce the bug as desribed in the bug report. I also observe unusual behavior (as suggested in the note) when adding new lines in the editable content. Then it creates new inner <div> elements and then backspace can delete everything meaning that in one scenario an empty editable innerHTML becomes <br> and in another scenario it actually becomes empty.

If the <br> was meant as a sort of a workaround for layout/accessibility issues, then the behavior should really be:

  1. Consistent. An empty cell should always have the same content.
  2. Correct. An empty cell reports no content to any Javascript that wants to access it.
  3. Accessible. An empty cell should not disappear from the user.

A similar bug was reported in the past and discussed in fora:

In the past the issue was treated in relation to Google Sheets but I believe it is now clear that the behavior of contenteditable divs is buggy. I was asked to start a new bug report and here it is.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → DOM: Editor
Product: Firefox → Core
Attached file 1615852.html —

I can reproduce this on the current nightly (20200216093058) on macOS -- attached is the testcase from the reporter with steps to reproduce inlined.

This is probably a duplicate (though with a specific STR) of bug 503838. See discussion in bug 1573801 comment 12 and onward.

The Google issue is tracked in bug 1605018.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3

Additional experiments and results. Someone in #1573801 mentioned that the bug is about HTML contenteditable.

  1. Load the above document.

xxx

  1. Put the carret after xxx and hit Backspace thrice.

<br> (the workaround)

  1. Put back xxx.

xxx<br> (looks good in my case but becomes xxx\n<br> in Google Sheets)

  1. Hit Enter.

<div>xxx</div><div><br></div> (does this make sense?)

  1. Hit Backspace.

<div>xxx</div> (inconsistent, <br> is lost now)

  1. Hit Backspace thrice.

<div><br></div> (inconsistent, <br> is now back but enclosed in divs)

  1. Hit Backspace.

`` (<br> is now lost and the workaround failed completely, layout is of course broken)

  1. Type xxx.

xxx (just for completeness, you are now back to the initial state)

Also, if I change the source, so that I start with an HTML <p> element, it degrades easily to the original case.

Additional code needed:

    <button onclick="source.innerHTML = 'xxx'; display()">Reset (noparagraph)</button>
    <button onclick="source.innerHTML = '<p>xxx</p>'; display()">Reset (paragraph)</button>
  1. Load the document and click Reset (paragraph).

<p>xxx</p>

  1. Hit Backspace thrice.

<p><br></p> (the workaround)

  1. Hit Backspace.

`` (workaround lost, layout broken, paragraph editing lost)

  1. Type xxx.

xxx (parapraph editing lost)

Honestly, I don't even understand what was the intended behavior. Why do we need to ad <div>s rather than <br> in the non-paragraph example? If that was supposed to be a plaintext variant, why the <div>s? If the latter was supposed to be an HTML variant, why does it so easily degrade to the former?

Summary: spurious `<br>` in `<div contenteditable="true">` (also affects Google Sheets) → broken `<br>` workaround in `<div contenteditable="true">` with bad side effects (also affects Google Sheets)
No longer blocks: contenteditable
Depends on: better-invisible-br-handling
Severity: normal → S3
Webcompat Priority: --- → ?
Webcompat Priority: ? → P1

This spurious <br> avoids to have some a default text after making a div empty.

<style>
	div {
		width: 200px;
		border: 1px solid blue;
	}
	
	div:empty::before {
		content: "Enter something";
		color: gray;
	}
</style>
<div contenteditable></div>

The severity field for this bug is set to S3. However, this bug has a P1 WebCompat priority.
:hsinyi, could you consider increasing the severity of this web compatibility bug?

For more information, please visit auto_nag documentation.

Flags: needinfo?(htsai)

Talked with Masayuki that this could cause new web-compat issue in the wild. However, changing around the invisible <br> elements may cause regression with the difference with the white-space normalizer... (We've already experienced it in bug 503838).

This issue was originally reported to cause a major website breakage (Google sheet), which was however tracked in bug 1605018 and fixed. Do we know other website breakages due to this? If not, is WebCompat P1 still a right rating? Thank you.

Flags: needinfo?(htsai) → needinfo?(dschubert)

I don't see any immediate related site breakage related to this. However, since this is part of an ongoing cross-browser-effort, I'll bring this up to our next triage meeting.

Webcompat Priority: P1 → ?
Flags: needinfo?(dschubert)

We're unsetting the webcompat priority flag since we don't currently have any site issues specifically related to this bug now that sheets is fixed. But there's still a lot of other bugs related to contentEditable, and it's possible that some are actually this issue, so in general contentEditable is still considered a webcompat priority issue.

Webcompat Priority: ? → ---

I've found a workaround for this bug, using a bunch of js, here: https://stackoverflow.com/a/5721576/1395757

But seems it does not work well, so here is my quick but ugly one:

          if (navigator.userAgent.includes("Firefox")) 
            if (editorElement.innerHTML.match(/[^\s]<br\/\>/)) {
              var brs = editorElement.getElementsByTagName("br");
              for (var i = 0; i < brs.length; i++) { brs[i].parentNode.removeChild(brs[i]); }
            }
          }

It would be good to get this bug addressed. Here's a real-life scenario where this is hurting us at Chess.com. We have a contentEditable=true div that is a chat box and includes an emoji picker. In Chromium and Webkit, if you write some text and then pick an emoji, that emoji is on the same line (we insert it programmatically into that div via div.innerHTML += emojiObj). However in Firefox, if you write some text and then pick an emoji, that emoji gets kicked down one line below. It's especially egregious because at that point there's no way to get your cursor between the text of the previous line and the emoji; in other words, you couldn't get it on the same line even if you wanted to.

In other words, this Firefox bug is actively causing a bad user experience, contra this:

We're unsetting the webcompat priority flag since we don't currently have any site issues specifically related to this bug

I beg to differ. Would appreciate prioritizing this back to webcompat priority flag. Thanks.

I would also agree to increase the priority. My team is about to release a product and this bug is causing some issues only in Firefox.

Hi all, I have additional information that might be helpful. While googling around this issue, I found the following stack overflow answer to be insightful: https://stackoverflow.com/a/50059064/6111743

It says that using <span> element for the contenteditable (instead of a div) will not cause the <br> to be inserted and I confirmed this is true. I replaced my contenteditable <div> with a <span> and set display:block on it, and the browser does not insert <br> tags when I delete the content in the span. This allowed me to style its :empty case and have the desired behavior.

The stackoverflow poster gave the following reasoning for why this works: "Span is an inline element and Div is a block element. Block elements when empty, by default, will have zero dimensions(if no padding is applied to the block element). In contrast, empty inline elements tend to maintain their height and width becomes zero."

That makes it sound like adding the <br> tag when it is a div (or "block element") was a deliberate decision about how contenteditable block elements should behave, perhaps not even a bug, since the behavior is different for inline elements. There must be a reason it's coded to work this way. I would love to know the real reasoning behind this, if any Firefox contributors could provide insight, as I am just curious to understand it. Is this really a bug, or intentional behavior coded for block elements, and if so, why?

In the meantime, for the other devs who have posted about issues with this, I can confirm that using <span> with display:block is a viable workaround.

I don't recommend to use <span contenteditable> because:

  1. there are some incompatibles between browsers even in the basic features (e.g., line break behavior with styling the editing host)
  2. honestly, there are no enough tests of the behavior due to mainly not used so widely and the low compatibility

<br> elements are required when:

  1. block elements are empty, for making them non-zero height and to put the first line for good caret position
  2. text ends by a collapsible white-space, to make it visible

Similar bug is breaking search input (tree view) in old Odoo versions (7 and 8).

(In reply to luffah from comment #16)

Firefox 123

A user on SUMO noted this issue in Froala, a rich text editor:

I assume all rich text editors are affected and would need to implement a workaround to remove <br> if it causes a problem for their users.

Well, one of the known problem of our builtin editor is, we don't remove <br> which is automatically inserted even when it becomes unnecessary. If fixing it is not enough for some web apps, we need some big changes about the white-space handling to stop inserting <br> except in an empty block.

I've been doing some experiments with this and can confirm that the problem occurs in any element below the "contenteditable" root.

I have been trying various work around to try and solve it by looking at the mutation records that occur in a Mutation Observer that is based on "contenteditable".

The following code is a good way to remove it (isGecko is a flag set const ua = navigator.userAgent; isGecko = /Gecko\//.test(ua);)

  if (isGecko && mutations.length >= 2) {
      
      if (mutations[0].type === 'characterData' && mutations[0].target.data.slice(-1) === ' ' && 
        mutations[1].type === 'childList'  && (mutations[1]?.addedNodes[0]?.nodeName??'' === 'BR')) {
          console.log('removing extra BR');
          mutations[1].target.removeChild(mutations[1].addedNodes[0]);
      }    
    }

The only problem is after I have removed the <br> typing any other character caused the space to be removed. There is no mutation record for the removal of the space, just a single "characterData" mutation with the new version of the text.

Before the space is first added the content of the "contenteditable" (with "|" showing cursor)

<ul><li>This|</li></ul

Then immediately after the space has been added and the mutation observer was able to remove the <br>

<ul><li>This |</li></ul>

And finally after I type another "i"

<ul><li>Thisi|</li></ul>

I found a way around all of this that appears to work in the Mutation Observers Callback I do the following. brRemoved is a global flag initially set to false

if (isGecko && brRemoved && mutations.length > 0 && mutations[0].type === 'characterData') {
      mutations[0].target.insertData(mutations[0].target.data.length - 1, ' ');
}
if (isGecko && (mutations.length !== 1 || mutations[0].type !== 'childList' )) {
      brRemoved = false;
 }

 if (isGecko && mutations.length >= 2) {
      if (mutations[0].type === 'characterData' && mutations[0].target.data.slice(-1) === ' ' && 
        mutations[1].type === 'childList'  && (mutations[1]?.addedNodes[0]?.nodeName??'' === 'BR')) {
          mutations[1].target.removeChild(mutations[1].addedNodes[0]);
          brRemoved = true;
      }    
 }

Starting from 135, when a padding <br> for the previous collapsible white-space is removed when it becomes not required.

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

Attachment

General

Creator:
Created:
Updated:
Size: