Closed Bug 1415445 Opened 2 years ago Closed 2 years ago

Methods to create element should use RawRangeBoundary, RangeBoundary, EditorRawDOMPoint or EditorDOMPoint

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(4 files)

This is a follow up bug of bug 1407854.

Creating element methods should use RawRangeBoundary, RangeBoundary, EditorRawDOMPoint or EditorDOMPoint.
Comment on attachment 8926727 [details]
Bug 1415445 - part 1: CreateElementTransaction should use EditorRawDOMPoint and RangeBoundary

https://reviewboard.mozilla.org/r/197968/#review203202

::: editor/libeditor/CreateElementTransaction.cpp:78
(Diff revision 1)
>    // Try to insert formatting whitespace for the new node:
>    mEditorBase->MarkNodeDirty(GetAsDOMNode(mNewNode));
>  
>    // Insert the new node
> -  ErrorResult rv;
> -  if (mOffsetInParent == -1) {
> +  ErrorResult error;
> +  InsertNewNode(error);

If InsertNewNode returns error and GetShouldTxnSetSelection returns error, ErrorResult calls assertion because error isn't handled.  So you must handle error for InsertNewNode.
Attachment #8926727 - Flags: review?(m_kato) → review+
Comment on attachment 8926728 [details]
Bug 1415445 - part 2: EditorBase::CreateTxnForCreateElement() should take EditorRawDOMPoint for insertion point

https://reviewboard.mozilla.org/r/197970/#review203204
Attachment #8926728 - Flags: review?(m_kato) → review+
Comment on attachment 8926729 [details]
Bug 1415445 - part 3: nsIEditActionListener's WillCreateElement() and DidCreateElement() should take child node at insertion point or new node itself rather than the container node and offset in it

https://reviewboard.mozilla.org/r/197972/#review203220
Attachment #8926729 - Flags: review?(m_kato) → review+
Comment on attachment 8926730 [details]
Bug 1415445 - part 4: EditorBase::CreateNode() should take EditorRawDOMPoint as insertion point instead of a set of container, child and offset of the child in the container

https://reviewboard.mozilla.org/r/197974/#review203226

::: editor/libeditor/HTMLEditRules.cpp:6978
(Diff revision 1)
>            nsAtom* listAtom = nodeAtom == nsGkAtoms::dt ? nsGkAtoms::dd
>                                                          : nsGkAtoms::dt;
> +          MOZ_DIAGNOSTIC_ASSERT(itemOffset != -1);
> +          EditorRawDOMPoint atNextListItem(list, aListItem.GetNextSibling(),
> +                                           itemOffset + 1);
>            nsCOMPtr<Element> newListItem =

nits: RefPtr<Element> ...
Attachment #8926730 - Flags: review?(m_kato) → review+
Comment on attachment 8926727 [details]
Bug 1415445 - part 1: CreateElementTransaction should use EditorRawDOMPoint and RangeBoundary

https://reviewboard.mozilla.org/r/197968/#review203202

> If InsertNewNode returns error and GetShouldTxnSetSelection returns error, ErrorResult calls assertion because error isn't handled.  So you must handle error for InsertNewNode.

Nice catch!

I wonder, InsertNewNode() should call GetChildAtOffset() to make sure that mPointToInser.mRef is stored since at redoing, the DOM tree might be different. E.g., JS inserted something after undo.  However, let's watch if this causes regressions.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ff1aff2fa5b6
part 1: CreateElementTransaction should use EditorRawDOMPoint and RangeBoundary r=m_kato
https://hg.mozilla.org/integration/autoland/rev/80b30e892162
part 2: EditorBase::CreateTxnForCreateElement() should take EditorRawDOMPoint for insertion point r=m_kato
https://hg.mozilla.org/integration/autoland/rev/57ed7f042212
part 3: nsIEditActionListener's WillCreateElement() and DidCreateElement() should take child node at insertion point or new node itself rather than the container node and offset in it r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4be8afd89645
part 4: EditorBase::CreateNode() should take EditorRawDOMPoint as insertion point instead of a set of container, child and offset of the child in the container r=m_kato
You need to log in before you can comment on or make changes to this bug.