390735 - innerHTML skips <meta http-equiv="content-type" content="text/html; charset=...">

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[j.j.]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007080204 Minefield/3.0a7pre
Build Identifier: 

innerHTML omits any tag of the type
<meta http-equiv="content-type" charset=...> ,
but not <meta charset=...> .

Reproducible: Always

Steps to Reproduce:
See testcases.

Comment 1

[j.j.]

Attachment: testcase1

This is a regression against 1.8.x (FF2)

Comment 2

[j.j.]

Attachment: testcase2 (unknown charset)

Comment 3

[j.j.]

Attachment: testcase2 (HTML5)

No bug with HTML5-style-markup:
<meta charset="">

Comment 4

[Boris Zbarsky [:bzbarsky]]

We should probably just not do this fixup when not serializing an entire document.  If we change nsIContentSerializer to take this information in its Init() method, this should be a simple fix.

Comment 5

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #4)
> We should probably just not do this fixup when not serializing an entire
> document.  If we change nsIContentSerializer to take this information in its
> Init() method, this should be a simple fix.
> 

Wouldn't we also need to add a similar parameter too nsIDocumentEncoder? Or could we just always pass a false too the nsIContentSerializer's parameter?

Comment 6

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Like this?

Is this the type of thing you meant?

Comment 7

[Ryan Jones-Ward [:sciguyryan]]

Attachment: A little better.

This one should be a little better (still not sure if this is the solution you were looking for though).

Comment 8

[Boris Zbarsky [:bzbarsky]]

That looks about right, with just a few nits:

1) Don't do the " = PR_FALSE" thing.  Just add an arg.
2) Put the PRPackedBool member next to the other PRPackedBools in that class.

Other than that, looks great!  I assume you did test to make sure it fixes this bug, right?

Comment 9

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.1

Patch v1.1

(In reply to comment #8)
> 1) Don't do the " = PR_FALSE" thing.  Just add an arg.
Done.

> 2) Put the PRPackedBool member next to the other PRPackedBools in that class.
> 
Done.

> Other than that, looks great!  I assume you did test to make sure it fixes this
> bug, right?

I tested it - both the saving fixup and the innerHTML seemed to be working as expected. (The HTML was fixed up with the saving and wasn't when called from .innerHTML)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275322 [details] [diff] [review]
Patch v1.1

Looks good if you actually init the new member in the constructor.  Sorry about not catching that the first time...

Comment 11

[Robert Sayre]

should come with a mochitest, no?

Comment 12

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.2

Patch v1.2

Now with a mochitest and corrected BZ's nit.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275455 [details] [diff] [review]
Patch v1.2

>Index: content/base/src/nsHTMLContentSerializer.cpp
>   mMayIgnoreLineBreakSequence(PR_FALSE),
>   mInCDATA(PR_FALSE),
>-  mNeedLineBreaker(PR_TRUE)
>+  mNeedLineBreaker(PR_TRUE),
>+  mIsWholeDocument(PR_FALSE)

These should be in the same order as the members in the header.  Fix that, please.

Comment 14

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.3

Patch v1.3

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275470 [details] [diff] [review]
Patch v1.3

Excellent.  And with tests, too!

Comment 16

[j.j.]

   // We need too skip any meta tags that set the content type
   // becase we set our own later.

Typo "becase"

Comment 17

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #16)
>    // We need too skip any meta tags that set the content type
>    // becase we set our own later.
> 
> Typo "becase"
> 

Thanks for pointing that out. If approval is granted I'll attach a fixed version before requesting checkin.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Checked in (sadly without the typo fix; didn't see that part).