77296 - NOSCRIPT not treated as a block when JS is turned off

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Marc Attinasi]

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]

Accepting but moving to future since this is very low priority. Voice your 
opinions if you feel differently...

Comment 2

[Chris Petersen]

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)]

nominating topembed... this may be important for future clients
Needs a new owner

Comment 4

[Kevin McCluskey (gone)]

-> Kin

Comment 5

[Boris Zbarsky [:bzbarsky]]

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]

need to get a new target milestone on this one.

Comment 7

[John Keiser (jkeiser)]

I have a patch started for this involving CSSFrameConstructor.

Comment 8

[John Keiser (jkeiser)]

Attachment: 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-4, no longer working on Mozilla)]

Comment on attachment 125769 [details] [diff] [review]
Patch

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

Comment 10

[harishd]

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

Comment 11

[John Keiser (jkeiser)]

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)]

Attachment: 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)]

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]

Comment on attachment 125855 [details] [diff] [review]
Patch v2

r=harishd for CNavDTD.cpp change :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

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) (not active in Mozilla any more)]

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)]

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)]

Attachment: Patch v2.0.1

This fixes all bz's nits, including SetPrefNoScriptRule.

Comment 19

[John Keiser (jkeiser)]

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)]

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)]

I'm going to be incommunicado until evening tomorrow, I'll check up on things then.