1416099 - HTMLEditRules::ReturnInParagraph() should take EditorRawDOMPoint

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

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

This is another follow up bug of bug 1407305. Since the priority is less than bug 1415800 but perhaps, the patch might be big.

Comment 1

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

FYI: I'll request review for patches.  However, even if they won't be landed by next merge, it's no problem. Of course, including the patches in 58 makes us easier to uplift, though.

Comment 2

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

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

Comment 3

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

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

Comment 4

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

Attachment: Bug 1416099 - part 1: Make HTMLEditRules::ReturnInParagraph() use EditActionResult

For reducing the number of arguments of HTMLEditRules::ReturnInParagraph(),
let's make it return EditActionResult and get rid of |aCancel| and |aHandled|.

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

Comment 5

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

Attachment: Bug 1416099 - part 2: Rename |sibling| in HTMLEditRules::ReturnInParagraph() to |brNode|

The local variable |sibling| in HTMLEditRules::ReturnInParagraph() is set to
aBRNode of SplitParagraph().  So, the name should be renamed to |brNode|.

Then, the |brNode| should be cleared if it's not a <br> node actually.  However,
SplitParagraph() doesn't allow aBRNode to be nullptr.  aBRNode is only used to
remove it from the document if it's not necessary.  So, it should be able to be
nullptr.  Therefore, this patch changes SplitParagraph() too.

Note that SplitParagrah() is called only by ReturnInParagraph().  So, we don't
need to worry about other callers.

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

Comment 6

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

Attachment: Bug 1416099 - part 3: Rename aPara of HTMLEditRules::ReturnInParagraph() to aParentDivOrP and make it and aSelection as references

This cleans up |aSelection| and |aPara| of HTMLEditRules::ReturnInParagraph().

|aSelection| should be reference for avoiding nullptr check.

|aPara| is so too.  Additionally, the name is not clear.  We should rename it
to |aParentDivOrP| because it's a block parent of the point and has to be
<div> or <p> element.

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

Comment 7

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

Attachment: Bug 1416099 - part 4: Make HTMLEditRules::ReturnInParagraph() use EditorRawDOMPoint to store point to insert new <br> element before splitting the parent block

HTMLEditRules::ReturnInParagraph() stores point to insert new <br> element with
a set of |parent|, |node| and |offset|.  Their names are really unclear and
they're really messy.

So, let's use new local variable, |EditorRawDOMPoint pointToInsertBR|.  This is
set only when new <br> element is necessary.  So, we can get rid of
|newBRneeded| too.

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

Comment 8

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

Attachment: Bug 1416099 - part 5: Rename |selNode|, |selOffset| and |newSelNode| in HTMLEditRules::ReturnInParagraph() to proper names

|selNode| and |selOffset| in HTMLEditRules::ReturnInParagraph() store the
point to split |aParentDivOrP|.  So, they should be |containerAtSplitPoint|
and |offsetAtSplitPoint|.

Then, |newSelNode| can be renamed to |splitAfterNewBR|.

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

Comment 9

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

Attachment: Bug 1416099 - part 6: Make HTMLEditRules::WillInsertBreak() use EditorDOMPoint to store selection start

The only caller of HTMLEditRules::ReturnInParagraph() is
HTMLEditRules::WillInsertBreak().  It should use EditorDOMPoint for storing
selection start.

Then, this patch makes what is set to ReturnInParagraph() clear.  So, the
point ReturnInParagraph() is always start of selection.

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

Comment 10

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

Attachment: Bug 1416099 - part 7: Make HTMLEditRules::ReturnInParagraph() use start of selection to split parent block instead of receiving the point from arguments

Now, we know HTMLEditRules::ReturnInParagraph() always splits the parent block
at start of selection.  So, it doesn't need to receive the position from the
caller because the cost to get start of selection from first range of Selection
is really cheap.

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

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8927376 [details]
Bug 1416099 - part 1: Make HTMLEditRules::ReturnInParagraph() use EditActionResult

https://reviewboard.mozilla.org/r/198616/#review203932

::: editor/libeditor/HTMLEditRules.h:297
(Diff revision 1)
>                         Tables aTables = Tables::yes);
>    Element* IsInListItem(nsINode* aNode);
>    nsAtom& DefaultParagraphSeparator();
>    nsresult ReturnInHeader(Selection& aSelection, Element& aHeader,
>                            nsINode& aNode, int32_t aOffset);
> -  nsresult ReturnInParagraph(Selection* aSelection, nsINode* aHeader,
> +  EditActionResult ReturnInParagraph(Selection* aSelection, nsINode* aHeader,

Although I will handle by another bug, aHeader should be Element instead of nsINode.

::: editor/libeditor/HTMLEditRules.cpp:6832
(Diff revision 1)
>        selNode = GetAsDOMNode(parent);
>        selOffset = offset + 1;
>      }
>    }
> -  *aHandled = true;
> -  return SplitParagraph(GetAsDOMNode(aPara), sibling, aSelection,
> +  EditActionResult result(
> +    SplitParagraph(GetAsDOMNode(aPara), sibling, aSelection,

Ah, I will file a new bug that SplitParagraph should replace nsIDOMNode with nsINode (nsIContent).

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8927377 [details]
Bug 1416099 - part 2: Rename |sibling| in HTMLEditRules::ReturnInParagraph() to |brNode|

https://reviewboard.mozilla.org/r/198618/#review203934

::: editor/libeditor/HTMLEditRules.cpp:6762
(Diff revision 1)
>      if (!aOffset) {
>        // is there a BR prior to it?
> -      sibling = htmlEditor->GetPriorHTMLSibling(node);
> -      if (!sibling ||
> -          !htmlEditor->IsVisibleBRElement(sibling) ||
> -          TextEditUtils::HasMozAttr(GetAsDOMNode(sibling))) {
> +      brNode = htmlEditor->GetPriorHTMLSibling(node);
> +      if (!brNode ||
> +          !htmlEditor->IsVisibleBRElement(brNode) ||
> +          TextEditUtils::HasMozAttr(GetAsDOMNode(brNode))) {

I will add nsINode version of HasMozAttr

Comment 13

[Makoto Kato [:m_kato]]

Comment on attachment 8927378 [details]
Bug 1416099 - part 3: Rename aPara of HTMLEditRules::ReturnInParagraph() to aParentDivOrP and make it and aSelection as references

https://reviewboard.mozilla.org/r/198620/#review203936

::: editor/libeditor/HTMLEditRules.cpp:6729
(Diff revision 1)
>    if (NS_WARN_IF(!mHTMLEditor)) {
>      return EditActionResult(NS_ERROR_NOT_AVAILABLE);
>    }
>  
>    nsCOMPtr<nsINode> node = do_QueryInterface(aNode);
> -  if (!aSelection || !aPara || !node) {
> +  if (!node) {

Use "if (!aNode)" and don't use QI (nsINode to nsINode)

Comment 14

[Makoto Kato [:m_kato]]

Comment on attachment 8927379 [details]
Bug 1416099 - part 4: Make HTMLEditRules::ReturnInParagraph() use EditorRawDOMPoint to store point to insert new <br> element before splitting the parent block

https://reviewboard.mozilla.org/r/198622/#review203938

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8927380 [details]
Bug 1416099 - part 5: Rename |selNode|, |selOffset| and |newSelNode| in HTMLEditRules::ReturnInParagraph() to proper names

https://reviewboard.mozilla.org/r/198624/#review203940

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 8927381 [details]
Bug 1416099 - part 6: Make HTMLEditRules::WillInsertBreak() use EditorDOMPoint to store selection start

https://reviewboard.mozilla.org/r/198626/#review203942

Comment 17

[Makoto Kato [:m_kato]]

Comment on attachment 8927382 [details]
Bug 1416099 - part 7: Make HTMLEditRules::ReturnInParagraph() use start of selection to split parent block instead of receiving the point from arguments

https://reviewboard.mozilla.org/r/198628/#review203944

::: editor/libeditor/HTMLEditRules.cpp:6740
(Diff revision 1)
>    if (NS_WARN_IF(!mHTMLEditor)) {
>      return EditActionResult(NS_ERROR_NOT_AVAILABLE);
>    }
>  
> -  nsCOMPtr<nsINode> node = do_QueryInterface(aNode);
> -  if (!node) {
> +  RefPtr<HTMLEditor> htmlEditor = mHTMLEditor;
> +

OK, OK.  node check is removed.  So please ignore previous commnet.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8347a35706c
part 1: Make HTMLEditRules::ReturnInParagraph() use EditActionResult r=m_kato
https://hg.mozilla.org/integration/autoland/rev/9d743e40fb77
part 2: Rename |sibling| in HTMLEditRules::ReturnInParagraph() to |brNode| r=m_kato
https://hg.mozilla.org/integration/autoland/rev/aaa09a3ca23c
part 3: Rename aPara of HTMLEditRules::ReturnInParagraph() to aParentDivOrP and make it and aSelection as references r=m_kato
https://hg.mozilla.org/integration/autoland/rev/874edea0da06
part 4: Make HTMLEditRules::ReturnInParagraph() use EditorRawDOMPoint to store point to insert new <br> element before splitting the parent block r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1b14ca996bea
part 5: Rename |selNode|, |selOffset| and |newSelNode| in HTMLEditRules::ReturnInParagraph() to proper names r=m_kato
https://hg.mozilla.org/integration/autoland/rev/68a11434e53c
part 6: Make HTMLEditRules::WillInsertBreak() use EditorDOMPoint to store selection start r=m_kato
https://hg.mozilla.org/integration/autoland/rev/a6cae052d973
part 7: Make HTMLEditRules::ReturnInParagraph() use start of selection to split parent block instead of receiving the point from arguments r=m_kato

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/e8347a35706c
https://hg.mozilla.org/mozilla-central/rev/9d743e40fb77
https://hg.mozilla.org/mozilla-central/rev/aaa09a3ca23c
https://hg.mozilla.org/mozilla-central/rev/874edea0da06
https://hg.mozilla.org/mozilla-central/rev/1b14ca996bea
https://hg.mozilla.org/mozilla-central/rev/68a11434e53c
https://hg.mozilla.org/mozilla-central/rev/a6cae052d973