Closed Bug 1361008 Opened 7 years ago Closed 7 years ago

execCommand("defaultparagraphseparator", false, "p"); sometimes adds a surplus paragraph

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 + fixed
firefox56 --- fixed

People

(Reporter: jorgk-bmo, Assigned: m_kato)

References

Details

(Keywords: regression)

Attachments

(3 files)

Simple web pages showing the problem in Nightly
852 bytes, text/html
Details
Bug 1361008 - Part 1. Don't select invisible text node on makeBasicBlock.
59 bytes, text/x-review-board-request
masayuki
: review+
jcristau
: approval-mozilla-beta+
Details
Bug 1361008 - Part 2. add test for invisible node.
59 bytes, text/x-review-board-request
masayuki
: review+
Details 
+++ This bug was initially created as a clone of Bug #1297414 +++

See attached HTML document. We have
  text<br>
  <div>...</div>

Instructions:
1) Position caret at the end of the line an hit "enter".
2) Observe the resulting *three* paragraphs where only *two* are expected.
Summary: execCommand("defaultparagraphseparator", false, "p"); sometimes add too many paragraphs → execCommand("defaultparagraphseparator", false, "p"); sometimes adds a surplus paragraph
Even if not using execCommand("defaultparagraphseparator", false, "p"), this issue will occurs.

Using the following content, 

<div contenteditable>
 [whitespace only text node]
 [text node]
 [caret position]
 [br element]
 [whitespace only text node]
</div>

when hitting enter key, we try to select all children of div.  So we will add <p> or <div> into [whitespace only text node].  So we should ignore [whitespace only text node].
tracking-firefox55: --- → ?
Keywords: regression
Assignee: nobody → m_kato
Tracking 55+ for this editor regression.
tracking-firefox55: ? → +
Makoto, do we think this will land in the 55 window?
Flags: needinfo?(m_kato)
(In reply to Mike Taylor [:miketaylr] (55 Regression Engineering Owner) from comment #3)
> Makoto, do we think this will land in the 55 window?

Now, although I am working this, I cannot land this week.  So I will fix this on 56.  (But this is a regression, I will request uplift.)
Flags: needinfo?(m_kato)
Comment on attachment 8876653 [details]
Bug 1361008 - Part 1. Don't select invisible text node on makeBasicBlock.

https://reviewboard.mozilla.org/r/148006/#review152312

With merge the new loop to the previous loop, r=masayuki.

::: commit-message-04282:1
(Diff revision 1)
> +Bug 1361008 - Part 1. Don't select invisual text node on makeBasicBlock. r?masayuki

s/invidual/invisible ?

::: commit-message-04282:3
(Diff revision 1)
> +GetNodesForOperation(..., makeBasicBlock) will get all nodes of promoted range.  But it includes non-visual text node after <br> element.  So HTMLEditRules::MakeBasicBlock will creates block for non-visual text node.
> +
> +So we should remove non-visual text node from nodes of formatBlock.

s/non-visual/invisible ? (3 times)

::: editor/libeditor/HTMLEditRules.cpp:5851
(Diff revision 1)
>      for (int32_t i = aOutArrayOfNodes.Length() - 1; i >= 0; i--) {
>        OwningNonNull<nsINode> node = aOutArrayOfNodes[i];
>        if (HTMLEditUtils::IsListItem(node)) {
>          int32_t j = i;
>          aOutArrayOfNodes.RemoveElementAt(i);
>          GetInnerContent(*node, aOutArrayOfNodes, &j);
>        }
>      }
> +    // Empty text node shouldn't be selected if unnecessary
> +    for (int32_t i = aOutArrayOfNodes.Length() - 1; i >= 0; i--) {
> +      OwningNonNull<nsINode> node = aOutArrayOfNodes[i];
> +      if (EditorBase::IsTextNode(node)) {

Why don't you make this |else if| of |if (HTMLEditUtils::IsListItem(node))| in above loop?
Attachment #8876653 - Flags: review?(masayuki) → review+
Comment on attachment 8876654 [details]
Bug 1361008 - Part 2. add test for invisible node.

https://reviewboard.mozilla.org/r/148008/#review152318

I assume that you have run this without part 1 to check if this detects regression.

::: commit-message-89965:1
(Diff revision 1)
> +Bug 1361008 - Part 2. add test for invisual node. r?masayuki

s/invisual/invisible ?

::: editor/libeditor/tests/test_bug1361008.html:29
(Diff revision 1)
> +SimpleTest.waitForFocus(() => {
> +  document.execCommand("defaultparagraphseparator", false, "div")
> +  let edit1 = document.getElementById("edit1");
> +  edit1.focus();
> +  let selection = window.getSelection();
> +  selection.collapse(edit1, 0);

This move selection at the start of first text node which only has white spaces, right?

Could you make it clear with comment?

::: editor/libeditor/tests/test_bug1361008.html:34
(Diff revision 1)
> +  selection.collapse(edit1, 0);
> +  synthesizeKey("A", {});
> +  synthesizeKey("B", {});
> +  synthesizeKey("VK_RETURN", {});
> +  // count <div> element into contenteidable
> +  is(edit1.getElementsByTagName('div').length, 2, "DIV element should be 2");

DIV element should have 2 children?

::: editor/libeditor/tests/test_bug1361008.html:35
(Diff revision 1)
> +  synthesizeKey("A", {});
> +  synthesizeKey("B", {});
> +  synthesizeKey("VK_RETURN", {});
> +  // count <div> element into contenteidable
> +  is(edit1.getElementsByTagName('div').length, 2, "DIV element should be 2");
> +  is(edit1.getElementsByTagName('div')[0].innerText, "AB", "AB should be into 1nd DIV element");

s/into/in
s/1nd/1st

::: editor/libeditor/tests/test_bug1361008.html:36
(Diff revision 1)
> +  synthesizeKey("B", {});
> +  synthesizeKey("VK_RETURN", {});
> +  // count <div> element into contenteidable
> +  is(edit1.getElementsByTagName('div').length, 2, "DIV element should be 2");
> +  is(edit1.getElementsByTagName('div')[0].innerText, "AB", "AB should be into 1nd DIV element");
> +  is(edit1.getElementsByTagName('div')[1].innerHTML, "<br>", "BR element should be into 2nd DIV element");

s/into/in

::: editor/libeditor/tests/test_bug1361008.html:46
(Diff revision 1)
> +  selection.collapse(edit2, 0);
> +  synthesizeKey("A", {});
> +  synthesizeKey("B", {});
> +  synthesizeKey("VK_RETURN", {});
> +
> +  is(edit2.getElementsByTagName('p').length, 2, "P element should be 2");

"P element should have 2 children"

::: editor/libeditor/tests/test_bug1361008.html:47
(Diff revision 1)
> +  is(edit2.getElementsByTagName('p')[0].innerText, "AB", "AB should be into 1st P element");
> +  is(edit2.getElementsByTagName('p')[1].innerHTML, "<br>", "BR element should be into 2nd P element");

s/into/in
Attachment #8876654 - Flags: review?(masayuki) → review+
Comment on attachment 8876653 [details]
Bug 1361008 - Part 1. Don't select invisible text node on makeBasicBlock.

https://reviewboard.mozilla.org/r/148006/#review152312

My concern is that previous GetInnerContent call might insert empty text node.   But if you say OK, it will merge it.
Comment on attachment 8876653 [details]
Bug 1361008 - Part 1. Don't select invisible text node on makeBasicBlock.

https://reviewboard.mozilla.org/r/148006/#review152312

Indeed, you're right.

Perhaps, GetInnerContent() should have an argument to ignore empty text nodes. However, it should be out of scope of this bug.
Comment on attachment 8876654 [details]
Bug 1361008 - Part 2. add test for invisible node.

https://reviewboard.mozilla.org/r/148008/#review153286

::: editor/libeditor/tests/test_bug1361008.html:29
(Diff revision 1)
> +SimpleTest.waitForFocus(() => {
> +  document.execCommand("defaultparagraphseparator", false, "div")
> +  let edit1 = document.getElementById("edit1");
> +  edit1.focus();
> +  let selection = window.getSelection();
> +  selection.collapse(edit1, 0);

I need insert text before br element.  I will add the comment.
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/a09b4082b715
Part 1. Don't select invisible text node on makeBasicBlock. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/5af61567654b
Part 2. add test for invisible node. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/a09b4082b715
https://hg.mozilla.org/mozilla-central/rev/5af61567654b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8876653 [details]
Bug 1361008 - Part 1. Don't select invisible text node on makeBasicBlock.

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1297414

[User impact if declined]:
When typing [Enter] key, new line is inserted twice on using HTML editor such as gmail.

[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.

[List of other uplifts needed for the feature/fix]:
N/A

[Is the change risky?]:
Low.

[Why is the change risky/not risky?]:
Excluding empty text node for new line block target.  Most html editing doesn't have empty text node.

[String changes made/needed]:
No.
Attachment #8876653 - Flags: approval-mozilla-beta?
Comment on attachment 8876653 [details]
Bug 1361008 - Part 1. Don't select invisible text node on makeBasicBlock.

regression fix for beta55, should be in 55.0b4
Attachment #8876653 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
For the record, you could make this a web-platform-test by using the "inserttext" and "insertparagraph" commands instead of keypresses.  They should behave identically (although it's theoretically possible they don't).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: