787434 - Insert HTML deletes "empty" tag pairs and their content/layout (provided by <style>)

Status: RESOLVED FIXED

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

Description

[Michel Merlin]

Attachment: Banner-Logo-Banner(Bug_Demo_Thunderbird_bugzilla.mozilla.org).zip

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.1 (KHTML, like Gecko) Chrome/21.0.1180.89 Safari/537.1

Steps to reproduce:

Open "Thunderbird > Write > Insert > Html..." then paste:

<STYLE>
BODY {OVERFLOW: hidden}
#BannerFull {WIDTH: 100%; HEIGHT: 90px; BACKGROUND: lime}
A IMG {BORDER: 0}
a.Logo IMG {margin: 6px 8px;}
a.BannerRight {position:absolute; width:100%; height:90px;
  background: #98FB98 url(Banner728×90.jpg) no-repeat 0 0;}
</STYLE>
<DIV id=BannerFull title=BannerFull></DIV>
<DIV><A class=Logo href="http://www.company.com"><IMG height=78 alt=Logo 
src="Logo115×78.jpg" width=115></A><A class=BannerRight title=BannerRight 
href="http://www.sponsor.com"></A></DIV>

and click "Apply".


Actual results:

(1) The Logo is duly displayed (probably because the according A is filled with an IMG)

(2) The BannerFull and the BannerRight have been removed from the source (probably because the according DIV and A are empty).


Expected results:

A full-width 90px-high LIME banner should have been displayed ABOVE the Logo, and a full-width GREEN banner should have been displayed at the RIGHT of the Logo.

To check it live, extract the 3 files from the attached ZIP to a localfolder then open the "Banner-Logo-Banner.htm" file in Chrome, IE or Firefox.

Notes:

(1) In OE or TB, due to the image relative addresses, the images are replaced with placeholders of the same sizes.

(2) This works perfect down to the pixel level in Chrome (21.0.1180.89m) and in ie6 (ie6.0.2900.5512.xpsp_sp3_gdr.120504-1619). I did NOT check if Firefox displays the same flaw as Thunderbird.

Versailles, Fri 31 Aug 2012 17:22:00 +0200

Comment 1

[Alice0775 White]

Regression window
Good:
http://hg.mozilla.org/mozilla-central/rev/42b1ad712868
http://hg.mozilla.org/comm-central/rev/190843c8536a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110727 Thunderbird/8.0a1 ID:20110727030335
Bad:
http://hg.mozilla.org/mozilla-central/rev/d066929dd830
http://hg.mozilla.org/comm-central/rev/989daa36e011
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110728 Thunderbird/8.0a1 ID:20110728030011
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=42b1ad712868&tochange=d066929dd830
http://hg.mozilla.org/comm-central/pushloghtml?fromchange=190843c8536a&tochange=989daa36e011

Suspicious: Bug 460740

Comment 2

[(no longer active)]

Hmm, we need someone to fix this regression...

Comment 3

[Michel Merlin]

Alice0775 White 20:42 GMT, Ehsan Akhgari 20:46, did you run the test in the ZIP?

To make my writing clearer, my "Notes:" 1 (placeholders in OE or TB) and 2 (OK pixel level in Chrome and ie6, not tested in FF) were explanations for the *test* in the ZIP, NOT for the bug.

Versailles, Fri 31 Aug 2012 23:08:00 +0200

Comment 4

[Alice0775 White]

(In reply to Michel Merlin from comment #3)
> Alice0775 White 20:42 GMT, Ehsan Akhgari 20:46, did you run the test in the
> ZIP?

No, I tested with STR in Comment#0 in Thunderbird , not zip.

Comment 5

[(no longer active)]

I didn't personally try to reproduce the bug.  But I trust Alice.  :-)

Comment 6

[Alice0775 White]

I confirmed in local build, Landing Bug 489202 triggers this problem.

Comment 7

[Mark Banner (:standard8)]

Not tracking for Thunderbird when this hasn't been realised in close to a year afaik. Obviously, it'd be great to get this fixed though.

Comment 8

[Mark Banner (:standard8)]

Oh and from the comments/regression bug moving this to core/editor as this appears to be a gecko issue rather than Thunderbird specific.

Comment 9

[Joe Sabash [:JoeS1]]

xref bug 787673 may help here.

Comment 10

[Joe Sabash [:JoeS1]]

The fix checked in for bug 787673 did _not_ alleviate this bugs symptoms.

Comment 11

[Thomas D. (:thomas8)]

Interesting... so it's not only TB's {Delivery Format: Auto-Detect} which has an over-appetite for munging content-relevant tags, even the editor itself just eats up user's tags when they are wrongly deemed "empty" :(

It certainly hurts ux-trust when users spoon-feed their favourite mailer with delicate but well-formed HTML tags via Insert > HTML, and instead of digesting them appropriately, Editor just throws them away without trace or warning...

So that's dataloss, hence critical by definition (although perhaps not affecting too many users).
I'd love to see this fixed as a matter of principle... (not urgency).

Comment 12

[Thomas D. (:thomas8)]

Next steps for this bug:
- unwrap zip file and re-attach more accessibly; change test case image file names and their references to ascii, avoid special characters (test case breaks when unzipped on WinXP)
- add screenshots of testcase before and after munging by editor

Comment 13

[Michel Merlin]

I have stopped using TB years ago after being extremely impolitely and dishonestly treated by the omnipotent gurus at bugzilla.mozilla.org (they went as far as hiding my suggestions, opening another "bug" file, and "suggest" there my own ideas, taking credit for themselves), making obvious that Thunderbird would never get the fixes and improvements it needs to get out of its current state of a joke program. So please don't expect me to spend more time to correct Mozilla's (obviously intentional) stupidities.
(I just stumbled on some TB bugzilla cases by chance which prompted me to login and check my account)
Versailles, Sat 09 Apr 2016 19:06:20 +0200

Comment 14

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, 4 years untouched DOM/Storage bugs' priority.

If you have reason to believe this is wrong (especially for the severity), please write a comment and ni :jstutte.

Comment 15

[Thomas D. (:thomas8)]

For Thunderbird, this should certainly be double-checked before downgrading.

Comment 16

[Wayne Mery (:wsmwk)]

(In reply to Thomas D. (:thomas8) - [PTO 18 Dec - 03 Jan] from comment #15)

For Thunderbird, this should certainly be double-checked before downgrading.

Comment 17

[Thomas D. (:thomas8)]

(In reply to Michel Merlin from comment #0)

Steps to reproduce:

Open "Thunderbird > Write > Insert > Html..." then paste:
[HTML - snip]
and click "Apply".

Actual results:

(2) The BannerFull and the BannerRight have been removed from the source
(probably because the according DIV and A are empty).

Still reproduces exactly as described on TB 78.6.0 (64-bit) (Win10).
As reported, this will affect any Thunderbird user who injects HTML into composition using that menu item.
This is outright dataloss, not acceptable.

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Moving to S3 from the point of views for Firefox users.
Masayuki thought this may be a simple fix.

Comment 19

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

Yeah, probably, I got it.
https://treeherder.mozilla.org/jobs?repo=try&revision=57d49bd9e856999db51897d64c4ca1ec7584260e

Comment 20

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

Hmm, some tests which were added by bug 1123505 and bug 1127835 fail. I'll check the compatibility of the expectations vs. Chrome.

Comment 21

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

these tests expect that empty <ul> element which is copied from first or second range of Selection should not be pasted. However, when I paste the copied data created by attachment 8555283 [details] in Chrome, the empty <ul> element as is. For web-compat with Chrome, it's better to stop removing the incomplete elements, but keeping our behavior seems reasonable. Perhaps, we should stop inserting such incomplete element instead.

On the other hand, this test is created for detecting crash bug regression. So, changing the expectation must be fine.

Comment 22

[Michel Merlin]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #21)

For web-compat with Chrome, it's better to stop removing the incomplete elements, but keeping our behavior seems reasonable. Perhaps, we should stop **inserting** such incomplete element instead.

Beyond Chrome, it is compatible with all programs and people to make copies are... copies, i.e. identical to the original without a single character or coma changed.
Mozilla's unknown users have no reason to be less knowledgeable or smart or careful than Mozilla's developers, commentators or other staff or clients or visitors. In addition they are the ones on the work and on the scene so they are the most and best informed about the code at stake.
If you assume they are smart and careful enough to write code sans useless empty tags, then please also assume they are smart and careful enough that when they write an empty tag it is useful and justified.
In the case object of this thread, the initial code was carefully written and worked perfect in all browsers I tested (up to ie6!). Intentionally falsifying user's code is impossible to understand by civilized people.
In case you lack time for reading and understanding this complete, I resume:
A copy is NO MORE a copy when it gets altered. Please don't alter others's writings, including code sources.
Versailles, Fri 14 Jan 2022 20:03:10 +0100

Comment 23

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

Michel Merlin, I don't understand what you said, but I guess that you misunderstood of the intention of my previous comment. I didn't say to stop fixing this bug. The expectation of testcases which are broken by my patch is not compatible with Chrome, but reasonable for our builtin editor. Therefore, I'd like to keep the behavior even with fixing this bug (i.e., stop removing empty elements). Otherwise, we'd see odd behavior with incomplete elements like only <ul></ul> (i.e., no <li> elements) coming from script or clipboard which is not assumed by our builtin editor.

Comment 24

[Michel Merlin]

(In reply to Masayuki Nakano [:masayuki] comment #23 of Sun 16 jan 2022 22:02 GMT-8)

The expectation of testcases which are broken by my patch is not compatible with Chrome, but reasonable for our builtin editor.
Therefore, I'd like to keep the behavior even with fixing this bug (i.e., stop removing empty elements).
Otherwise, we'd see odd behavior with incomplete elements like only <ul></ul> (i.e., no <li> elements)
coming from script or clipboard which is not assumed by our builtin editor.
Thanks for replying, especially kindly and precisely, but I still disagree, fundamentally and strongly.
Editing is changing. Copying is NOT changing. Editing IS NOT Copying.
An alleged "copy" stops being one as soon as it includes an edit, as tiny as it can be. Editing something while copying it is betraying the author and should theoretically never be done; yes in real life it sometimes can be done, but extremely rarely, only when it is obvious and well verified that it doesn't hurt the truth of what the original was, and above all at least after making full sure that the author would agree.
Example 1 (yours):
the author of an <ul></ul> empty tag can have written it as a placeholder for an EVENTUAL addition of an unknown number of <li></li>s.
Example 2 (fictive):
Original: « The POTUS lives in Washinggton »
Correct Copy: « The POTUS lives in Washinggton (sic) »
Copy admissible ONLY IF you have made FULL sure that this was NOT an intentional or meaningful alteration, e.g. that nothing exists called "Washinggton" , that it was a typo, and above all that the author would fully agree: « The POTUS lives in Washington (one typo corrected) »
Among possible verifications, Google "Washinggton", with duly following the sublink to "Search instead for Washinggton", will show that even such an improbable alteration DOES HAVE possible and sensible reasons to exist - see e.g. What Does Washinggton Stand For - and that one should not too easily assume that the other is less knowledgeable or aware or careful than oneself.
Note. My present attempt at clarifying, while NOT necessary for the simple case I reported when creating the present Bug 787434 report, IS useful IMO for eventual similar cases: Copying EXCLUDES editing. Editing a copy is betraying author and generally is counterproductive to everyone
Versailles, Mon 17 Jan 2022 14:22:25 +0100

Comment 25

[Michel Merlin]

(In reply to Michel Merlin from comment #24)

(In reply to Masayuki Nakano [:masayuki] comment #23 of Sun 16 jan 2022 22:02 GMT-8)

The expectation of testcases which are broken by my patch is not compatible with Chrome, but reasonable for our builtin editor.
Therefore, I'd like to keep the behavior even with fixing this bug (i.e., stop removing empty elements).
Otherwise, we'd see odd behavior with incomplete elements like only <ul></ul> (i.e., no <li> elements)
coming from script or clipboard which is not assumed by our builtin editor.
Thanks for replying, especially kindly and precisely, but I still disagree, fundamentally and strongly.
Editing is changing. Copying is NOT changing. Editing IS NOT Copying.
An alleged "copy" stops being one as soon as it includes an edit, as tiny as it can be. Editing something while copying it is betraying the author and should theoretically never be done; yes in real life it sometimes can be done, but extremely rarely, only when it is obvious and well verified that it doesn't hurt the truth of what the original was, and above all at least after making full sure that the author would agree.
Example 1 (yours):
the author of an <ul></ul> empty tag can have written it as a placeholder for an EVENTUAL addition of an unknown number of <li></li>s.
Example 2 (fictive):
Original: « The POTUS lives in Washinggton »
Correct Copy: « The POTUS lives in Washinggton (sic) »
Copy admissible ONLY IF you have made FULL sure that this was NOT an intentional or meaningful alteration, e.g. that nothing exists called "Washinggton" , that it was a typo, and above all that the author would fully agree: « The POTUS lives in Washington (one typo corrected) »
Among possible verifications, Google "Washinggton", with duly following the sublink to "Search instead for Washinggton", will show that even such an improbable alteration DOES HAVE possible and sensible reasons to exist - see e.g. What Does Washinggton Stand For - and that one should not too easily assume that the other is less knowledgeable or aware or careful than oneself.
Note. My present attempt at clarifying, while NOT necessary for the simple case I reported when creating the present Bug 787434 report, IS useful IMO for eventual similar cases: Copying EXCLUDES editing. Editing a copy is betraying author and generally is counterproductive to everyone
Versailles, Mon 17 Jan 2022 14:22:25 +0100, edited (indents) 14:25:15

Comment 26

[Michel Merlin]

To anyone who has the power (or who would and could teach me how to do):
Sorry I don't understand how this site's indent system works.
My (accidentally posted by my error) comment 25 of Mon 17 Jan 2022 13:25:15 GMT should be removed;
My comment 24 of Mon 17 Jan 2022 13:22:25 GMT needs ONE little correction: after « not assumed by our builtin editor. », END the 1st indentation and REMOVE the start of a new one, thus showing more clearly that my reply starts with « Thanks for replying ».
My apologies again for the trouble.
Versailles, Mon 17 Jan 2022 14:34:45 +0100

Comment 27

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

This is now handled as a bug of editor. Even if Gecko changes the data putting onto the clipboard, same or similar data may come into Gecko's builtin editor. Therefore, we need to handle such odd data anyway. If you know actual scenario which are some bugs caused copying from Gecko and pasting to another app, please file new bugs for each case. Nobody should handle mixed issue in a bug.

Comment 28

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

Attachment: Bug 787434 - part 1: Make `HTMLEditor` stop removing empty elements of inserted content r=m_kato!

This is same issue as bug 1747008, but the remover is different method, that is
HTMLEditor::RemoveEmptyNodesIn called by
HTMLEditor::OnEndHandlingTopLevelEditSubActionInternal. It should be called
only when the flag which was added by bug 1747008 is set to false.

Comment 29

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

Attachment: Bug 787434 - part 2: Make `HTMLWithContextInserter::FragmentFromPasteCreator` delete incomplete list elements in the inserting fragment r=mbrodesser!

With the change of the previous patch, HTMLEditor won't delete empty elements
in the inserted HTML content. However, at bug 1123505, it intentionally tried
to delete empty list elements which have no list item elements since such
list elements won't be editable. Therefore, the following patch makes some
tests in test_copypaste.html fail.
https://searchfox.org/mozilla-central/rev/3de56eb5f266f523340e739ae1b53258e0a95dfe/dom/base/test/copypaste.js#343,360
Unfortunately, the broken behavior is compatible with Chrome (Chrome accept
list elements which have no list item elements), but it does not make sense for
Gecko's builtin editor. Therefore, I think that we should keep our traditional
behavior with deny-list.

This patch makes FragmentFromPasteCreator delete list elements which have no
list item elements and are empty from the inserting document fragment.

Depends on D136210

Comment 30

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

Attachment: Bug 787434 - part 3: Change the expectation of a test in copypaste.js r=mbrodesser!

It tries to test an assertion failure bug which was fixed by bug 1127835.
It expects that empty <a href="..."> element coming from the clipboard should
be deleted. However, this now mismatches with the new behavior which wants to
keep inline elements for web-compat and avoiding any dataloss. Therefore, we
need to change the expectation for conforming to the new behavior.

Depends on D136211

Comment 31

[Michel Merlin]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #27)

This is now handled as a bug of editor.

What is "This", which "bug", which "Editor"? The most urgent correction needed here is correcting my 3 posts of Mon 17 Jan 2022 13:22:25, 13:25:15, 13:34:45 (GMT).
Was this my error? or are you suggesting this is a bug in the editor that handles this current conversation? or in the editor that handles the COPY that was at stake (when inserting HTML into Mozilla Thunderbird or Firefox)?
Such imprecision seems to be the cause why we are apparently speaking of full different things and issues.
Now I don't want to complicate anything. So if my inputs are misunderstood or or not appreciated or ignored, so be it, better ignore me than adding any more to the troubles Thunderbird already suffers.
Versailles, Wed 19 Jan 2022 12:48:15 +0100

Comment 32

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

(In reply to Michel Merlin from comment #31)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #27)

This is now handled as a bug of editor.

What is "This", which "bug", which "Editor"?

"This" means bug 787434. Please read the document which a "bug" in bugzilla is. Handling multiple bugs (especially different issues) does not make sense for managing regressions, the commit history nor any discussion log in a "bug".

Note that Gecko which is the core technology and product/module of both Firefox and Thunderbird is not built by only a few people so that splitting to multiple bugs from one symptom is reasonable and necessary steps to go advance.

Comment 33

[Michel Merlin]

Hopeless
Versailles, Fri 21 Jan 2022 02:48:15 +0100

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c2200ae41e5
part 1: Make `HTMLEditor` stop removing empty elements of inserted content r=m_kato
https://hg.mozilla.org/integration/autoland/rev/90f26e9bed05
part 2: Make `HTMLWithContextInserter::FragmentFromPasteCreator` delete incomplete list elements in the inserting fragment r=mbrodesser
https://hg.mozilla.org/integration/autoland/rev/dd39d3d4da0b
part 3: Change the expectation of a test in copypaste.js r=mbrodesser

Comment 35

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

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

Comment 36

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4c2200ae41e5
https://hg.mozilla.org/mozilla-central/rev/90f26e9bed05
https://hg.mozilla.org/mozilla-central/rev/dd39d3d4da0b

Comment 37

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

Upstream PR merged by moz-wptsync-bot