1510183 - It's not possible to erase a web link when it is create

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[[e-mail]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0 SeaMonkey/2.49.4

Steps to reproduce:

(sorry but I'm using a french version of thunderbird 60.3.1 (32 bits))
In a new email, you create a Web link on words with the menu "insert" "link ..." you insert the Web address in the box "link properties" and you confirm with "OK
When you edit this link again with a double-click on the words, the box "link properties" opens. Inside, you delete the link (the field becomes white) and you confirm with "OK" but the link is not deleted (the words are always in blue and underlined)


Actual results:

the link is not deleted

Comment 1

[Wayne Mery (:wsmwk)]

I can reproduce this, and bug 1510210 using 60.3.1
I didn't test in safe mode

Comment 2

[Jorg K (CEST = GMT+2)]

Aceman, can you check this for us.

Comment 3

[:aceman]

Just inputing the link into the dialog throws:
NS_ERROR_FILE_NOT_FOUND:  UnifiedComplete.js:2528 (mozilla/toolkit/components/places/UnifiedComplete.js)

Clicking "Advanced edit" throw:
TypeError: right-hand side of 'in' should be an object, got null in text.xml:31:15

But those may not cause the bug here. I'll continue investigating.

Comment 4

[:aceman]

So if the href value is removed, the editor calls:
EditorRemoveTextProperty("href", "");
which calls:

which ends up here:
https://searchfox.org/comm-central/source/mozilla/editor/libeditor/HTMLStyleEditor.cpp#1325

And then runs through all of HTMLEditor::RemoveInlinePropertyInternal() until the final NS_OK, so no errors.

There were recent changes in the file in bug 1497746, but if you say it is already in TB60, it can't be caused by those.

Other candidate is Bug 1482015 which actually creates the HTMLEditor::RemoveInlinePropertyInternal() function (on 13th Aug 2018).

Did this work before TB60?

Comment 5

[Jorg K (CEST = GMT+2)]

I'm sure Alice can find the regression. Yes, worked with TB 52.

Comment 6

[Alice0775 White]

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=96593a2f19eff987b165009a0be750d24a0a9270&tochange=96593a2f19eff987b165009a0be750d24a0a9270
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=351c75ab74c9a83db5c0662ba271b49479adb1f1&tochange=ac93fdadf1022211eec62258ad22b42cb37a6d14

Comment 7

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

I also reached bug 1427060 from source code.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf4dccca4ea34b56b26a4b74966a8fac9a13f4d8

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c4521e1f310777662438b0a003862cc102d41fe

Comment 10

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

Attachment: Bug 1510183 - Make HTMLEditor treat empty string attribute of style as nullptr of nsAtom rather than nsGkAtoms::_empty

After fixing bug 1427060, HTMLEditor treats attribute of style as nullptr.
However, if empty string is used to call NS_Atomize(), it returns
nsGkAtoms::_empty.  Therefore, HTMLEditor fails to check whether attribute is
specified or not with nullptr check since some root callers sets
nsGkAtoms::_empty instead of nullptr.

This patch makes HTMLEditor always use nullptr for empty string of attribute
of style with wrapping NS_Atomize() with AtomzieAttribute().  Additionally,
for safer change, this patch makes PropItem and TypeInState treat
nsGkAtom::_empty as nullptr.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b096dc953832
Make HTMLEditor treat empty string attribute of style as nullptr of nsAtom rather than nsGkAtoms::_empty r=m_kato

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/b096dc953832

Comment 13

[Jorg K (CEST = GMT+2)]

Masayuki-san, any chance for a backport to mozilla-esr60?

Comment 14

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41898826058836ad55366bd4e6da3095590fd708

Comment 15

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af68531d03a5b9500cfd45b4cd715d51e59681a5

Comment 16

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

Attachment: Patch for ESR60

Here is. If this is not enough to fix on Thunderbird, please file another bug.

Comment 17

[Jorg K (CEST = GMT+2)]

I'll try it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0343cdf21428da5620ac409e2268ae543c95e8b
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f97496548d410cefe3ad54017777b03c93505bd6

Comment 18

[Jorg K (CEST = GMT+2)]

That try build didn't work. We use a trick to base the Mozilla repo on try instead of mozilla-esr60, and that trick didn't work here.

Comment 19

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/5311a17b136e5e3370f686ffa18fe2deebc44cfe on THUNDERBIRD_60_VERBRANCH

Try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=94b2f5ca4491db222e0585dd5e0864ff5cd5c8f6
This time it worked, I'll check it when it's done.

Comment 20

[Jorg K (CEST = GMT+2)]

Works for me, thanks Masayuki-san! I'll include it in TB 60.3.3 or TB 60.4.