Closed
Bug 1445929
Opened 3 years ago
Closed 3 years ago
Editor(Raw)DOMPoint::IsEndOfContainer() doesn't work if container is a text node
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | wontfix |
firefox60 | --- | fixed |
firefox61 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
(Keywords: regression)
Attachments
(1 file)
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.
Assignee | ||
Comment 1•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a03387931aea08b41c486d10d14510bc2bd3dd11
Assignee | ||
Comment 2•3 years ago
|
||
Unfortunately, I found this bug when I work on another bug. I'm not sure how to meet actual symptom.
Comment hidden (mozreview-request) |
Comment 4•3 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 6•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5800617915bbfe3b299ee41c1e2e2ff5357fa949
Comment 7•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6d25e14fec76
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•3 years ago
|
Assignee | ||
Comment 8•3 years ago
|
||
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?
Comment 9•3 years ago
|
||
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)
Assignee | ||
Comment 10•3 years ago
|
||
(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 11•3 years ago
|
||
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+
Comment 12•3 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/ee4ed3b4b945
You need to log in
before you can comment on or make changes to this bug.
Description
•