Custom element being inserted on backspace in contenteditable

NEW
Assigned to

Status

()

Core
Editor
P3
normal
11 months ago
11 months ago

People

(Reporter: ebert, Assigned: m_kato)

Tracking

(Blocks: 1 bug)

50 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.51 Safari/537.36

Steps to reproduce:

The contenteditable-enabled element contains a custom element that acts as a block-level wrapper, containing p-elements itself. Backspacing a child paragraph that contains only a `<br>` element causes Firefox to wrap the `<br>` with the custom-element itself.

You can find a full test case at https://jsfiddle.net/dtdesign2/156v703L/, it contains full instructions to reproduce this issue.


Actual results:

The `<p><br></p>` line was replaced with `<p><custom-element><br type="_moz"></custom-element></p>`


Expected results:

The `<p><br></p>` element should have been removed on backspace.

Updated

11 months ago
Component: Untriaged → Editor
Product: Firefox → Core
(Reporter)

Comment 1

11 months ago
This issue is even worse when the caret is positioned at the beginning of a non-empty paragraph and then backspace it hit. You can use the fiddle above to reproduce it by setting the caret in front of "Third line" and hit backspace. It'll split the parent into two separate containers and each consecutive use of backspace will spawn an empty `<custom-element></custom-element>` node after the upper custom element (the one that now ends with "Third line").

I've also noticed that the entire `<custom-element>`-block is being recreated when the originally described bug hits. If you capture the original custom element and store a reference in a variable, you'll notice that after reproducing the bug, the reference points to an element that is no longer part of the DOM. At this point the DOM contains two custom elements, but none of them were originally there, but have been spawned by Firefox.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Blocks: 1332973
Assignee: nobody → m_kato
Ah, custom element isn't as block since parser doesn't detect as block...
Comment hidden (mozreview-request)

Comment 4

11 months ago
mozreview-review
Comment on attachment 8830681 [details]
Bug 1329639 - Don't duplicate custom element by merging nodes.

https://reviewboard.mozilla.org/r/107416/#review108812

::: editor/libeditor/HTMLEditor.cpp:773
(Diff revision 1)
> +  nsIAtom* name = aElement->NodeInfo()->NameAtom();
> +  int32_t tag = parserService->HTMLCaseSensitiveAtomTagToId(name);
> +  if (tag == eHTMLTag_userdefined &&
> +      nsContentUtils::IsCustomElementName(name)) {

Hmm, although, I'm not familiar with how to check if it's custom element, this looks like hacky because this can hit unknown element which will be defined in a future spec. And also this can hit some obsolete element names, see https://dxr.mozilla.org/mozilla-central/rev/d92fd6b6d6bfc5b566222ae2957e55772d60151a/dom/base/CustomElementRegistry.cpp#659-666

I think that there should be nsINode::IsCustomElement() const, nsIContent::IsCustomElement() const or Element::IsCustomElement() const. (I like nsINode.) Or in EditorBase or EditorUtils. And I guess that it can use nsIContent::GetCustomElementData() to check if it's a custom element.

::: editor/libeditor/tests/test_bug1329639.html:42
(Diff revision 1)
> +  is(document.getElementsByTagName('custom-element').length, 1,
> +     "<custom-element> element shouldn't be duplicated");

Cannot we make the contents in <custom-element> smaller and use innerHTML? I like innerHTML because it indicates what the result is expected explicitly.
Attachment #8830681 - Flags: review?(masayuki) → review-
Smaug, do you know how to check if an nsINode is a custom element?
Flags: needinfo?(bugs)
oh, so this isn't about CustomElement, as in the CustomElement spec, but just some randomly named element?
If you replace 'custom-element' with 'random' in the test, there seems to be the same behavior.
(style random as display: block)

So, why to use nsContentUtils::IsCustomElementName in the patch?

By default unknown elements are display: inline, but can of course be styled as block.
Does this block issue happen also with by-default-inline-element, but styled as block?
Looks like so. Use <span> and not <custom-element> and style <span> as block.
So, isn't the issue here that editor relies on some default "isBlock" here, yet styling can change it.
Flags: needinfo?(bugs)
Indeed, you're right. I misunderstood the testcase.

Actually, when I replace "custom-element" with "ins" (styled as "display: block") in the testcase, I reproduce this bug. So, looks like that this is just a problem of listing-up which elements should be inserted.
You need to log in before you can comment on or make changes to this bug.