Last Comment Bug 77296 - NOSCRIPT not treated as a block when JS is turned off
: NOSCRIPT not treated as a block when JS is turned off
Status: RESOLVED FIXED
edt_b3, composer++
: topembed+
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla1.5beta
Assigned To: John Keiser (jkeiser)
: Chris Petersen
Mentors:
Depends on:
Blocks: 67899 82472 176910
  Show dependency treegraph
 
Reported: 2001-04-23 22:07 PDT by Marc Attinasi
Modified: 2003-06-25 23:27 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (15.18 KB, patch)
2003-06-16 13:32 PDT, John Keiser (jkeiser)
no flags Details | Diff | Review
Patch v2 (7.05 KB, patch)
2003-06-17 15:35 PDT, John Keiser (jkeiser)
bzbarsky: review+
Details | Diff | Review
Patch v2.0.1 (7.05 KB, patch)
2003-06-24 09:14 PDT, John Keiser (jkeiser)
john: review+
dbaron: superreview+
Details | Diff | Review

Description Marc Attinasi 2001-04-23 22:07:15 PDT
This was spun-off of bug 67899. When JS is turned off, NOSCRIPT elements should 
be rendered as a block element, not an inline. It might be possible to just put 
a rule in the html.css 'NOSCRIPT { display:block; }' because it appears that the 
content sink will not even generate a NOSCRIPT node when HS is on, so the 
display:none is unnecessary. Needs further investigation.
Comment 1 Marc Attinasi 2001-04-23 22:09:13 PDT
Accepting but moving to future since this is very low priority. Voice your 
opinions if you feel differently...
Comment 2 Chris Petersen 2001-04-25 21:11:31 PDT
Mark,

I was looking at 67899 and realized the working NOSCRIPT sample is rendered as
html not xml.I originally thought the issue was because strict layout mode, but
I believe this is nolonger the case. If you change the html sample file's
extension to .xml, the NOSCRIPT no longer is displayed.I understand the strict
layout is used in xml docs but the sample xml file is compliant to the strict
DTD( The NOSCRIPT content model requires a block element). In this case, the
noscript element in the test case contains a P child. If you want me to reopen
67889, let me know.
Comment 3 saari (gone) 2003-03-14 10:09:06 PST
nominating topembed... this may be important for future clients
Needs a new owner
Comment 4 Kevin McCluskey (gone) 2003-03-14 14:44:45 PST
-> Kin
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2003-04-21 23:18:06 PDT
The issue is that in HTML the parser drops the <noscript> and just shows its
contents.  In XML, the noscript is in the content model and gets display:none
from html.css.

I suppose we could stick something in the "preference stylesheet" to set its
display to block when JS is off....
Comment 6 chris hofmann 2003-05-05 10:51:46 PDT
need to get a new target milestone on this one.
Comment 7 John Keiser (jkeiser) 2003-06-13 12:36:49 PDT
I have a patch started for this involving CSSFrameConstructor.
Comment 8 John Keiser (jkeiser) 2003-06-16 13:32:17 PDT
Created attachment 125769 [details] [diff] [review]
Patch

This patch fixes the problem by moving the code out of the parser into CSS
frame constructor, and just changing the display type depending on whether
scripting is enabled in the document.  I also created
nsIDocument::IsScriptEnabled() and consolidated code from two different places
into it.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-06-16 15:48:01 PDT
Comment on attachment 125769 [details] [diff] [review]
Patch

Something in/around ModifyStyle needs to check that the namespace is HTML.
Comment 10 harishd 2003-06-17 10:48:32 PDT
John: We have to find out how residual style and autoclosure will affect
noscript content when script is enabled.
Comment 11 John Keiser (jkeiser) 2003-06-17 14:40:24 PDT
I have a new patch cooking that preserves some of the old
behavior--particularly, when scripting is enabled, we *don't* want to parse the
contents of noscript.
Comment 12 John Keiser (jkeiser) 2003-06-17 15:35:22 PDT
Created attachment 125855 [details] [diff] [review]
Patch v2

This version of the patch:
- consolidates IsScriptEnabled() from two places that now need it into
nsIDocument::IsScriptEnabled()
- adds the rule "noscript{display:block}" to the pref stylesheet if script is
disabled
- doesn't strip the noscript tag anymore (the CNavDTD.cpp change)
Comment 13 John Keiser (jkeiser) 2003-06-17 15:38:36 PDT
Comment on attachment 125855 [details] [diff] [review]
Patch v2

bz, want to review?  can't think of anyone else who might have touched both
parser and layout :)
Comment 14 harishd 2003-06-17 18:41:52 PDT
Comment on attachment 125855 [details] [diff] [review]
Patch v2

r=harishd for CNavDTD.cpp change :)
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2003-06-20 17:38:37 PDT
Comment on attachment 125855 [details] [diff] [review]
Patch v2

What about checking the return value of CanExecuteScripts()?  If it failed,
|enabled| may be garbage.  I know you just moved the code, but still...

Why SetPrefNoXXXRules?	All it does is set NoScriptRules; please name it
accordingly.

Just declare "rv" inside the !mDocument->IsScriptEnabled() if, isntead of
declaring it twice.

Do we want to deal with the user turning off script after a page has loaded? 
Should that make the NOSCRIPT blocks show?  I guess they won't contain the
content they really should, huh?  So let's not.

r=me with the nits addressed.
Comment 16 Daniel Glazman (:glazou) 2003-06-23 04:21:32 PDT
I just tested this patch in my own tree. It works fine! This was very
important for Composer as it will allow us to implement

  (a) templates
  (b) downloadable smart widgets

Thanks a lot, jkeiser!!!!
Comment 17 John Keiser (jkeiser) 2003-06-23 12:43:04 PDT
bz, I'd be happy to change it to SetPrefNoscriptRules if you like, but I intend
to add noframes into this mix, and I'd like to put it in that method.  I simply
didn't do that yet because I haven't figured out how to properly test it.

Agreed on the dynamic switching of NOSCRIPT.  Unfortunately you'd have to
re-parse or re-serialize NOSCRIPT contents every time the pref value was
switched.  Not to mention if you disable scripting after it was already on, the
page would be in an inconsistent state: SCRIPT tags would already have run, yet
NOSCRIPT is showing.  I believe the script disabled pref should stop any future
scripts from running but not have any retroactive effects.

I'll fix the other stuff.
Comment 18 John Keiser (jkeiser) 2003-06-24 09:14:55 PDT
Created attachment 126370 [details] [diff] [review]
Patch v2.0.1

This fixes all bz's nits, including SetPrefNoScriptRule.
Comment 19 John Keiser (jkeiser) 2003-06-24 09:20:11 PDT
Comment on attachment 126370 [details] [diff] [review]
Patch v2.0.1

Carrying r=bz.	dbaron, you will be pleased to note that this patch does not
have the word "CSSFrameConstructor" in it anywhere.
Comment 20 John Keiser (jkeiser) 2003-06-25 23:20:18 PDT
Fix checked in.  Should we go for approval?  Risk is minimal for people with
script enabled, and medium for people with script disabled.  Depends on whether
Daniel's extensions would be useful in 1.4 or not.
Comment 21 John Keiser (jkeiser) 2003-06-25 23:27:17 PDT
I'm going to be incommunicado until evening tomorrow, I'll check up on things then.

Note You need to log in before you can comment on or make changes to this bug.