Closed Bug 315999 Opened 19 years ago Closed 19 years ago

innerHTML breaking badly when the source text contains a STYLE tag (FF 1.5rc2)

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.8final

People

(Reporter: kevykev, Assigned: bzbarsky)

References

Details

(Keywords: regression, testcase, verified1.8)

Attachments

(1 file)

Several examples of innerHTML breaking badly when the source text contains a STYLE tag
(deleted), text/html
Details 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051107 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051107 Firefox/1.5

I work on the new Yahoo Mail Beta (Oddpost descendant), and we are seeing
frequent problems with display of email bodies within our application.  They
are updated into the preview pane, or compose areas, using innerHTML.  Bodies
that contain STYLE tags are showing a lot of problems.

We started seeing these with RC1 (we had no reports of this in beta 2 that I
know of, but probably many fewer users using FF beta 2).  Reports from one user are that RC2 works much better for her mail than RC1.  I still have several message bodies that fail to display in RC2, however.  We have reduced an example to a simple self-contained set of test cases.  One of them is really interesting in that it severely damages the DOM.  Attachment coming shortly.


Reproducible: Always

Steps to Reproduce:
1. in JavaScript, try:
divElement.innerHTML = "<div>text1<style type='text/css'></style><div> text2</div></div>";

That is all.

Actual Results:  
In the example above, the second word 'test2' is not rendered visibly in the DOM.

Other examples do even more damage to the document, as an attachment will show.


Expected Results:  
Straightforward rendering of the example HTML snippet.


Pardon my hubris as a new user suggesting this is a major bug.  I think it is warranted.
This document demonstrates, using very short examples, some severe problems
rendering into the innerHTML of an element, when the source text contains a
STYLE tag.

The last example is particularly interesting, as it badly damages the document. Elements all over the page will disappear when you press it.

We are looking into a workaround, e.g. whether we can achieve what we want with
document.write instead.
FYI, the test document appears to work really well with 1.5b1.

Confirmed with trunk nightly Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051110 Firefox/1.6a1

In the trunk I don't see the DOM damage with the fifth test, and the DOM looks correct in each case.  The computed height of the nested <div> is 0px though.  If you use the DOM Inspector you can see where the zero-height <div> is hiding.
->Layout

Side note 1: it's not valid HTML to have <style> outside of the <head> section.  Not that that's the problem here.
Side note 2: how do we get in on the beta? ;-)
Status: UNCONFIRMED → NEW
Component: General → Layout
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
QA Contact: general → layout
Version: unspecified → Trunk
OS/2 & Linux also.
OS: Windows XP → All
doesn't seem likely that this is a layout bug, to parser
Assignee: nobody → mrbkap
Component: Layout → HTML: Parser
QA Contact: layout → parser
With current trunk build, I can see the problem with button 2 and button 3.
The problem(s) with button 5 seems to be fixed, I can see the bug in 2005-10-26 trunk build, but not anymore with current trunk build.
Flags: blocking1.8.1?
(In reply to comment #5)
> doesn't seem likely that this is a layout bug, to parser

Could be something wrong with style reresolution, too.
(Though more likely it has something to do with batching and flushing of notifications in the content sink interacting with batching of style reresolution or something like that.)
Has someone tested that this is or is not indeed present in beta2?
Keywords: testcase
[11 09:58:24] <mkaply> the testcases work in b2
[11 09:59:15] <mkaply> and fail in rc1

My first guess at a cause would be bug 312695, which landed on the trunk between 2005-10-17 and 2005-10-18 and on the 1.8 branch on either side of the 2005-10-18 builds depending on the timestamp of the branch builds.
On the trunk, it worked on Linux firefox nightly 2005-10-17-04, was broken even worse on 2005-10-18-05, was in the state described for the branch (test 5 damaging other things) on 2005-10-19-05.
The extra problems with button 5 went away on the trunk between
2005-11-06-04-trunk and 2005-11-07-04-trunk (Linux firefox).
So, I think these windows correspond nicely to:
 * bug 312695 caused the main regression
 * bug 312847 fixed the extra problems in the 10-18 trunk build
 * bug 314776 fixed the problems with button 5 doing extra damage
Blocks: 312695
Assignee: mrbkap → bzbarsky
Component: HTML: Parser → Layout
QA Contact: parser → layout
Backing 312695/312847 out of a 1.5 branch build does fix these problems.
Component: Layout → HTML: Parser
Component: HTML: Parser → Layout
This is basically bug 315189 -- this is exactly the situation described in bug 315189 comment 8.  Drivers seemed pretty loath to take it, though.

I've done quite a bit of testing on that patch at this point, and I think it's safe to take for branch if we have to take something.  Probably just as safe as backing out bug 312695 and bug 312847...

There will still be issues if the <style> is not a "root" in the innerHTML but a child of one of the other nodes -- in that case we'll probably end up doubling up the content in that subtree (that's bug 314776).  But at least we won't be notifying on content that's not actually in the tree yet, which is what we're doing right now....
Depends on: 315189
Hey all, thanks.  We still don't have a report on a workaround.  Not sure we will have anything to say about it today.  Unfort. I can't test directly with the FF 11/11 daily build due to other errors during application start-up-- though I should be able to jury-rig some test documents using the HTML of known bad emails.

Re:

> Side note 1: it's not valid HTML to have <style> outside of the
> <head> section.

Yep; we have to deal with a lot of **** HTML in email bodies.  This is an issue for displaying stuff we don't control, we wouldn't ask for it to fix our own code.  In reviewing my own drafts folder, I found three examples that had been created by copying and pasting junk from web pages.


> Side note 2: how do we get in on the beta? ;-)

You'd be welcome to!  Commenters here are welcome to send an email requesting access to Pete Hicks at jph@yahoo-inc.com.  Please include a Yahoo ID.  Either make a new one, or send an existing one-- if you haven't logged in for a while, do so first and make sure the account is active, including the email (that should only take a minute, just hit mail.yahoo.com and sign in).

Thanks again for the prompt analysis.
Hi Kevin,

Can you please forward some of the bad messages to me (y_davel@yahoo.com) - I'm already in the beta, and I have some local builds I can use to test proposed fixes.

Thanks

_DaveL (former yahoo!)
Fixed on trunk (by checkin for bug 315189).
Status: NEW → RESOLVED
Closed: 19 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8final
Dave -- thanks a lot, will do.  I'm sending three shortly.  I sent out an internal request to gather a few more, not sure what that will turn up.  I was hoping to find some godawful layout-heavy newsletter-ish one to send, but I can't find one at the moment.  I have another account I can check, but it might take a while.

Fixed on branch -- mscott landed the fix for bug 315189 there.
Keywords: fixed1.8
Testcase looks good on the Mac using 2005-11-11-17-mozilla1.8/. Changing keyword to verified1.8.
Keywords: fixed1.8 → verified1.8
I'm going to clear the 1.8.1 nomination flag since we respun 1.8 to pick up this fix on Friday.
Flags: blocking1.8.1?
The nightly branch build from Friday works for every test case I have at hand.  Thanks to everyone involved!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: