1435123 - Pressing enter/return inside an inline editing host causes creating another inline editing host

Status: RESOLVED FIXED

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

Description

[Chen Hui Jing]

Attachment: contenteditable.mp4

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180201220225

Steps to reproduce:

1. Have a style tag set to display: block and contenteditable="true"
2. Press enter/return while caret is active


Actual results:

The style element was duplicated.


Expected results:

There should have been a line break as per previous editions of Firefox.

Comment 1

[Chen Hui Jing]

Attachment: contenteditable.gif

Comment 2

[Chen Hui Jing]

Attachment: testcase-contenteditable.html

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, this must be serious regression. I don't remember the bug#, IIRC, I fixed same bug...

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm not sure exact regression bug. However, this is caused by changing the default paragraph separator.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37430c5698740df3e00d4d1b620a2e89405ffaa4

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I think that all vendors can agree with inserting <br> element in inline editing host. However, it shouldn't be included into WPT for now since no documents declares this behavior and even Chrome/Safari doesn't work well in insertParagraph.html.

Filed spec bug:
https://github.com/w3c/editing/issues/175

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a897a003d2b68093b42526e011e6d9793a15ff9

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1435123 - HTMLEditor::GetBlock() has to specify given ancestor limit node to HTMLEditor::GetBlockNodeParent()

This is a simple bug of internal API of HTMLEditor.  HTMLEditor::GetBlock()
tries to retrieve nearest ancestor block node (including itself) of a node.
HTMLEditor::GetBlock() may have ancestor limiter typically it's active
editing host to prevent to modify editing host or its ancestor accidentally.

However, it forgets to call HTMLEditor::GetBlockNodeParent() with the given
ancestor limit node.  Therefore, if editing host is an inline element and
its parent is a block element, the editing host is split accidentally.

Review commit: https://reviewboard.mozilla.org/r/232498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232498/

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8963600 [details]
Bug 1435123 - HTMLEditor::GetBlock() has to specify given ancestor limit node to HTMLEditor::GetBlockNodeParent()

https://reviewboard.mozilla.org/r/232498/#review238154

Comment 10

[Makoto Kato [:m_kato]]

Comment on attachment 8963600 [details]
Bug 1435123 - HTMLEditor::GetBlock() has to specify given ancestor limit node to HTMLEditor::GetBlockNodeParent()

https://reviewboard.mozilla.org/r/232498/#review238158

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7fa5324198f
HTMLEditor::GetBlock() has to specify given ancestor limit node to HTMLEditor::GetBlockNodeParent() r=m_kato

Comment 12

[Narcis Beleuzu [:NarcisB]]

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

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8963600 [details]
Bug 1435123 - HTMLEditor::GetBlock() has to specify given ancestor limit node to HTMLEditor::GetBlockNodeParent()

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1430551 (from point of view of release channel).

[User impact if declined]:
May duplicate editing host (i.e., <foo contenteditable>) unexpectedly when typing Enter key.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No since automated tests completely tests possible simple cases.

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
HTMLEditor::GetBlock() is an internal API of HTMLEditor. However, the second argument, which is the cause of this bug, is not used by the other callers (i.e., called only by HTMLEditRules::WillInsertBreak()):
https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla10HTMLEditor8GetBlockER7nsINodePS1_&redirect=false
Therefore, this patch actually changes only the path handling Enter key press.

[String changes made/needed]:
No.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8963600 [details]
Bug 1435123 - HTMLEditor::GetBlock() has to specify given ancestor limit node to HTMLEditor::GetBlockNodeParent()

One-line fix with an automated test to fix a new regression in Fx60 (from the release point of view). Approved for 60.0b9.

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d0a166d249fb

Comment 16

[Andrei Vaida [:avaida]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #13)
> [Is this code covered by automated tests?]:
> Yes.
> 
> [Has the fix been verified in Nightly?]:
> Yes.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: 
> No since automated tests completely tests possible simple cases.

Setting qe-verify- based on Masayuki Nakano's assessment on manual testing needs and the fact that this fix has automated tests.