Closed Bug 14827 Opened 21 years ago Closed 20 years ago

FRAME and IFRAME attribute scrolling=no is ignored

Categories

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

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: gemma, Assigned: pollmann)

References

()

Details

(Keywords: testcase, Whiteboard: [PDT+] (scrolling="no" shouldn't be ignored))

Attachments

(1 file)

for frames with scrolling="no" still have the scroll bar visible.
Assignee: karnaze → pollmann
Eric, this must be a regression.
Assignee: pollmann → evaughan
If I had to guess, I'd say this regression was caused by evaughan's recent
change to nsHTMLContentSink::StartLayout().  He added this line:

2323 evaughan 3.232     // initially show the scrollbars. We need to do this be
cause ano the scrollbars. -EDV
2325                    mWebShell->SetScrolling(-1, PR_FALSE);
ther

This looks like an attempted fix of bug 11683.  Eric, can you verify this?
One possible fix:

Index: mozilla/webshell/public/nsIWebShell.h
===================================================================
RCS file: /cvsroot/mozilla/webshell/public/nsIWebShell.h,v
retrieving revision 1.72
diff -r1.72 nsIWebShell.h
420c420
<   NS_IMETHOD SetScrolling(PRInt32 aScrolling, PRBool aSetCurrentAndInitial =
PR_TRUE)  = 0;
---
>   NS_IMETHOD SetScrolling(PRInt32 aScrolling, PRBool aSetCurrentAndInitial =
PR_FALSE)  = 0
Index: mozilla/webshell/src/nsWebShell.cpp
===================================================================
RCS file: /cvsroot/mozilla/webshell/src/nsWebShell.cpp,v
retrieving revision 1.261
diff -r1.261 nsWebShell.cpp
220c220
<   NS_IMETHOD SetScrolling(PRInt32 aScrolling, PRBool aSetCurrentAndInitial =
PR_TRUE);
---
>   NS_IMETHOD SetScrolling(PRInt32 aScrolling, PRBool aSetCurrentAndInitial =
PR_FALSE);
1793d1792
<   mScrolling[1] = aScrolling;
1794a1794
>	mScrolling[1] = aScrolling;
1795a1796,1798
>   } else {
>     // Ignore aScrolling and reset scrolling;
>     mScrolling[1] = mScrolling[0];
*** Bug 15208 has been marked as a duplicate of this bug. ***
*** Bug 12910 has been marked as a duplicate of this bug. ***
*** Bug 15652 has been marked as a duplicate of this bug. ***
*** Bug 15957 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
yes this was to fix bug 11683. When you go to a frameset it disables the
scrollbars bug never reenables them. Do you have a better fix?
The problem is that the meaning of the SetScrolling method in the webshell is
overloaded.  Framesets have been using this method to store their "scrolling"
attribute, and now XUL is using it to turn off the scrollbars whenever a XUL
document is being displayed.  The problem, of course, is that when XUL turns off
scrollbars, anyone who checks thinks that this frame/frameset had it's scrolling
attribute set to "no", even when a new page is loaded.

The fix you used to fix the XUL-stealing-scrollbars bug was to set the value to
"no value was specified" every time a new page is loaded.  The problem with that
is that now whenever anyone checks, they can't see the value for scrolling that
the frameset specified.

The diff I included above fixes this bug, bit it is a hack, as it just pushes
the scrolling state in a 2 level deep stack.  I don't have a better solution to
this yet, can you think of one?
*** Bug 16705 has been marked as a duplicate of this bug. ***
*** Bug 16384 has been marked as a duplicate of this bug. ***
*** Bug 17307 has been marked as a duplicate of this bug. ***
*** Bug 6724 has been marked as a duplicate of this bug. ***
*** Bug 18807 has been marked as a duplicate of this bug. ***
Summary: scrollbar still visible → FRAME and IFRAME attribute scrolling=no is ignored
Changing Summary to:
FRAME and IFRAME attribute scrolling=no is ignored
to make it easier to find...
*** Bug 19947 has been marked as a duplicate of this bug. ***
*** Bug 20123 has been marked as a duplicate of this bug. ***
Priority: P3 → P4
Target Milestone: M13
setting p4 for m13. EVaughan, would you please look at Pollman's proposed fix
and comment?
I was just about to report this bug.  Thats a lot of dupes... I did a search for
"frame scroll" and nothing came up.  It only came up when I did a search for
"frame".  Maybe thats why there are so many dupes.

No Dupe for you!
*** Bug 21960 has been marked as a duplicate of this bug. ***
*** Bug 21960 has been marked as a duplicate of this bug. ***
Whiteboard: [TESTCASE] scrolling="no" shouldn't be ignored
*** Bug 17652 has been marked as a duplicate of this bug. ***
*** Bug 22470 has been marked as a duplicate of this bug. ***
Blocks: 16346
Target Milestone: M13 → M14
*** Bug 22547 has been marked as a duplicate of this bug. ***
*** Bug 23027 has been marked as a duplicate of this bug. ***
*** Bug 23038 has been marked as a duplicate of this bug. ***
*** Bug 23250 has been marked as a duplicate of this bug. ***
*** Bug 23603 has been marked as a duplicate of this bug. ***
Summary: FRAME and IFRAME attribute scrolling=no is ignored → [DOGFOOD] FRAME and IFRAME attribute scrolling=no is ignored
Adding "DOGFOOD" tag to get a PDT review.  This bug has a high dup count
and makes many sites look wrong and somewhat unusable.
Can you give us a list of the sites that have this problem?  Any of the top 100?
 -pdt
*** Bug 23825 has been marked as a duplicate of this bug. ***
Whiteboard: [TESTCASE] scrolling="no" shouldn't be ignored → [PDT+][TESTCASE] scrolling="no" shouldn't be ignored
Putting on PDT+ radar.
*** Bug 23949 has been marked as a duplicate of this bug. ***
*** Bug 24348 has been marked as a duplicate of this bug. ***
Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!
Keywords: testcase
*** Bug 24805 has been marked as a duplicate of this bug. ***
*** Bug 25675 has been marked as a duplicate of this bug. ***
*** Bug 25598 has been marked as a duplicate of this bug. ***
*** Bug 25890 has been marked as a duplicate of this bug. ***
*** Bug 25954 has been marked as a duplicate of this bug. ***
*** Bug 25971 has been marked as a duplicate of this bug. ***
*** Bug 26071 has been marked as a duplicate of this bug. ***
*** Bug 26157 has been marked as a duplicate of this bug. ***
*** Bug 26535 has been marked as a duplicate of this bug. ***
Putting dogfood in the keyword field.
Keywords: dogfood
*** Bug 26676 has been marked as a duplicate of this bug. ***
Summary: [DOGFOOD] FRAME and IFRAME attribute scrolling=no is ignored → FRAME and IFRAME attribute scrolling=no is ignored
Whiteboard: [PDT+][TESTCASE] scrolling="no" shouldn't be ignored → [PDT+]scrolling="no" shouldn't be ignored
bumping priority to p2
Priority: P4 → P2
Blocks: 26981
Ok this turns out to be a bug in nsHTMLContentSink. If you remove the line: 3012 
which shouldn't be there anyway (This was an old hack) xul works without it now.

    // initially show the scrollbars. We need to do this because another
    // document like a XUL document, could have have hidden the scrollbars. -EDV
    //mWebShell->SetScrolling(-1, PR_FALSE);

You will find that the bug still happens if you go to page with frames and they 
and they are set to hidden they will still show up. This is why:

1) When the "Frame" tag that has "scrolling=no" is parsed nsFrameFrame builds a 
webshell and sets scrolling on it to 1 meaning "no scrollbars"
2) StartLayout is called on the nsHTMLContentSink.
3) At line 3018 it checks to see if mBody is set. Which it is because this is
   a frame not a frame set.
4) It then gets the nsIMarkupDocumentViewer from the webshell. And asks it
   if it is a Frame by calling IsFrameDoc. It then returns FALSE which is wrong!    
   It is a frame document.
5) It then proceeds to set scrolling to -1 meaing make scrollbars even though  
   nsFrameFrame has explicitly set it not to have scrollbars.

Looks like someone is not telling the nsIMarkupDocumentViewer that is a Frame 
document. So I figure that when we discover its a frame document we should be 
calling SetIsFrameDoc(PR_TRUE) nsIMarkupDocumentViewer. Unfortunately the 
frameset webshell's nsIMarkupDocumentViewer is nsnull.

Refering to layout to figure out who might be able to fix this.
Assignee: evaughan → troy
Status: ASSIGNED → NEW
Component: HTMLFrames → Layout
Eric, frameset related so assigning to you
Assignee: troy → pollmann
Blocks: 25824
I see the problem - this is new since the webshell redesign and was masked
by the SetScrolling(-1, PR_FALSE) that was just removed.

In nsHTMLFrameInnerFrame, where we want call SetIsFrame() we never actually
do because the content viewer has not been embeded in the webshell yet.
I'll take a look at this after I get finish the other PDT+ bugs I started
already.

As I just got this today, it will probably take me a day or three to get
to it, so I'm guestimating at 16-Feb-2000?
Status: NEW → ASSIGNED
Component: Layout → HTMLFrames
OS: Windows NT → All
Hardware: PC → All
Whiteboard: [PDT+]scrolling="no" shouldn't be ignored → [PDT+] 16-Feb-2000 scrolling="no" shouldn't be ignored
*** Bug 28075 has been marked as a duplicate of this bug. ***
Whiteboard: [PDT+] 16-Feb-2000 scrolling="no" shouldn't be ignored → [PDT+] Fix in hand, need r/a (scrolling="no" shouldn't be ignored)
Reviewed by Vidur and Harish.
Whiteboard: [PDT+] Fix in hand, need r/a (scrolling="no" shouldn't be ignored) → [PDT+] Fix in hand, need approval (scrolling="no" shouldn't be ignored)
Fixed.

My fix initially was to re-break bug 11683.  It turns out bug 11683 was really
simple (nsXULDocument was setting current and initial scroll state when it only
should have set current scroll state), so I fixed that too.

To verify, go to any of these pages.  Shouldn't show scrollbars where they say
"scrolling='no'"  Also you should see scrollbars after viewing a XUL file (see
bug 11683 for a test case.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+] Fix in hand, need approval (scrolling="no" shouldn't be ignored) → [PDT+] (scrolling="no" shouldn't be ignored)
Fixed in the Feb 21 build.
Status: RESOLVED → VERIFIED
*** Bug 29769 has been marked as a duplicate of this bug. ***
Product: Core → Core Graveyard
Component: Layout: HTML Frames → Layout: Images
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.