Closed Bug 1098151 Opened 10 years ago Closed 5 years ago

Empty contenteditable elements should have <br> or min-height as one line-height to prevent collapsing

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla76
Tracking Flags:
Tracking Status
firefox76 --- verified

People

(Reporter: masayuki, Assigned: emilio)

References

(Blocks 2 open bugs,
URL
)

Details

Attachments

(1 file)

Bug 1098151 - Make empty editable blocks at least one line-height tall. r=jfkthame
47 bytes, text/x-phabricator-request
Details | Review
I'm not sure if this is a job of ns*Editor, though. On IE, contenteditable editor always has <br> for making the min-height is at line one line. On Chrome, neither innerHTML of empty contenteditable editor includes <br> nor its min-height is specified but it has one line height. The simplest fix of this is adding min-height for *[contenteditable=true]. But I don't like it due to hacky. So, I like inserting <br> better. But I'm not sure who should insert <br> element. And also if contenteditable attribute is removed, should the <br> element is removed??
Hmm, IE removes <br> element when removing contenteditable attribute if it was inserted automatically.
julienw: Oh, sorry, you've already filed it...
It's ok :)
Hmm, IE magically removes <br> element appended automatically. E.g., 1-1. Press toggle contenteditable (1) button. Then, <br> element is removed. 2-2. Press toggle contenteditable (2) button. Then, <br> element which is created by HTML source isn't removed. 3-1. Focus second contentediable element which has <br>. 3-2. Do select all. 3-3. Press Delete key. 3-4. Press toggle contenteditable (2) button. Then, <br> element is removed. 4-1. Press make it contenteditable (3) button. 4-2. Press append p element (3) button. Then, <br> element is removed. It's hard to emulate same behavior??
URL: http://jsfiddle.net/xdLt8ouh/5/
Hmm, there are already 2 special <br> elements inserted by editor. One is <br mozeditorbogusnode="TRUE">. That looks like used for a holder place of empty <body> element. The other is <br type="_moz">. That looks like used for a holder place to create a block element by editor. I think that we need to create new special <br> like <br mozeditinghostbogusnode="TRUE">. And when an Element's editable state is changing and it's editable host, it should insert the special <br> and becoming it's not editable, such special <br> element should be removed. However, for the result of innerHTML, perhaps, we should not use attribute for distinguishing if a <br> element is special. We should set a flag into nsIContent or something similar level. I.e., we should not expose such information to web apps. So, I think we should do: 1. Hide special <br> elements' non-standard attributes from web apps. 2. An empty element should create new special <br> element when it becomes editing host. 3. A contenteditable element should remove the special <br> element when it becomes non-editing host. 4. nsHTMLEditor should mark <br> node which is inserted by nsHTMLEditRules::InsertBRIfNeeded() as new special <br> element. 5. nsHTMLEditor should remove new special <br> element when user types a character (optional) Then, we can emulate IE's behavior and make result of innerHTML better compatibility with Chrome. How do you think, Ehsan?
Flags: needinfo?(ehsan.akhgari)
I'm not so enthusiastic about adding magic <br>s. What's wrong with some sort of min-height approach? We'd have to come up with a proper selector -- we can't just check the contenteditable attribute because of contenteditable=inherit (sigh). Maybe :read-write, or if that doesn't work, will the CSS people less us make something up? https://html.spec.whatwg.org/multipage/scripting.html#selector-read-write If we implement :read-write per spec, it seems ":read-only > :read-write:not(textarea):not(input)" should match all editing hosts and nothing else. I don't know what the correct value is offhand -- is 1em right, or is that not the value you'll get when you start typing? I'm sure this would cause some corner-case problems, but it's not magical, so I'd want to try it before resorting to magic <br>s. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6) > 4. nsHTMLEditor should mark <br> node which is inserted by > nsHTMLEditRules::InsertBRIfNeeded() as new special <br> element. I don't think we want to do this. The <br> inserted by nsHTMLEditRules::InsertBRIfNeeded is currently a regular <br> -- why shouldn't it stay that way? The only problem we need to solve is when the whole editing host collapses, so there's nowhere to put the cursor. > 5. nsHTMLEditor should remove new special <br> element when user types a > character (optional) For blocks that contain only a <br>, we should already be removing the <br> when the user types a character, IMO. The <br> doesn't affect rendering, so it's useless and should be left out. (And we need to insert a <br> when the last character in the block is deleted, of course.) The spec does this, and a quick test indicates Chrome does, but IE and Gecko do not.
(In reply to :Aryeh Gregor from comment #7) > For blocks that contain only a <br>, we should already be removing the <br> > when the user types a character, IMO. The <br> doesn't affect rendering, so > it's useless and should be left out. (And we need to insert a <br> when the > last character in the block is deleted, of course.) The spec does this, and > a quick test indicates Chrome does, but IE and Gecko do not. I should add that of course, we should first make sure we're correctly adding a <br> in every case when we delete the last character in a block, before we start removing <br>s. Otherwise we'd cause regressions like <p><br></p> -> (press "a") -> <p>a</p> -> (backspace) -> <p></p> Currently it's not such a problem if we don't add a <br> to a block we just emptied, because we often have a <br> at the end anyway from when the empty block was first created.
Please let's not mess with any br elements here. It would be great if we can just get away with adding the correct min-height rule here: <http://mxr.mozilla.org/mozilla-central/source/layout/style/contenteditable.css>
Flags: needinfo?(ehsan.akhgari)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9) > Please let's not mess with any br elements here. It would be great if we > can just get away with adding the correct min-height rule here: > <http://mxr.mozilla.org/mozilla-central/source/layout/style/contenteditable. > css> Okay, then, I'll try with that.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
I realized that if we use min-height for this, the height doesn't match in most cases because we cannot specify the height with line-height value. I.e., the height may be changed at inputting first character. It's not good behavior. I guess that using ::before or ::after is better.
Maybe layout has something else more appropriate for this purpose?
Flags: needinfo?(dbaron)
Using pseudo element causes reproducing some hidden bugs in a11y and some XBL components... I'm still researching...
If we had a unit which is per line-height like "1.0lh", it'd be easier to fix this, though.
From CKEditor perspective (and I think that other editor developers would agree with me) the important aspect is that the browser engine should not interfere with an editor built on top of it. There's no problem if inserting the <br> filler follows a user action (like pressing backspace), but as you can see in bug 911201 in that case Gecko adds the filler when I set `editable.innerHTML`. This is highly undesirable because it may break algorithms implemented in the editor (e.g. because some ranges get desynchronised with the DOM). (Note that setting `editable.innnerHTML = ''` should not trigger the <br> insertion as well.) Another undesirable behaviour would be to insert the <br> filler if user action didn't change the DOM (e.g. when user pressed delete at the beginning of a first paragraph). If a <br> would be mistakenly inserted by the browser engine in this case it would result in breaking some editors' undo managers. This makes using some CSS hack the nicest (because safest) option if you plan to fix this issue (because it's not a problem for editors like CKEditor).
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11) > I realized that if we use min-height for this, the height doesn't match in > most cases because we cannot specify the height with line-height value. > I.e., the height may be changed at inputting first character. It's not good > behavior. It wouldn't be great, but I don't think it would be so bad. It's certainly much better than our current behavior. The point here is really that the user should have somewhere to click, instead of the editing host becoming invisible when emptied. It's not essential that it be exactly the right height. If the author wants a consistent height after the user inputs the first character, they can put in a <br> to start with. It would certainly be more elegant to have an exact solution, but I don't think it should block this bug. Just use 1em, or alternatively, whatever the default line-height is.
I'm still trying to fix this with ::before/::after approach. However, restyling (removing) them at inserting text causes some oranges and their causes still unknown. I guess that min-height approach is the best, but I believe that if we could, we should add new CSS unit which represents line-height. dbaron, how do you think that our style system has new <length> unit for internal use?
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #12) > Maybe layout has something else more appropriate for this purpose? Are you talking about a line-height relative unit, such as proposed in http://lists.w3.org/Archives/Public/www-style/2014Jun/0000.html ? I think it seems reasonably straightforward to implement, although we need to be careful of style struct dependencies, and the memory / performance implications of large scale use aren't great (though this doesn't sound like large-scale use). (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17) > dbaron, how do you think that our style system has new <length> unit for > internal use? We could probably add such a unit, but we don't have one now.
Flags: needinfo?(dbaron)
Summary: Empty contenteditable elements should have <br> or min-height as one line-height for preventing collappsed → Empty contenteditable elements should have <br> or min-height as one line-height to prevent collapsing
(Actually, it might be somewhat difficult to add because of dependencies between style structs.)
Thank you, dbaron. I'll research about it when I'm available. But now, I have some other work, so, if somebody wants to take this, feel free to do it.
Assignee: masayuki → nobody
Status: ASSIGNED → NEW
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #19) > (Actually, it might be somewhat difficult to add because of dependencies > between style structs.) For this, I guess the simplest way would be moving mLineHeight from nsStyleText to nsStyleFont, so that we won't add new dependency between style structs. OTOH, I suspect that line-height is usually specified with font properties, hence this movement should be fine.
I had to work once again with contenteditable elements, and once again it's painful that this bug happens.
Now there is a new error. CSS pseudo selector :empty not work after we have changed contenteditable back to empty.
Assignee: nobody → m_kato
Blocks: 1327333
Chrome does not do this. It's a bit of a shock when your data model expects "" or null, and gets a spurious "<br>". Even worse, if the user started with "100", hit backspace once or twice, and then retyped, then it's "100", but backspace thrice, followed by retyping and suddenly, it's "100<br>".
No longer blocks: 1327333
Blocks: 1486179

(In reply to Richard Neill from comment #25)

Chrome does not do this.

It's a bit of a shock when your data model expects "" or null, and gets a
spurious "<br>".
Even worse, if the user started with "100", hit backspace once or twice, and
then retyped, then it's "100", but backspace thrice, followed by retyping
and suddenly, it's "100<br>".

Chrome has a layout hack, see https://github.com/w3c/csswg-drafts/issues/4904.

Attachment #9136363 - Attachment description: Bug 1098151 - WIP: Make empty editable blocks at least one line-height tall. → Bug 1098151 - Make empty editable blocks at least one line-height tall. r=jfkthame
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/66e5251cc1e2 Make empty editable blocks at least one line-height tall. r=jfkthame
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/1893ec9ff2de Make the test a bit more forgiving to pass on Mac, like the following test.
Backout by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f86c7079e1c8 Backed out 2 changesets for refrest failures on overflow.html. CLOSED TREE
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bb399a356419 Make empty editable blocks at least one line-height tall. r=jfkthame

https://hg.mozilla.org/mozilla-central/rev/bb399a356419

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox76: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
Assignee: m_kato → emilio

Verified with 76.0a1 (2020-03-31), also issue reported in bug 1625434 is no longer encountered.

Status: RESOLVED → VERIFIED
status-firefox76: fixed → verified
Regressions: 1628770
Depends on: 1907433
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: