NOSCRIPT not treated as a block when JS is turned off

RESOLVED FIXED in mozilla1.5beta

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
16 years ago
14 years ago

People

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

Tracking

({topembed+})

Trunk
mozilla1.5beta
topembed+
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: edt_b3, composer++)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

16 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.
(Reporter)

Comment 1

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

Comment 2

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

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

Updated

14 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

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

Updated

14 years ago
Target Milestone: Future → mozilla1.5beta
(Assignee)

Comment 7

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

Comment 8

14 years ago
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.
(Assignee)

Updated

14 years ago
Blocks: 67899
(Assignee)

Updated

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

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.
(Assignee)

Updated

14 years ago
Blocks: 82472
(Assignee)

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.
(Assignee)

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
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)
Attachment #125769 - Attachment is obsolete: true
(Assignee)

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
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.
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!!!!
(Assignee)

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.
(Assignee)

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
(Assignee)

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+
(Assignee)

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.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

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.