1658302 - Changing the `placeholder` property of the input cause loss of editing history.

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[nayinain]

Attachment: testcase.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:81.0) Gecko/20100101 Firefox/81.0

Steps to reproduce:

1. Open the testcase.html
2. Delete the text in input 1 or textarea 1.
3. Undo (<kbd>Ctrl + Z</kbd>) the changes.

Actual results:

Cannot undo changes.

Expected results:

Can undo the changes.

Comment 1

[nayinain]

The issue already exists in Firefox 4.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

:nayinain, if you think that's a regression, then could you try to find a regression range in using for example mozregression?

Comment 3

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

It seems that editor instance is recreated when placeholder attribute is changed.
https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/dom/html/HTMLInputElement.cpp#5181-5182
If so, editor is recreated without previous transaction.

emilio: Any ideas?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Changing the placeholder attribute reconstruct the layout frame.

Same seems happens if instead of changing placeholder I toggle .style.display, fwiw.

We don't seem to preserve the editing history across reframes, which seems a bit unexpected? Masayuki, do you know what is the background for this if any?

For this specific case, we probably can avoid reframing by incrementally updating the textnode in nsTextControlFrame::CreatePlaceholderIfNeeded (and only reframe if it's an addition / removal). But the root cause is ^ I think.

Comment 5

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

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

We don't seem to preserve the editing history across reframes, which seems a bit unexpected? Masayuki, do you know what is the background for this if any?

As this bug reported, I feel it's unexpected behavior setting placeholder value resets editor transactions (if I were a web app developer).

For this specific case, we probably can avoid reframing by incrementally updating the textnode in nsTextControlFrame::CreatePlaceholderIfNeeded (and only reframe if it's an addition / removal). But the root cause is ^ I think.

Yeah, I also guessed that the root cause is difficult to fix. I think that we don't need to fix only specific cases. So, perhaps, we should do it if actual web-compat issue is reported.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1658302 - Don't reframe for changes to the placeholder attribute value. r=masayuki

We still reframe for additions / removals of the attribute because that
makes us create the placeholder <div>. We could avoid it if we created
it independently of the presence of the attribute but that seems like it
could regress perf for the case where there's no placeholder attribute,
which is probably common enough.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1658302 - Remove useless reframe when allowdir or webkitdirectory are used on <input type="file">. r=masayuki

There's nothing in nsFileControlFrame that would change the frame tree
depending on these attributes.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e94014166b3
Don't reframe for changes to the placeholder attribute value. r=masayuki

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/0e94014166b3

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/4a18696ce81f
Backed out changeset 0e94014166b3 for causing bug 1662483

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa4266b83461
Don't reframe for changes to the placeholder attribute value. r=masayuki

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/25348 for changes under testing/web-platform/tests

Comment 13

[Razvan Maries]

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

Comment 14

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

Upstream PR merged by moz-wptsync-bot