895239 - OutputNonTextContentAsPlaceholder fails to output non-textual container element as placeholder

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect, P1)

Description

[Yuan Xulei [:yxl] (Leave from May 18)]

This is a followup of Bug 890580.

By design, the plaintext serializer with OutputNonTextContentAsPlaceholder flag should convert iframe to placeholder character. But the current implementation converts iframe to empty string.

Comment 1

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: iframe_as_placeholder

Without this patch, `<iframe></iframe>` and `<iframe>XXX</iframe>` is serialized to empty string and "XXX" respectively. 

With this patch and OutputNonTextContentAsPlaceholder flag, <iframe> will be serialized to the placeholder character - U+FFFC (OBJECT REPLACEMENT CHARACTER).

Hi Olli, as Henry(:hsivonen) isn't available until 29 July, could help to review the patch?

Comment 2

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Serialize iframe as placeholder charater with test case

Fix a typo in the test_bug895239.html.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 780155 [details] [diff] [review]
Serialize iframe as placeholder charater with test case

># HG changeset patch
># Parent fb4bf993a58a153a1d63302ccd2aa3103975f850
># User Yuan Xulei <[e-mail]>
>Serialize iframe as placeholder if OutputNonTextContentAsPlaceholder is set. r=smaug
>
>diff --git a/content/base/src/nsDocumentEncoder.cpp b/content/base/src/nsDocumentEncoder.cpp
>--- a/content/base/src/nsDocumentEncoder.cpp
>+++ b/content/base/src/nsDocumentEncoder.cpp
>@@ -477,21 +477,26 @@ nsDocumentEncoder::SerializeToStringRecu
> 
>   if (!aDontSerializeRoot) {
>     rv = SerializeNodeStart(maybeFixedNode, 0, -1, aStr, aNode);
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>   nsINode* node = serializeClonedChildren ? maybeFixedNode : aNode;
> 
>-  for (nsINode* child = nsNodeUtils::GetFirstChildOfTemplateOrNode(node);
>-       child;
>-       child = child->GetNextSibling()) {
>-    rv = SerializeToStringRecursive(child, aStr, false);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+  // If OutputNonTextContentAsPlaceholder is set, don't serialize child nodes
>+  // of <iframe>.
>+  if (!((mFlags & OutputNonTextContentAsPlaceholder) &&
>+      aNode->Tag() == nsGkAtoms::iframe)) {
This code really shouldn't have to deal with iframes.
(In general all the special handling for some particular element in nsDocumentEncoder class is a bug, IMO, and looks like
it is currently limited to selection handling only, and <br>)
You need to somehow deal this stuff in serializer.
And is iframe really the only case broken?

Comment 4

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Olli Pettay [:smaug] from comment #3)
> This code really shouldn't have to deal with iframes.
> (In general all the special handling for some particular element in
> nsDocumentEncoder class is a bug, IMO, and looks like
> it is currently limited to selection handling only, and <br>)
> You need to somehow deal this stuff in serializer.

I tried to keep the handling logic inside serializer, but it seems impossible to put all in serializer. 

Traditionally, to be compatible with browsers which don't support <iframe>, bea short text is added between <iframe> and </iframe>. This short text is interpreted as child nodes of <iframe>.
These child nodes should be skipped during the serializing process. 

Since nsDocumentEncoder is responsible for node traversal and determines which node to serialize and which node not, we are able to skip the child nodes of <iframe> in this class.

While nsPlainTextSerializer is only responsible for converting node to text. It doesn't know the node context. When receiving child nodes of <iframe>, nsPlainTextSerializer cannot tell whether they belong to <iframe> and then cannot skip them. 

> And is iframe really the only case broken?

No, unfortunately I found another broken case of <canvas>. I'll figure out a list of all broken tags.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Well, it really needs to go to serializer. You can skip stuff there. It already keeps track of
various states. Add something to AppendElementStart and AppendElementEnd or so.

Comment 6

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Serialize non-textual container element as placeholder charater.

This patch handles all text serialization work in nsPlainTextSerializer. A member variable of mIgnoredChildNodeLevel is added to ignore child nodes of <iframe>.

By checking HTML element list(https://developer.mozilla.org/en-US/docs/Web/HTML/Element), I got other similar broken cases, including <audio>, <canvas>, <meter>, <progress>, <object>, <svg> and <video>. 

This patch also fixed these cases.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 783250 [details] [diff] [review]
Serialize non-textual container element as placeholder charater.

Based on bugzilla, Henri should be back now.

Henri, please re-assign the review to me if you don't want or don't have time to
review this.

Yuan, please re-assign the review to me if you don't hear back from hsivonen in the next few days.


(Looks like the patch is rather huge mainly because of whitespace fixes.)

Comment 8

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: no_whitespace_fixes

Olli, thank you.

Should I remove the whitespace fixes?

To help with review, I create this patch without whitespace fixeds.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I guess you could just create a -w patch too.
hg diff -w or some such. That shouldn't contain the whitespace changes.

Comment 10

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 783250 [details] [diff] [review]
Serialize non-textual container element as placeholder charater.

Review of attachment 783250 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Olli, Henri seems too busy to review the patch and could you give a help?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Ok, reviewing.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 783250 [details] [diff] [review]
Serialize non-textual container element as placeholder charater.

Thanks for the -w patch!

Perhaps rename IsChildNodeIgnorable to ShouldReplaceContainerWithPlaceholder()

>-NS_IMETHODIMP 
>+bool
>+nsPlainTextSerializer::IsChildNodeIgnorable(nsIAtom* aTag)
>+{
>+  // If nsIDocumentEncoder::OutputNonTextContentAsPlaceholder is set,
>+  // non-textual container element should be serialized as placeholder
>+  // character and its child nodes should be ignored. See bug 895239.
>+  if (!(mFlags & nsIDocumentEncoder::OutputNonTextContentAsPlaceholder)) {
>+    return false;
>+  }
>+
>+  return
>+    (aTag == nsGkAtoms::audio) ||
>+    (aTag == nsGkAtoms::canvas) ||
>+    (aTag == nsGkAtoms::iframe) ||
>+    (aTag == nsGkAtoms::meter) ||
>+    (aTag == nsGkAtoms::progress) ||
>+    (aTag == nsGkAtoms::object) ||
>+    (aTag == nsGkAtoms::svg) ||
>+    (aTag == nsGkAtoms::video);
>+}
Hmm, it is not super clear how we should handle this case when some fallback is applied.
Like <object> may have some sane fallback content.
But I guess this is good enough for now.

>@@ -244,16 +244,17 @@ MOCHITEST_FILES_A = \
> 		test_bug282547.html \
> 		bug282547.sjs \
> 		test_domparser_null_char.html \
> 		test_bug811701.html \
> 		test_bug811701.xhtml \
> 		test_bug820909.html \
> 		test_bug704063.html \
> 		test_bug894874.html \
>+                test_bug895239.html \
indentation (looks like tabs are used for other cases here)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

I asked around, and I was told the \u syntax is not supported everywhere.
So, \x syntax need to be used. Fix the case in this and the case added in bug 890580

Comment 14

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Serialize non-textual container element as placeholder charater.

Olli, thanks for you help. The path addresses all your comments.

1. Renames IsChildNodeIgnorable to ShouldReplaceContainerWithPlaceholder.

2. Fixes indentation in content/base/test/Makefile.in.

3. Uses `\x` instead of `\u` to escape special unicode character in C++ code. Fixs the cases both in this and bug 890580.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 785952 [details] [diff] [review]
Serialize non-textual container element as placeholder charater.

Please push to try before to m-i.

Comment 16

[Yuan Xulei [:yxl] (Leave from May 18)]

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=6dbafd0b8ee3

The mochitest on window and linux has been passed and it's ready to check in.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d6e9e87f6f9

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d6e9e87f6f9

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for uplift.

Comment 20

[Yuan Xulei [:yxl] (Leave from May 18)]

Hi Ryan, the patch of bug 890580 is required to uplift before this. After bug 890580 lands on b2g18, I guess this patch could be appied to b2g18 branch without confict.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0a57b78ed706

Comment 22

[Yuan Xulei [:yxl] (Leave from May 18)]

Ryan, thanks for your help!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/0a57b78ed706