1411687 - getplan.co: Hitting enter when creating tasks does not create multiple tasks

Status: RESOLVED FIXED

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

Description

[Mike Taylor [:miketaylr]]

Attachment: 31808136-9489285a-b57a-11e7-88b1-43571b846b64.gif

Originally reported @ https://webcompat.com/issues/12652

STR:

Open https://getplan.co/list/work (you can auth with Google)
Create a task.
Open a task.
Try to create 2 steps: type "-\<space>First\<enter>Second"


----



https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2ba7fac730edb476a51a732011872e95d34d82c8&tochange=b6cbd404c8453d0f726eaab50eceeba22d859554

Fallout from Bug 1297414.

Comment 1

[Mike Taylor [:miketaylr]]

" Aryeh Gregor (:ayg) (no longer with Mozilla) <[e-mail]> is not currently accepting "needinfo" requests. "


masayuki-san, since you reviewed the patch, can you have a look? Is there a workaround we should be suggesting to sites that are broken due to this change?

Comment 2

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

Okay, looks like that the cause is, HTMLEditRules::WillInsertBreak() doesn't assume that custom element is an editing host:
https://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/editor/libeditor/HTMLEditRules.cpp#1566,1574

Although, I have no idea how should we behave in this case...

Comment 3

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

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

Comment 4

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

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

Comment 5

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

Attachment: Bug 1411687 - part 0: Get rid of HTMLEditor::GetBlockNodeParent(nsIDOMNode*)

Currently, HTMLEditor::GetBlockNodeParent(nsIDOMNode*) is used only by
HTMLEditor::DoInsertHTMLWithContext() and there is a variable of nsINode*.

So, we don't need to keep it anymore.

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

Comment 6

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

Attachment: Bug 1411687 - part 1: HTMLEditor::GetBlockNodeParent() and HTMLEditor::GetBlock() should take an ancestor limiter node optionally

Perhaps, most callers don't need parent block outside active editing host.
Therefore, callers of these methods should be able to specify the editing
host for making those methods stop looking for a block ancestor.

Then, callers can avoid using EditorUtils::IsDescendantOf() and
nsContentUtils::IsContentDescendantOf().

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

Comment 7

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

Attachment: Bug 1411687 - part 2: Rewrite the check to insert a <br> element in HTMLEditRules::WillInsertBreak()

Currently, HTMLEditRules::WillInsertBreak() checks if the editing host can
contain a <p> element as a child or a descendant.  However, this is not enough.
If an inline element has a block element which can contain a <p> element,
current implementation considers to insert a <br>.  This is possible when
* The editing host is an unknown element including user defined element.
* The editing host is an inline element and its children and/or descendants
  were added by JS.  E.g., <span contenteditable> element can have <div>
  element.

I think that we should consider to insert a <br> element when:
- There is no block ancestors in the editing host.
- The editing host is the only block element and it cannot contain <p> element
  or the default paragraph separator is <br> element.
- The nearest block ancestor isn't a single-line container declared in the
  execCommand spec and there are no block elements which can contain <p>
  element.

Note that Chromium checks if CSS box of ancestors is block too.  However,
it must be out of scope of this bug.

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

Comment 8

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

Attachment: Bug 1411687 - part 3: Add webplatform-test for testing "insertParagraph" command when user-defined element is the editing host

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

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8922774 [details]
Bug 1411687 - part 0: Get rid of HTMLEditor::GetBlockNodeParent(nsIDOMNode*)

https://reviewboard.mozilla.org/r/193914/#review199418

Comment 10

[Makoto Kato [:m_kato]]

Comment on attachment 8922775 [details]
Bug 1411687 - part 1: HTMLEditor::GetBlockNodeParent() and HTMLEditor::GetBlock() should take an ancestor limiter node optionally

https://reviewboard.mozilla.org/r/193916/#review199420

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8922776 [details]
Bug 1411687 - part 2: Rewrite the check to insert a <br> element in HTMLEditRules::WillInsertBreak()

https://reviewboard.mozilla.org/r/193918/#review199422

::: editor/libeditor/HTMLEditRules.cpp:1599
(Diff revision 1)
>    }
> -  ParagraphSeparator separator = mHTMLEditor->GetDefaultParagraphSeparator();
> -  if (!IsBlockNode(*host) ||
> -      // The nodes that can contain p and div are the same.  If the editing
> -      // host is a <p> or similar, we have to just insert a newline.
> -      (!mHTMLEditor->CanContainTag(*host, *nsGkAtoms::p) &&
> +
> +  // Look for the nearest parent block.  However, don't return error even if
> +  // there is no block parent here because in such case, i.e., editing host
> +  // is an inline element, we should insert <br> simply.
> +  nsCOMPtr<Element> blockParent = HTMLEditor::GetBlock(node, host);

RefPtr<Element> if possible

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8922777 [details]
Bug 1411687 - part 3: Add webplatform-test for testing "insertParagraph" command when user-defined element is the editing host

https://reviewboard.mozilla.org/r/193920/#review199424

Comment 13

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

Comment on attachment 8922774 [details]
Bug 1411687 - part 0: Get rid of HTMLEditor::GetBlockNodeParent(nsIDOMNode*)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193914/diff/1-2/

Comment 14

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

Comment on attachment 8922775 [details]
Bug 1411687 - part 1: HTMLEditor::GetBlockNodeParent() and HTMLEditor::GetBlock() should take an ancestor limiter node optionally

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193916/diff/1-2/

Comment 15

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

Comment on attachment 8922776 [details]
Bug 1411687 - part 2: Rewrite the check to insert a <br> element in HTMLEditRules::WillInsertBreak()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193918/diff/1-2/

Comment 16

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

Comment on attachment 8922777 [details]
Bug 1411687 - part 3: Add webplatform-test for testing "insertParagraph" command when user-defined element is the editing host

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193920/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63a18c9ef302
part 0: Get rid of HTMLEditor::GetBlockNodeParent(nsIDOMNode*) r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c8b86294e389
part 1: HTMLEditor::GetBlockNodeParent() and HTMLEditor::GetBlock() should take an ancestor limiter node optionally r=m_kato
https://hg.mozilla.org/integration/autoland/rev/eeec327a40fa
part 2: Rewrite the check to insert a <br> element in HTMLEditRules::WillInsertBreak() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/777f76d30950
part 3: Add webplatform-test for testing "insertParagraph" command when user-defined element is the editing host r=m_kato

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/63a18c9ef302
https://hg.mozilla.org/mozilla-central/rev/c8b86294e389
https://hg.mozilla.org/mozilla-central/rev/eeec327a40fa
https://hg.mozilla.org/mozilla-central/rev/777f76d30950

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Real-world web compat issue, but there since Fx55. Any thoughts about whether this is something we should consider for uplift this late in the cycle vs. letting it ride the 58 train?

Comment 20

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

As usual release cycle, this might be approved. However, 57's uplift rules are really strict. Similar request has already been not granted. So, I think that this cannot be fixed on 57.