1406726 - After pressing ENTER appeard new line but cursor dont nove to new line, only to start of line.

Status: RESOLVED FIXED

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

Comment 1

[Václav Skála]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7d39d921ab4241c3dc9dd9013ee5d3c950c084fa&tochange=21eac8bd06451064524aaafbfa6a4ec5c2bd8130

Happens on email.cz when answer on mail. I can give test account.

Comment 2

[Valentina Claudia Ona]

Hi Skala
Can you please tell me steps to reproduce this issue? Which versions of FF are affected? On which OS you reproduce this issue?

Comment 3

[Václav Skála]

Hi
its appeard on both Windows and Linux on x86_64 architecture in nightly
with mozregression i detect that it affect all builds after 
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7d39d921ab4241c3dc9dd9013ee5d3c950c084fa&tochange=21eac8bd06451064524aaafbfa6a4ec5c2bd8130

I sent you a login to the test account on the your mail. To reproduce this bug log in on email.cz, open first message and press "odpovědět" (reply), then write some text and press ENTER, you will see that new line appeard but cursor dont move down. This bug is only in nightly, on firefox and other browsers it work fine.

Comment 4

[Valentina Claudia Ona]

Hi Skala,
Thanks for your help.
I verify this issue using Nightly 58.0a1 on Windows 10 x64, with Build ID 20171009220104
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
To reproduce this issue you need to have an account and to be logged in.

Comment 5

[Makoto Kato [:m_kato]]

(In reply to Valentina Claudia Ona from comment #4)
> Hi Skala,
> Thanks for your help.
> I verify this issue using Nightly 58.0a1 on Windows 10 x64, with Build ID
> 20171009220104

Does "verify" mean whether confirmed this bug or fixed by 58?  If this is regression by 1297414, it can change behavior by editor.use_div_for_default_newlines.  (beta and release channel is false, and nightly is true).

Comment 6

[Valentina Claudia Ona]

I can confirm this bug is true.

Comment 7

[Makoto Kato [:m_kato]]

OK, although his is regression, but I think that this won't occur on beta and release...  Even if this occur with editor.use_div_for_default_newlines=false on Nightly, I will raise priority.  Do anyone this it?

Comment 8

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

Hi, could you give a test account to fix this bug? I think that we should enable the new behavior in default setting for all users starting from Firefox 60 (next ESR). So, we need to fix this bug ASAP.

Comment 9

[Václav Skála]

Hi, I sent you a login to the test account on the your mail. To reproduce this bug log in on email.cz, open first message and press "odpovědět" (reply), then write some text and press ENTER. 

But there is one complication. After this commit
(https://hg.mozilla.org/integration/autoland/json-pushes?changeset=bda35e3d8ce4e6fbdc416b427f2200110ce52ff4&full=1
MozReview-Commit-ID: LF2MwASuXzZ   )

behavior changed, now pressing ENTER add 2 new lines instead of 1.

Comment 10

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

Thank you.

Minimized testcase is here: https://jsfiddle.net/d_toybox/c9d308b7/

When you type Enter, HTMLEditRules::WillInsertBreak() calls HTMLEditRules::ApplyBlockStyle() via HTMLEditRules::MakeBasicBlock() to wrap existing nodes with <div> and create new <div> since the nearest ancestor block is editing host. When HTMLEditRules::ApplyBlockStyle() wraps existing nodes with <div>, HTMLEditRules::ApplyBlockStyle() sets its mNewBlock to the last <div> created by the method. Finally, HTMLEditRules::AfterEditInner() moves selection in the last new block which is mNewNode. However, from the point of view of users, it doesn't make sense to move caret into the last div if it just wraps existing node (in this case, it's the <button>).

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=037ce4127e7ea3b55a2e22ff22be10095581b0cd

Comment 12

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

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

Comment 13

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

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

Comment 14

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

Attachment: Bug 1406726 - HTMLEditRules::WillInsertBreak() should reset mNewNode with caret position

HTMLEditRules::WillInsertBreak() started to use HTMLEditRules::MakeBasicBlock()
to wrap existing inline elements with default paragraph separator if inline
elements are children of editing host.  However,
HTMLEditRules::ApplyBlockStyle() called by HTMLEditRules::MakeBasicBlock() sets
mNewNode to last new block node.  So, if it wraps inline elements after caret
position, mNewNode becomes after expected caret position and
HTMLEditRules::AfterEditInner() will move caret into it.

This patch make HTMLEditRules::WillInsertBreak() reset mNewNode with
caret position after calling HTMLEditRules::MakeBasicBlock().

Additionally, this patch fixes a bug of HTMLEditor::IsVisibleBRElement().
That is, it uses only editable nodes to check if given <br> element is
visible.  However, editable state is not related to this check.  If <br>
element is followed by non-editable inline node (except invisible data
nodes), it always visible.  This bug caused getting wrong nodes with
HTMLEditRules::GetNodesFromSelection() which is used by
HTMLEditRules::MakeBasicBlock().  Therefore, this patch also adds lots of
EditorBase::Get(Next|Previous)ElementOrText*() and
HTMLEditor::Get(Next|Previous)HTMLElementOrText*() to ignore only invisible
data nodes.

Note that even with this fix, the range of nodes computed by
HTMLEditRules::GetNodesFromSelection() is still odd if only non-editable
elements follow a <br> node which is first <br> element after the caret
position.  However, what is expected by the execCommand spec is unclear.
So, automated test added by this patch checks current inconsistent behavior
for now.

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

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8950847 [details]
Bug 1406726 - HTMLEditRules::WillInsertBreak() should reset mNewNode with caret position

https://reviewboard.mozilla.org/r/220076/#review226350

Good, but it might be weird when using 4 bool paramers.  After fixing this, we might consider good way except to using a lot of bool parameter.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac9440a7256c
HTMLEditRules::WillInsertBreak() should reset mNewNode with caret position r=m_kato

Comment 17

[Andreea Pavel [:apavel]]

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