1906559 - createLink command on an image with display inline-block does not put href on link

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Andrew Nicols]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0.0.0 Safari/537.36

Steps to reproduce:

1. Create a page with body content:

<div contenteditable="true">
<img src="https://www.mozilla.org/media/protocol/img/logos/mozilla/logo-word-hor.e20791bb4dd4.svg" alt="Mozilla" width="112" height="32" style="display:inline-block" >
</div>

2. Open the developer console and inspect the image
3. In the browser click on the image
4. Back in the developer console run:

    ```js
document.execCommand('createLink', false, 'https://mozilla.org/');

Actual results:

The image was wrapped with with:

<a _moz_dirty="">
    <!-- ... -->
</a>

Expected results:

The anchor should have included the href too.

Comment 1

[Andrew Nicols]

Things I've discovered whilst looking into this bug:

• It started in Firefox 110.0b1 (Was not present in 109.0.2)
• I don't see the same issue with display: block or display: inline

Comment 2

[Andrew Nicols]

Correction. If the display is block then no anchor is added at all.

Comment 3

[BugBot [:suhaib / :marco]]

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Core & HTML' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 4

[Sean Feng [:sefeng211]]

I can take a look

Comment 5

[Andrew Nicols]

Is there anything I can do to help with this one? This affects us quite badly and I can't find a reason why this would be a correct or documented behaviour.

Comment 6

[Sean Feng [:sefeng211]]

I dug a bit into this, and it turned out we were able to create the anchor element with the correct href correctly, however the anchor element got deleted right away by https://searchfox.org/mozilla-central/rev/02a4a649ed75ebaf3fbdf301c3d3137baf6842a1/editor/libeditor/HTMLEditSubActionHandler.cpp#722 . Feels like we shouldn't delete this anchor.

Masayuki, thoughts on this?

Comment 7

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

Yeah, this is obviously a bug. Well, but if the code runs you mentioned in comment 6, the <a href> element itself should be removed not only href attribute. Additionally, here should return false because for returning true, it needs to be considered as empty. However, if it reaches the <img> in the <a href>, IsEmpty() should return false because <img> is not a container.

Comment 8

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1730429

Comment 9

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

It seems that there are 2 bugs at least. One is, we use HTMLEditor::CreateElementWithDefaults which adds _moz_dirty attribute automatically. Then, HTMLEditor::InsertLinkAroundSelectionAsAction() calls SetInlinePropertiesAsSubAction() with the array having 2 elements, one for _moz_dirty and the other for href. Then, when AutoInlineStyleSetter::ApplyStyle() handles the first element, AutoInlineStyleSetter::OnHandled is called with the <img>. Then, OnHandled sets the range to the collapsed range in the <img>. Then, the second loop which is for handling href tries to work with collapsed range which won't work as expected because it just puts href style into the cache for next typing. So, we should fix the OnHandle's bug and avoid to add the unnecessary _moz_dirty attribute to the array.

Comment 10

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

Attachment: Bug 1906559 - Make `AutoInlineStyleSetter::OnHandled()` never set point in `aContent` if it's not a container r=m_kato!

When HTMLEditor::InsertLinkAroundSelectionAsAction() calls
HTMLEditor::SetInlinePropertiesAsSubAction(), it adds 2 elements to the array.
One is for _moz_dirty attribute and the other is for href attribute because
InsertTagCommand::DoCommandParam() uses
HTMLEditor::CreateElementWithDefaults().

Then, HTMLEditor::SetInlinePropertiesAsSubAction() wraps the <img> into
<a _moz_dirty=""> with calling AutoInlineStyleSetter::ApplyStyle(). Then,
it calls OnHandle() with the <img> and it collapse the applied range into
the <img>. Therefore, when SetInlinePropertiesAsSubAction() handles href,
there is only collapsed range and it just puts the style into the cache for
applying the style when the user types new text.

So, first, OnHandle() should not set the boundary points of the applied range
into non-container node. And also InsertLinkAroundSelectionAsAction() should
ignore _moz_dirty attribute because if it's truly required,
AutoInlineStyleSetter should set the attribute to every new element.

Note that this causes new failure in editing/run/fontname.html?1001-200,
but it was accidentally passed. The case is, queryCommandValue("fontname")
result after calling execCommand("fontname", false, "sans-serif") for
foo<tt>{<br></tt>}bar. The result of the DOM tree is
foo<tt><font face="sans-serif"><br></font></tt>bar and Selection was
collapsed in the <br> before, but with this patch, the <br> is selected.
Therefore, the result is changed but similar cases in the tests fail too. So I
believe that it accidentally passed with odd Selection.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a33d5e7176b
Make `AutoInlineStyleSetter::OnHandled()` never set point in `aContent` if it's not a container r=m_kato

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/47661 for changes under testing/web-platform/tests

Comment 13

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/7a33d5e7176b

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot