Closed Bug 390735 Opened 13 years ago Closed 12 years ago

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

Categories

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

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: moz, Assigned: sciguyryan)

References

Details

(Keywords: regression, testcase)

Attachments

(4 files, 4 obsolete files)

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.
Attached file testcase1
This is a regression against 1.8.x (FF2)
Keywords: regression, testcase
Version: unspecified → Trunk
Attached file testcase2 (HTML5)
No bug with HTML5-style-markup:
<meta charset="">
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: innerHTML skips <meta http-equiv="content-type" charset=...> → innerHTML skips <meta http-equiv="content-type" content="text/html; charset=...">
Blocks: 331991
Assignee: nobody → sciguyryan
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.
(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?
Attached patch Like this? (obsolete) — Splinter Review
Is this the type of thing you meant?
Attachment #275241 - Flags: review?(bzbarsky)
Attachment #275241 - Attachment is obsolete: true
Attachment #275241 - Flags: review?(bzbarsky)
Attached patch A little better. (obsolete) — Splinter Review
This one should be a little better (still not sure if this is the solution you were looking for though).
Attachment #275248 - Flags: review?(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?
Attached patch Patch v1.1 (obsolete) — Splinter Review
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)
Attachment #275248 - Attachment is obsolete: true
Attachment #275322 - Flags: superreview?(bzbarsky)
Attachment #275322 - Flags: review?(bzbarsky)
Attachment #275248 - Flags: review?(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...
Attachment #275322 - Flags: superreview?(bzbarsky)
Attachment #275322 - Flags: superreview+
Attachment #275322 - Flags: review?(bzbarsky)
Attachment #275322 - Flags: review+
should come with a mochitest, no?
Flags: in-testsuite?
Attached patch Patch v1.2 (obsolete) — Splinter Review
Patch v1.2

Now with a mochitest and corrected BZ's nit.
Attachment #275322 - Attachment is obsolete: true
Attachment #275455 - Flags: superreview?(bzbarsky)
Attachment #275455 - Flags: review?(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.
Attached patch Patch v1.3Splinter Review
Patch v1.3
Attachment #275455 - Attachment is obsolete: true
Attachment #275470 - Flags: superreview?(bzbarsky)
Attachment #275470 - Flags: review?(bzbarsky)
Attachment #275455 - Flags: superreview?(bzbarsky)
Attachment #275455 - Flags: review?(bzbarsky)
Comment on attachment 275470 [details] [diff] [review]
Patch v1.3

Excellent.  And with tests, too!
Attachment #275470 - Flags: superreview?(bzbarsky)
Attachment #275470 - Flags: superreview+
Attachment #275470 - Flags: review?(bzbarsky)
Attachment #275470 - Flags: review+
Attachment #275470 - Flags: approval1.9?
   // We need too skip any meta tags that set the content type
   // becase we set our own later.

Typo "becase"
(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.
Status: NEW → ASSIGNED
Attachment #275470 - Flags: approval1.9? → approval1.9+
Checked in (sadly without the typo fix; didn't see that part).
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.