Bug 29240 - iframes keep getting scrollbars with scrolling="no"
Summary: iframes keep getting scrollbars with scrolling="no"
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: PC Windows Vista
: P2 Normal
Assignee: Nobody
URL: http://www.w3schools.com/TAGS/tryit.a...
Keywords: HasReduction, InRadar
: 30813 (view as bug list)
Depends on:
Reported: 2009-09-14 02:14 PDT by Korneel Wever
Modified: 2011-05-20 07:03 PDT (History)
14 users (show)

See Also:

Patch (42.07 KB, patch)
2010-09-23 22:09 PDT, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (14.25 KB, patch)
2010-10-05 18:07 PDT, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (11.47 KB, patch)
2010-10-06 19:06 PDT, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (11.23 KB, patch)
2010-10-07 15:43 PDT, Mike Lawther
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Korneel Wever 2009-09-14 02:14:08 PDT
Iframes keep displaying scrollbars with the attribute scrolling="no" and when their content is larger than the given width and height attributes.

Using Webkit 530.5 (Chrome

It also appears in Webkit 531.9.1 (Safari 4.0.3)

See an example here:

Comment 1 Mark Rowe (bdash) 2009-09-14 10:05:34 PDT
Comment 2 johnnyding 2009-10-27 07:33:01 PDT
bug 30813 should be duplicate of this bug
Comment 3 Simon Fraser (smfr) 2009-11-17 11:31:48 PST
*** Bug 30813 has been marked as a duplicate of this bug. ***
Comment 4 Blake Sening 2009-11-24 15:41:01 PST
This is tracked in Chromium as:

Comment 5 Saurya Velagapudi 2010-09-03 09:58:20 PDT
For those looking for a workaround try to override the overflow-y, overflow-x and overflow attributes within the body of the page which is being iframed.
Comment 6 Ojan Vafai 2010-09-20 19:14:59 PDT
To clarify, webkit only breaks when you change the scrolling property after the element has been appended to the DOM. If you load a static HTML page with the following, you don't get scrollbars:
<iframe scrolling=no></iframe>
Comment 7 Mike Lawther 2010-09-23 22:09:12 PDT
Created attachment 68645 [details]
Comment 8 Hajime Morrita 2010-09-27 22:56:07 PDT
Comment on attachment 68645 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=68645&action=review

Hi, thank you for doing this!
Although i'm not a reviewer, I'd like to have a (shadow) review:

> LayoutTests/fast/frames/iframe-scrolling-attribute-overflowscroll.html:1
> +<style>

Could you do this in a dumpAsText()-based test, instead of relies on pixel test?
For example, how about to compare clientWidth of boxes with and without scrollbars?
dumpAstText()-based test is preferable because of its maintainability.

> WebCore/page/FrameView.cpp:721
> +

Could you make the computation of vMode and hMove more local?
If we can extract it into the function, it would be fine.
Comment 9 Mike Lawther 2010-10-05 17:14:37 PDT
Hi - thanks for the review! Apologies for the delay in my response (I thought I was cc'd on this bug but I wasn't).

I've got a new patch which I'll upload shortly.

I've broken out the computation of hMode and vMode into a separate function. Hopefully this is clearer.

Thanks for the suggestion about not using pixel tests. I've rewritten the test so that it can work with just the text dump.
Comment 10 Mike Lawther 2010-10-05 18:07:09 PDT
Created attachment 69872 [details]
Comment 11 Ojan Vafai 2010-10-06 09:04:59 PDT
Comment on attachment 69872 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=69872&action=review

r- due to the layout test output. I don't really know the C++ side very well. It looks reasonable to me, but I'm not 100% it's correct.

> LayoutTests/fast/frames/iframe-scrolling-attribute-overflowscroll.html:10
> +<script src="../../resources/dump-as-markup.js"></script>

I don't think this test benefits from being dump-as-markup. dumpAsText would be fine and would make the output less bulky. Also, just spitting out the dimensions makes it harder to figure out what the correct behavior is. It would be better if it did something like:

if (expected != actual)
    output.innerHTML = "PASS";
    output.innerHTML = "FAIL: expected clientWidth " + expected + " actual clientWidth " + actual;

Where expected is the iframe's width (i.e. 200), ideally grabbed via JS (iframe.clientWidth should do) instead of hardcoded and actual is the body element's clientWidth.

Also, you can move these checks into the top-level document. I think that will make the test easier to follow than having them in the subframes. You can dig into the frames by doing iframe.contentWindow.document.body.clientWidth.

> LayoutTests/fast/frames/iframe-scrolling-attribute-overflowscroll.html:15
> +<p>This page tests the behavior of the <tt>scrolling=no</tt> attribute on
> +<tt>&lt;iframe&gt;</tt> elements which contain a page large enough to need to
> +be scrolled, and which have overflow:scroll set on the html or body tags.</p>

I'd change this to explicitly say what the correct behavior is. Something like:
This page tests that there are no scrollbars with iframe elements which have scrolling=no, contain a page large enough to need to be scrolled and have overflow:scroll set on the html or body elements. If the page doesn't have a scrollbar, then the iframe's body's clientWidth should be equal to the iframe's clientWidth.

> LayoutTests/fast/frames/iframe-scrolling-attribute-overflowscroll.html:23
> +<iframe src="resources/big-page-htmlscroll.html" scrolling="no"></iframe>
> +<iframe src="resources/big-page-bodyscroll.html" scrolling="no"></iframe>

Nit: Using resource files is fine. You could also inject the contents of the iframes on the iframes load event, i.e. onload="this.contentWindow.document.write(...)". and keep the whole test in one file. I find it easier to understand tests that don't require navigating between files, but it's not a big deal
Comment 12 Mike Lawther 2010-10-06 19:03:19 PDT
Hi Ojan - thanks for the review.

I had tried doing the test in the main page before, but kept hitting errors because the embedded resource pages were counted as from a difference site because they were accessed from file:// urls, and hence JS from the main page couldn't dig into them. Injecting everything locally neatly gets around this (yay!), so I've got everything in a single page now. I've learned something today :)

Updated patch incoming.
Comment 13 Mike Lawther 2010-10-06 19:06:55 PDT
Created attachment 70026 [details]
Comment 14 Ojan Vafai 2010-10-07 07:28:21 PDT
Comment on attachment 70026 [details]

r- for the tab characters in the test. Otherwise, the test looks fine. I prefer if someone more familiar with the C++ side review the rest if possible.

View in context: https://bugs.webkit.org/attachment.cgi?id=70026&action=review

> LayoutTests/fast/frames/iframe-scrolling-attribute-overflowscroll.html:17
> +		var content = 
> +		    '<html><head><style type="text/css">' + scrolltype + ' { overflow:scroll; }</style></head>' +
> +		    '<body><div style="width:380px; height:400px; background-color:green"></div></body></html>';
> +
> +		var doc = document.getElementById(frameId).contentDocument;

Looks like some tabs crept in here and below.

> LayoutTests/fast/frames/iframe-scrolling-attribute-overflowscroll.html:32
> +		return "expected (" + frame.clientWidth + "," + frame.clientHeight + "), " + 

Nit: might want to include the frameId in the output so people can know which frame broke.

> LayoutTests/fast/frames/iframe-scrolling-attribute-overflowscroll.html:47
> +		if (window.layoutTestController) {
> +			layoutTestController.dumpAsText();
> +		}

One-line if statements should not have curly braces.
Comment 15 Mike Lawther 2010-10-07 15:40:57 PDT
Gah - not sure how those tabs snuck in :( Fixed now.
Comment 16 Mike Lawther 2010-10-07 15:43:38 PDT
Created attachment 70160 [details]
Comment 17 James Robinson 2010-10-11 16:03:32 PDT
Comment on attachment 70160 [details]

Looks OK.  Adding Hyatt to CC list, he knows more about scrollbars than most.  Any last thoughts before I c-q+, Dave?
Comment 18 James Robinson 2010-10-14 11:42:35 PDT
Comment on attachment 70160 [details]

Let's hold off until Hyatt can take a look
Comment 19 Dave Hyatt 2010-10-15 13:06:09 PDT
Comment on attachment 70160 [details]

Some comments... seems like it would be cleaner to turn off scrollbars early and bail in the helper function rather than doing it at the very end.  That way you're not flipping stuff on and off.

In order to do that, though, you need to pull the setChildNeedsLayout/setNeedsLayout calls (which should never have been put in this code in the first place) out of the helper function and back into the main layout method.
Comment 20 Dave Hyatt 2010-10-15 13:07:25 PDT
None of this is minus-worthy, though, so could be fixed in a followup.
Comment 21 James Robinson 2010-10-15 16:21:36 PDT
Comment on attachment 70160 [details]

Mike, would you like to address some of the comments raised as a follow-up after this part lands?
Comment 22 WebKit Commit Bot 2010-10-15 16:39:16 PDT
Comment on attachment 70160 [details]

Clearing flags on attachment: 70160

Committed r69896: <http://trac.webkit.org/changeset/69896>
Comment 23 WebKit Commit Bot 2010-10-15 16:39:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Mike Lawther 2010-10-17 23:01:26 PDT
(In reply to comment #21)
> (From update of attachment 70160 [details])
> Mike, would you like to address some of the comments raised as a follow-up after this part lands?

Yep, for sure! I filed https://bugs.webkit.org/show_bug.cgi?id=47797 to track it.
Comment 25 Esteban Ortega 2011-05-19 09:22:12 PDT
On Windows using safari 5.0.5


doesn't work the iframe attribute
Comment 26 Mike Lawther 2011-05-20 07:03:24 PDT
I can confirm that with Safari 5.0.5 on Windows XP, the scrollbars on the test page (http://www.w3schools.com/TAGS/tryit.asp?filename=tryhtml_iframe_scrolling) incorrectly show when scrolling="no".

I also tested with some other WebKit browsers:

Safari 5.0.5 Windows XP : FAIL
Chrome 13.0.770.0 canary Windows XP : PASS
WebKit nightly r86920 MacOS X : PASS