Bug 29240 – iframes keep getting scrollbars with scrolling="no"

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Korneel Wever]

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

It also appears in Webkit 531.9.1 (Safari 4.0.3)

See an example here:

http://www.w3schools.com/TAGS/tryit.asp?filename=tryhtml_iframe_scrolling

[Mark Rowe (bdash)]

<rdar://problem/7221264>

[johnnyding]

bug 30813 should be duplicate of this bug

[Simon Fraser (smfr)]

*** Bug 30813 has been marked as a duplicate of this bug. ***

[Blake Sening]

This is tracked in Chromium as:

http://code.google.com/p/chromium/issues/detail?id=25912

[Saurya Velagapudi]

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.

[Ojan Vafai]

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>

[Mike Lawther]

Created attachment 68645 [details]
Patch

[Hajime Morrita]

Comment on attachment 68645 [details]
Patch

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.

[Mike Lawther]

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.

[Mike Lawther]

Created attachment 69872 [details]
Patch

[Ojan Vafai]

Comment on attachment 69872 [details]
Patch

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";
else
    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

[Mike Lawther]

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.

[Mike Lawther]

Created attachment 70026 [details]
Patch

[Ojan Vafai]

Comment on attachment 70026 [details]
Patch

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.

[Mike Lawther]

Gah - not sure how those tabs snuck in :( Fixed now.

[Mike Lawther]

Created attachment 70160 [details]
Patch

[James Robinson]

Comment on attachment 70160 [details]
Patch

Looks OK.  Adding Hyatt to CC list, he knows more about scrollbars than most.  Any last thoughts before I c-q+, Dave?

[James Robinson]

Comment on attachment 70160 [details]
Patch

Let's hold off until Hyatt can take a look

[Dave Hyatt]

Comment on attachment 70160 [details]
Patch

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.

[Dave Hyatt]

None of this is minus-worthy, though, so could be fixed in a followup.

[James Robinson]

Comment on attachment 70160 [details]
Patch

Mike, would you like to address some of the comments raised as a follow-up after this part lands?

[WebKit Commit Bot]

Comment on attachment 70160 [details]
Patch

Clearing flags on attachment: 70160

Committed r69896: <http://trac.webkit.org/changeset/69896>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mike Lawther]

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

[Esteban Ortega]

On Windows using safari 5.0.5

http://www.w3schools.com/TAGS/tryit.asp?filename=tryhtml_iframe_scrolling

doesn't work the iframe attribute
scrolling="no"

[Mike Lawther]

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