Don't use nsIDOM* in ConfirmSelectionInBody

RESOLVED FIXED in Firefox 55

Status

()

Core
Editor
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

55 Branch
mozilla55
Unspecified
All
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
When using DOM API into contenteditable's element, mozilla::HTMLEditRules::DocumentModifiedWorker() is always called.  But 40%-50% of DocumentModifiedWorker spends into mozilla::HTMLEditRules::ConfirmSelectionInBody().

ConfirmSelectionInBody uses nsIDOM* and TextEditUtils::IsBody doesn't use nsIAtom.  So I would like to clean up it
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8858233 [details]
Bug 1356496 - Don't use nsIDOM* in ConfirmSelectionInBody.

https://reviewboard.mozilla.org/r/130210/#review132872

Temporarily, marking as r- because I'd like to check the approach which you'd take.

::: editor/libeditor/HTMLEditRules.cpp:8080
(Diff revision 1)
> -  nsCOMPtr<nsIDOMElement> rootElement = do_QueryInterface(mHTMLEditor->GetRoot());
> -  NS_ENSURE_TRUE(rootElement, NS_ERROR_UNEXPECTED);
> +  RefPtr<Element> rootElement = mHTMLEditor->GetRoot();
> +  if (NS_WARN_IF(!rootElement)) {
> +    return NS_ERROR_UNEXPECTED;
> +  }

This is probably possible if all nodes under the document node is removed by JS. It might be better to editor work with only when the document has <body> and check it earlier.

::: editor/libeditor/HTMLEditRules.cpp:8102
(Diff revision 1)
> -  temp = selNode;
> +  nsINode* temp = selNode;
>  
>    // check that selNode is inside body
> -  while (temp && !TextEditUtils::IsBody(temp)) {
> -    temp->GetParentNode(getter_AddRefs(parent));
> +  while (temp && !temp->IsHTMLElement(nsGkAtoms::body)) {
> +    temp = temp->GetParentNode();
> -    temp = parent;
>    }
>  
>    // if we aren't in the body, force the issue
>    if (!temp) {
>  //    uncomment this to see when we get bad selections
>  //    NS_NOTREACHED("selection not in body");
>      selection->Collapse(rootElement, 0);
> +    // XXX selection is collapsed, so should we check selection end?
>    }

So, this is possible case too. Actually, our tests have such case. AFAIK, we cannot edit anything in such HTML editor but IME is enabled. So, IME is confused with querying contents...

::: editor/libeditor/HTMLEditRules.cpp:8102
(Diff revision 1)
> -  temp = selNode;
> +  nsINode* temp = selNode;
>  
>    // check that selNode is inside body
> -  while (temp && !TextEditUtils::IsBody(temp)) {
> -    temp->GetParentNode(getter_AddRefs(parent));
> +  while (temp && !temp->IsHTMLElement(nsGkAtoms::body)) {
> +    temp = temp->GetParentNode();
> -    temp = parent;
>    }

I wonder, isn't it better to use nsContentUtils::ContentIsDescendantOf() with retrieving <body> from document.body? This can decide it valid if there are two or more body elements. But I guess that HTMLEditor doesn't assume it works with non-primary <body>.

::: editor/libeditor/HTMLEditRules.cpp:8114
(Diff revision 1)
>    // if we aren't in the body, force the issue
>    if (!temp) {
>  //    uncomment this to see when we get bad selections
>  //    NS_NOTREACHED("selection not in body");
>      selection->Collapse(rootElement, 0);
> +    // XXX selection is collapsed, so should we check selection end?

Not necessary to check anymore. You should return from here.

::: editor/libeditor/HTMLEditRules.cpp:8118
(Diff revision 1)
>    rv = EditorBase::GetEndNodeAndOffset(selection,
>                                         getter_AddRefs(selNode), &selOffset);
>    NS_ENSURE_SUCCESS(rv, rv);
>    temp = selNode;
>  
>    // check that selNode is inside body
> -  while (temp && !TextEditUtils::IsBody(temp)) {
> -    rv = temp->GetParentNode(getter_AddRefs(parent));
> +  while (temp && !temp->IsHTMLElement(nsGkAtoms::body)) {
> +    temp = temp->GetParentNode();
> -    temp = parent;
>    }
>  
>    // if we aren't in the body, force the issue
>    if (!temp) {
>  //    uncomment this to see when we get bad selections
>  //    NS_NOTREACHED("selection not in body");
>      selection->Collapse(rootElement, 0);
>    }

EditorBase::GetStartNodeAndOffset() and EditorBase::GetEndNodeAndOffset() use only first range (Selection.getRangeAt(0)). So, checking with nsContentUtils::ContentIsDescendantOf() and nsRange::GetCommonAncestor() make this method simpler. However, I'm not sure if it's faster the current implementation because nsRange::GetCommonAncestor() calls nsContentUtils:::GetCommonAncestor() but looks like that it's not faster than this...
Attachment #8858233 - Flags: review?(masayuki) → review-

Comment 3

a year ago
mozreview-review
Comment on attachment 8858233 [details]
Bug 1356496 - Don't use nsIDOM* in ConfirmSelectionInBody.

https://reviewboard.mozilla.org/r/130210/#review132892

::: editor/libeditor/HTMLEditRules.cpp:8102
(Diff revision 1)
> -  temp = selNode;
> +  nsINode* temp = selNode;
>  
>    // check that selNode is inside body
> -  while (temp && !TextEditUtils::IsBody(temp)) {
> -    temp->GetParentNode(getter_AddRefs(parent));
> +  while (temp && !temp->IsHTMLElement(nsGkAtoms::body)) {
> +    temp = temp->GetParentNode();
> -    temp = parent;
>    }

Ah, but it's possible to edit in any body elements. https://jsfiddle.net/d_toybox/4q3jnt66/ (It creates:

<body id="primary">
  <body id="tertiary"></body>
</body>
<body id="secondary"></body>

And I can edit in any <body> element. So, we cannot use nsContentUtils::ContentIsDescendantOf() with document.body. Sigh.

Comment 4

a year ago
mozreview-review
Comment on attachment 8858233 [details]
Bug 1356496 - Don't use nsIDOM* in ConfirmSelectionInBody.

https://reviewboard.mozilla.org/r/130210/#review132894

Giving r+ due to changing the logic is risky. But please add return statement at your XXX comment.
Attachment #8858233 - Flags: review- → review+

Comment 5

a year ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d29bf11528ec
Don't use nsIDOM* in ConfirmSelectionInBody. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/d29bf11528ec
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.