1484092 - Drop "namedanchor" support from nsIHTMLEditor::CreateElementWithDefaults(), nsIHTMLEditor::GetSelectedElement() and nsIHTMLEditor::GetElementOrParentByTagName()

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

nsIHTMLEditor::CreateElementWithDefaults() and nsIHTMLEditor::GetSelectedElement() treats "namedanchor" as <a name="..."> element. However, it seems that this is legacy name since there is another name, "anchor", and only "anchor" is used in comm-central and BlueGriffon. Due to not defined as an Atom, it does not make sense to keep supporting this value.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

nsIHTMLEditor::GetElementOrParentByTagName() is the last user of IsNamedAnchorTag(const nsAString&) and this is not used with "namedanchor" actually. So, we can completely drop "namedanchor".

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1484092 - part 1: Make HTMLEditor::GetElementOrParentByTagName() use nsAtom for the tag name

HTMLElementOrParentByTagName() is the last user of IsLinkTag(const nsAString&)
and IsNamedAnchorTag(const nsAString&).  For making their maintenance easier,
let's make GetElementOrParentByTagName() take const nsAtom& for tag name.

GetElementOrParentByTagName() has two functions, one is looking for an element
starting from a node.  The other is, if the start node is nullptr, it retrieves
anchor node of Selection as start node.  Therefore, this patch splits the
first part to GetElementOrParentByTagNameInternal().  Then, creates its
wrapper which retrieves anchor of Selection automatically,
GetElementOrParentByTagNameAtSelection().

Additionally, this patch makes all internal callers of HTMLEditor use
GetElementOrParentByTagNameInternal() or
GetElementOrParentByTagNameAtSelection() directly.  Then, public method,
GetElementOrParentByTagName() is called only by outer classes.

Note that some callers use both GetElementOrParentByTagNameInternal()
and GetElementOrParentByTagNameAtSelection() since they don't check whether
setting node is nullptr.  They may be bug of them.  We should investigate
the API callers later.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1484092 - part 2: Drop supporting "namedanchor" special element name from nsIHTMLEditor::GetSelectedElement(), nsIHTMLEditor::GetElementOrParentByTagName() and nsIHTMLEditor::CreateElementWithDefaults()

Nobody (including comm-central and BlueGriffon) does not use "namedanchor"
special element name with those XPCOMs.  Of course, our internal callers too.
Therefore, we can drop.

Note that there is no static Atom for this, so, keeping it makes unnecessary
runtime cost for Firefox users.

This could cause breaking some legacy add-ons for Thunderbird.  However,
they can use "anchor" special element name for same purpose.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1484092 - part 3: IsLinkTag() and IsNamedAnchorTag() should compare with nsGkAtoms

The methods compared with const characters since we've supported "namedanchor"
which is not in nsGkAtoms.  Now, it's dropped so that we can compare given
atom with nsGkAtoms.

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 9001862 [details]
Bug 1484092 - part 1: Make HTMLEditor::GetElementOrParentByTagName() use nsAtom for the tag name

Makoto Kato [:m_kato] has approved the revision.

Comment 6

[Makoto Kato [:m_kato]]

Comment on attachment 9001863 [details]
Bug 1484092 - part 2: Drop supporting "namedanchor" special element name from nsIHTMLEditor::GetSelectedElement(), nsIHTMLEditor::GetElementOrParentByTagName() and nsIHTMLEditor::CreateElementWithDefaults()

Makoto Kato [:m_kato] has approved the revision.

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 9001864 [details]
Bug 1484092 - part 3: IsLinkTag() and IsNamedAnchorTag() should compare with nsGkAtoms

Makoto Kato [:m_kato] has approved the revision.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0b14e8711df
part 1: Make HTMLEditor::GetElementOrParentByTagName() use nsAtom for the tag name r=m_kato

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10fdd041f1b5
part 2: Drop supporting "namedanchor" special element name from nsIHTMLEditor::GetSelectedElement(), nsIHTMLEditor::GetElementOrParentByTagName() and nsIHTMLEditor::CreateElementWithDefaults() r=m_kato

Comment 10

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (Bug 1484092) for build bustages in builds/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:100:53 on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=10fdd041f1b5ef4ab557d6d6d7c4b405ae55764b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=194500093

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=194500093&repo=autoland&lineNumber=21024

Backout: https://hg.mozilla.org/integration/autoland/rev/32edecc9301f9c8aeaf511809a084b9482aa7e9d

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, the line will be overwritten by the next patch. Sorry for the mistake. And I really hope Lando support to land multiple patches once...

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/717fb8632016
part 1: Make HTMLEditor::GetElementOrParentByTagName() use nsAtom for the tag name r=m_kato

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm... Lando failed to rebase at part 2 so that only part 1 landed first... Sigh...

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2a95c8855db
part 2: Drop supporting "namedanchor" special element name from nsIHTMLEditor::GetSelectedElement(), nsIHTMLEditor::GetElementOrParentByTagName() and nsIHTMLEditor::CreateElementWithDefaults() r=m_kato

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b5906a29b0c
part 3: IsLinkTag() and IsNamedAnchorTag() should compare with nsGkAtoms r=m_kato

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/717fb8632016
https://hg.mozilla.org/mozilla-central/rev/a2a95c8855db
https://hg.mozilla.org/mozilla-central/rev/4b5906a29b0c

Comment 17

[Chris Mills [:cmills]]

I've updated the following page:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIHTMLEditor

to remove mention of named anchors from the sections on CreateElementWithDefaults(), GetSelectedElement(), and GetElementOrParentByTagName(). Let me know if you need anything else.