Bug 29240 - iframes keep getting scrollbars with scrolling="no"
: iframes keep getting scrollbars with scrolling="no"
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 420+
: PC Windows Vista
: P2 Normal
Assigned To:
: http://www.w3schools.com/TAGS/tryit.a...
: HasReduction, InRadar, ReviewedForRadar
:
:
  Show dependency treegraph
 
Reported: 2009-09-14 02:14 PST by
Modified: 2011-05-20 07:03 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-09-14 02:14:08 PST
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
------- Comment #1 From 2009-09-14 10:05:34 PST -------
<rdar://problem/7221264>
------- Comment #2 From 2009-10-27 07:33:01 PST -------
bug 30813 should be duplicate of this bug
------- Comment #3 From 2009-11-17 11:31:48 PST -------
*** Bug 30813 has been marked as a duplicate of this bug. ***
------- Comment #4 From 2009-11-24 15:41:01 PST -------
This is tracked in Chromium as:

http://code.google.com/p/chromium/issues/detail?id=25912
------- Comment #5 From 2010-09-03 09:58:20 PST -------
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 From 2010-09-20 19:14:59 PST -------
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 From 2010-09-23 22:09:12 PST -------
Created an attachment (id=68645) [details]
Patch
------- Comment #8 From 2010-09-27 22:56:07 PST -------
(From update of 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 From 2010-10-05 17:14:37 PST -------
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 From 2010-10-05 18:07:09 PST -------
Created an attachment (id=69872) [details]
Patch
------- Comment #11 From 2010-10-06 09:04:59 PST -------
(From update of 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";
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
------- Comment #12 From 2010-10-06 19:03:19 PST -------
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 From 2010-10-06 19:06:55 PST -------
Created an attachment (id=70026) [details]
Patch
------- Comment #14 From 2010-10-07 07:28:21 PST -------
(From update of 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 From 2010-10-07 15:40:57 PST -------
Gah - not sure how those tabs snuck in :( Fixed now.
------- Comment #16 From 2010-10-07 15:43:38 PST -------
Created an attachment (id=70160) [details]
Patch
------- Comment #17 From 2010-10-11 16:03:32 PST -------
(From update of 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 From 2010-10-14 11:42:35 PST -------
(From update of attachment 70160 [details])
Let's hold off until Hyatt can take a look
------- Comment #19 From 2010-10-15 13:06:09 PST -------
(From update of 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 From 2010-10-15 13:07:25 PST -------
None of this is minus-worthy, though, so could be fixed in a followup.
------- Comment #21 From 2010-10-15 16:21:36 PST -------
(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?
------- Comment #22 From 2010-10-15 16:39:16 PST -------
(From update of attachment 70160 [details])
Clearing flags on attachment: 70160

Committed r69896: <http://trac.webkit.org/changeset/69896>
------- Comment #23 From 2010-10-15 16:39:23 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #24 From 2010-10-17 23:01:26 PST -------
(In reply to comment #21)
> (From update of attachment 70160 [details] [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 From 2011-05-19 09:22:12 PST -------
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"
------- Comment #26 From 2011-05-20 07:03:24 PST -------
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