Bug 29240

<rdar://problem/7221264>

bug 30813 should be duplicate of this bug

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

This is tracked in Chromium as: http://code.google.com/p/chromium/issues/detail?id=25912

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.

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>

Created attachment 68645 [details] Patch

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.

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.

Created attachment 69872 [details] Patch

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

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.

Created attachment 70026 [details] Patch

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.

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

Created attachment 70160 [details] Patch

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?

Comment on attachment 70160 [details] Patch Let's hold off until Hyatt can take a look

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.

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

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?

Comment on attachment 70160 [details] Patch Clearing flags on attachment: 70160 Committed r69896: <http://trac.webkit.org/changeset/69896>

All reviewed patches have been landed. Closing bug.

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

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"

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