56501 - Dealing with noscript bugs - the new way

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P3)

Description

[harishd]

I'm attaching the mail that I sent out:

From the time NOSCRIPT got enabled we've been seeing a significant amount of
regressions.The primary reason for this is the way NOSCRIPT contents are
interpreted. That is, noscript seems to be considered equivalent to #ifdef (!!)
whereas it is not.  Because of this notion we're forced to deal with lots (
countless ) of wackiness. Theoretically speaking all these wackiness could be
corrected at some point of the project...but practically it's just not
possible to predict what kind of ugliness we might get into and will become
virtually impossible to fix 'em all.. In order to
avoid such a predicament here are a few suggestions:

1) Throw away NOSCRIPT content when JS is enabled.
     Upside - Noscript content cannot interfere with the rest of the content and
therefore will guarantee a good layout.
    Downside - Editor will suffer data loss!!!!

2) Collect NOSCRIPT content as *text* when JS is enabled.
    Upside - Noscript content cannot interfere with the rest of the content and
therefore will guarantee a good layout.
    Downside - Editor/Serializer should deal with this content as a special
case. No data loss.

3) If either of the above could not be done within the given time frame, that's
by 10/13/00, then we have to live with existing patch fixes ( as a backup plan ).
   Upside - A few of the know problems will be fixed.
   Downside - There could be hidden regressions.

Comment 1

[harishd]

Will attach a patch for plan 2..

Comment 2

[harishd]

Attachment: Proposed patch version 1.1

Comment 3

[Johnny Stenback (:jst)]

Attachment: First stab at dealing with noscript-content-as-cdata in the serializer

Comment 4

[harishd]

Attachment: Final patch [ a=vidur includes jst's Serializer fix r=me ]

Comment 5

[harishd]

Btw a=vidur and r=jst [ for parser/sink changes ], r=me [ for serializer 
changes ]

Comment 6

[Scott Collins]

Patch of 10/13/00 14:36 looks good; though there is extra logic around ending a
CDATA container that probably doesn't need to be there ... since you can't be
nesting any actual elements, there's no need to test on exit to reset
|mInCDATA|, it can be reset unconditionally.  Other than that, sr=scc.

Comment 7

[Scott Collins]

I should note that jst made me aware during the review of the outstanding bug
(for which a bug should be filed) with entity replacement in the HTML fragment
sink that is exposed by this work.

Comment 8

[harishd]

Marking rtm+ for PDT approval.

Comment 9

[Jim Roskind]

I'm convinced we need to give good consideration to this... so it should be
first landed on the trunk.
This is also big enough (and time late enough) that if you know of other folks
that could help with additional reviews, that would be helpful in holding down
the risk.
Please land on the trunk, and then report back with any regression info.  I
expect that basic browsing will be tested aggressively on the trunk, so I'm
hoping for could exposure of problems.
Please also tell me how the top 100 sites do with this patch (I believe you have
some way to test such parser changes in an automated fashion??).
Marking need info, till we have 24 hours of trunk-cook-time.

Comment 10

[harishd]

Attachment: Yet another patch [ probably a better one!!! ]

Comment 11

[harishd]

Attachment: Patch [ With changes suggested by [e-mail] ]

Comment 12

[Johnny Stenback (:jst)]

r=jst for the 10/14/00 15:17 patch.

Comment 13

[Scott Collins]

went over these changes carefully both with Harish and with Johnny; had Harish
run them for me in both a js-enabled and a js-disabled state, and examined both
the output and the actual tree.  He then made the suggested improvements and
also built and tested top 100 on an additional platform.  sr=scc.

Comment 14

[harishd]

Removing rtm need info and marking rtm+ ( since the changes are in the trunk - 
as requested by jar ).

Comment 15

[Peter Trudelle]

PDT says rtm++

Comment 16

[harishd]

Fix is in ( both branch & trunk ). Marking FIXED.

Comment 17

[Jan Carpenter]

verified

Comment 18

[root]

*** Bug 61181 has been marked as a duplicate of this bug. ***