Closed Bug 1356496 Opened 7 years ago Closed 7 years ago

Don't use nsIDOM* in ConfirmSelectionInBody

Categories

(Core :: DOM: Editor, enhancement)

Product:
Core
Component:
DOM: Editor
Version:
55 Branch
Platform:
Unspecified
All
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

Details

Attachments

(1 file)

Bug 1356496 - Don't use nsIDOM* in ConfirmSelectionInBody.
59 bytes, text/x-review-board-request
masayuki
: review+
Details 
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 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 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 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+
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
Closed: 7 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: