NOSCRIPT not treated as a block when JS is turned off

RESOLVED FIXED in mozilla1.5beta



17 years ago
14 years ago


(Reporter: Marc Attinasi, Assigned: John Keiser (jkeiser))



Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: edt_b3, composer++)


(1 attachment, 2 obsolete attachments)



17 years ago
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

17 years ago
Accepting but moving to future since this is very low priority. Voice your 
opinions if you feel differently...
Priority: -- → P3
Target Milestone: --- → Future

Comment 2

17 years ago

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

15 years ago
nominating topembed... this may be important for future clients
Needs a new owner
Keywords: topembed
-> Kin
Assignee: attinasi → kin


15 years ago
Keywords: topembed → topembed+
Whiteboard: edt_b3
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

15 years ago
need to get a new target milestone on this one.


15 years ago
Target Milestone: Future → mozilla1.5beta

Comment 7

14 years ago
I have a patch started for this involving CSSFrameConstructor.
Assignee: kin → jkeiser

Comment 8

14 years ago
Created attachment 125769 [details] [diff] [review]

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.


14 years ago
Blocks: 67899


14 years ago
Blocks: 176910
Comment on attachment 125769 [details] [diff] [review]

Something in/around ModifyStyle needs to check that the namespace is HTML.

Comment 10

14 years ago
John: We have to find out how residual style and autoclosure will affect
noscript content when script is enabled.


14 years ago
Blocks: 82472

Comment 11

14 years ago
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

14 years ago
Created attachment 125855 [details] [diff] [review]
Patch v2

This version of the patch:
- consolidates IsScriptEnabled() from two places that now need it into
- adds the rule "noscript{display:block}" to the pref stylesheet if script is
- doesn't strip the noscript tag anymore (the CNavDTD.cpp change)
Attachment #125769 - Attachment is obsolete: true

Comment 13

14 years ago
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 :)
Attachment #125855 - Flags: review?(bzbarsky)

Comment 14

14 years ago
Comment on attachment 125855 [details] [diff] [review]
Patch v2

r=harishd for CNavDTD.cpp change :)
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

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.
Attachment #125855 - Flags: review?(bzbarsky) → review+
Whiteboard: edt_b3 → edt_b3, composer++
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

14 years ago
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

14 years ago
Created attachment 126370 [details] [diff] [review]
Patch v2.0.1

This fixes all bz's nits, including SetPrefNoScriptRule.
Attachment #125855 - Attachment is obsolete: true

Comment 19

14 years ago
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.
Attachment #126370 - Flags: superreview?(dbaron)
Attachment #126370 - Flags: review+
Attachment #126370 - Flags: superreview?(dbaron) → superreview+

Comment 20

14 years ago
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.
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 21

14 years ago
I'm going to be incommunicado until evening tomorrow, I'll check up on things then.
You need to log in before you can comment on or make changes to this bug.