240129 - noframes should have style display:block when allowFrames is false

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[timeless]

when html.css changed the default, the c++ didn't change to reflect this.

the result is that frames get a style from css and from c++, and noframes 
always get display:none from css and no love from c++.

Comment 1

[timeless]

Attachment: mark noframes as block for !allowFrames

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145781 [details] [diff] [review]
mark noframes as block for !allowFrames

Mutating style data is evil.  This should be done like noscript via the prefs
stylesheet, probably.  What bug introduced the current problem?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Per our irc discussion:

1)  The fix to bug 134401 is wrong
2)  The html.css part of that fix should stay
3)  When frames are disabled, the user sheet should include a rule resetting the
    noframes display.
4)  The code in nsCSSFrameConstructor that mutates the style data should be
    removed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

timeless, could you actually fix this up?

Comment 5

[timeless]

i'm just going to post a checkpoint. currently accessing the prefstylesheet is
'hard'. dbaron mentioned perhaps exposing nsIDOMCSSStyleSheet.

note to self:
PresShell::CreatePreferenceStyleSheet
PresShell::SetPrefColorRules
PresShell::SetPrefNoScriptRule

Comment 6

[timeless]

Attachment: checkpoint

with this (improper) patch, my browser starts, the sidebar works, and i can
toggle between showing <frameset>s or <noframes>.

iframes unfortunately honor browser.frames.enabled, which seems wrong. it also
results in assertions (not properly handling
NS_ERROR_CONTENT_BLOCKED_SHOW_ALT).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note that bug 269566 may add more code that could be removed once this patch is
done...

Comment 8

[Bernd]

Attachment: guess

boris is that something that you had in mind with your comment?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175187 [details] [diff] [review]
guess

>Index: nsPresShell.cpp
>@@ -2174,16 +2176,19 @@ PresShell::SetPreferenceStyleRules(PRBoo
>+      if (NS_SUCCEEDED(result)) {
>+        result = SetPrefNoFramesRule();
>+      }

Putting this here has no effect, since prescontext doesn't observer this
preference...

Also note that docshells can have subframes disallowed directly, not via pref. 
So really, the docshell should notify its associated prescontext if it has
frames disabled.

There's another side issue here.  What happens with printing?  Do we even have
a docshell there?

>+    rv = sheet->InsertRule(NS_LITERAL_STRING("noframes{display:block!important}"),

Why important?	I don't see a reason for it to be...

>+    rv = sheet->InsertRule(NS_LITERAL_STRING("iframe{display:none!important}"),
>+                           sInsertPrefSheetRulesAt, &index);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = sheet->InsertRule(NS_LITERAL_STRING("frameset{display:none!important}"),
>+                           sInsertPrefSheetRulesAt, &index);

For good measure, set display to "none" on "frame" too (in case we ever fix it
to be constructed by frame constructor).

And just insert a single:

  frame, frameset, iframe { display: none !important; }

rule instead of three separate rules.

>Index: nsCSSFrameConstructor.cpp
>+      // the nsSubDocumentFrame needs to know about its content parent during ::Init.
>+      // there is no reasonable way to get the value there.
>+      // so we store it as a frame property.

Can you file a bug on this?  cc me and roc.  I think this should be easy to
fix.

>@@ -5729,27 +5684,18 @@ nsCSSFrameConstructor::ConstructXULFrame

The change to this seems not-quite-right for the case when the docshell
explicitly has frames disabled (ie, not via the pref).

I guess this is ok short-term, but file a followup on XUL or docshell to sort
out what the behavior should be?  cc me.

Comment 10

[Bernd]

from irc:
	bernd	bz:then I can continue to look for code that I can copy to get the
noframe thing right
	bz	bernd: heh
	bz	bernd: I was thinking.... I really think the state should be stored on the
document, not the docshell
	bz	bernd: and docshell should just set its state on documents that get stuck in it
	bz	bernd: this is going a little far afield from your patch....
	bz	bernd: but it would 100% work with printing (which has a document, but
probably no docshell)
	bernd	but the previous code did query the docshell inside the frame construction?
	bz	yes, yes
	bz	And I'm not even sure _that_ worked when printing....
	jst	bz: printing is a huge mess, as you know... so make sure we don't freeze in
any of it's nastiness into any APIs etc
	bz	jst: that's the plan
	jst	bz: good :)
	bz	jst: oh, good. I can ask you this.
	bz	jst: bernd is fixing our impl of the "allowFrames" boolean on docshell
	bz	jst: current proposal is as follows
	bz	jst: 1) The boolean only affects HTML frames (so not XUL, eg)
	-->|	v0id ([e-mail]) has joined
#developers
	bz	jst: 2) The boolean is gotten from the docshell by the prescontext
	bz	jst: I was thinking that it'd make sense to replace #2 with "docshell sets
the boolean on the document, so prescontexts have access to it"
	bz	jst: along with "document notifies prescontexts if the boolean changes"
	bz	jst: thoughts?
	bz	jst: put another way, given a DOM-created document, when we have the pref
set, do we want to honor the pref?
	bz	jst: and not load the subframes?
	bz	jst: note there is no docshell in sight there...
	jst	bz: hmm, good question. I think I'd say we do honor the pref, we could just
check it at document creation time or something... or do we just check the pref
at each load?
	-->|	jesus_X (jesus_x@204.95.67.22) has joined #developers
	bz	I'd say check the pref at document creation
	bz	And set up a listener...
	jst	sure
	bz	(and fix content policy to get the data off the document instead of the
docshell)
	bernd	bz: doesnt that pref style sheet persist between documents?
	jst	bz: or have a global static that holds the up-to-date pref value?
	bz	And allow docshell to disable if it's enabled but not the reverse
	bz	jst: oh, hmmm.. I like the global static
	bz	jst: then document could store the docshell state
	bz	jst: and return the && of the two
	bz	bernd: no
	jst	bz: yeah
	bz	bernd: it's per-presshell, no?
	jst	bz: fewer pref observers too, which is good
	bz	bernd: as in, we have a lot of them around
	bz	jst: agreed
	bz	bernd: if you want, do the patch as-is (with my comments on "frame"), and
file a folowup bug on me to implement what jst and I just talked about
	bernd	bz:I dont really get what is bad with the current approach
	bernd	or is it only that the patch is doing better but not right
	bz	bernd: exactly
	bz	bernd: it's better
	bz	bernd: it solves some of the existing issues (the ones the bug is filed on)
	bz	bernd: it misses some cases, but those should really go to a separate bug

Comment 11

[Bernd]

the crash from
http://nova.serverway.net/~mw22/test/mozilla/280217_crash/280217_noframes.html
which was found in  bug 280217 will be fixed by the patch

Comment 12

[Bernd]

Boris, just another question why cant we hold that property on the prescontext
like mUseDocumentFonts
(http://lxr.mozilla.org/seamonkey/search?string=mUseDocumentFonts), add an
preference observer and call SetPrefNoFramesRule when the preference changes?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Because frames can be disabled on a per-docshell basis.  See
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIDocShell.idl#231

Comment 14

[Bernd]

Attachment: revised patch

Comment 15

[Bernd]

Boris, I filed bug 284319 and bug 284320 as suggested.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175959 [details] [diff] [review]
revised patch

r+sr=bzbarsky.	Thank you for doing this!

Comment 17

[Bernd]

fix checked in