Closed Bug 1445929 Opened 2 years ago Closed 2 years ago

Editor(Raw)DOMPoint::IsEndOfContainer() doesn't work if container is a text node

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1445929 - Make Editor(Raw)DOMPoint::IsEndOfContainer() check if mParent is container node before checking mChild
59 bytes, text/x-review-board-request
m_kato
: review+
jcristau
: approval-mozilla-beta+
Details 
>  bool
>  IsEndOfContainer() const
>  {
> ...
>    if (mIsChildInitialized) {
>      if (!mChild) {
>        NS_WARNING_ASSERTION(!mOffset.isSome() ||
>                             mOffset.value() == mParent->Length(),
>          "If mOffset was initialized, it should be length of the container");
>        return true;
>      }
>      NS_WARNING_ASSERTION(!mOffset.isSome() ||
>                           mParent->GetChildAt_Deprecated(mOffset.value()) == mChild,
>        "If mOffset and mChild are mismatched");
>      return false;
>    }
>    MOZ_ASSERT(mOffset.isSome());
>    return mOffset.value() == mParent->Length();
>  }

mIsChildInitialized may be true even if mParent is a text node. Therefore, if mIsChildInitialized is true and mParent is a text node, this always returns true because mChild is always nullptr.
Unfortunately, I found this bug when I work on another bug. I'm not sure how to meet actual symptom.
Comment on attachment 8959173 [details]
Bug 1445929 - Make Editor(Raw)DOMPoint::IsEndOfContainer() check if mParent is container node before checking mChild

https://reviewboard.mozilla.org/r/228032/#review234072
Attachment #8959173 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/6d25e14fec76
Make Editor(Raw)DOMPoint::IsEndOfContainer() check if mParent is container node before checking mChild r=m_kato
https://hg.mozilla.org/mozilla-central/rev/6d25e14fec76
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8959173 [details]
Bug 1445929 - Make Editor(Raw)DOMPoint::IsEndOfContainer() check if mParent is container node before checking mChild

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1419745.

[User impact if declined]:
Not sure, we don't have actual bug report.

The bug is, IsEndOfContainer() returns true if Editor(Raw)DOMPoint's container is text node or something other data nodes and offset is even not same as length of the text node. So, callers of IsEndOfContainer() may set the container to a text node, the check won't work as expected.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes since this bug blocked to fix bug 1445569.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Before IsEndOfContainer() does something complicated, this patch inserts an early-return block for non-container nodes such as text node.

[String changes made/needed]:
No.
Attachment #8959173 - Flags: approval-mozilla-beta?
If there's no known user impact, and this is already on release, what's the downside of letting this ride the trains?
Flags: needinfo?(masayuki)
(In reply to Julien Cristau [:jcristau] from comment #9)
> If there's no known user impact, and this is already on release, what's the
> downside of letting this ride the trains?

We still don't have enough testcases for editor module. But this is actual regression of API in editor. So, callers of this method in editor assumes that it checks if the point is end of a node even when the node is a text node.

Unfortunately, there are a lot of callers:
https://dxr.mozilla.org/mozilla-beta/search?q=IsEndOfContainer&redirect=false
And some of them look treating DOM point in text node (e.g., EditorBase::DeleteSelectionAndPrepareToCreateNode(), HTMLEditRules::ReturnInParagraph()). So, I believe that in some situation, this bug causes something odd behavior.
Flags: needinfo?(masayuki)
Comment on attachment 8959173 [details]
Bug 1445929 - Make Editor(Raw)DOMPoint::IsEndOfContainer() check if mParent is container node before checking mChild

editor regression fix, beta60+
Attachment #8959173 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.