[Webkit-unassigned] [Bug 29240] iframes keep getting scrollbars with scrolling="no"

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=29240


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69872|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #11 from Ojan Vafai <ojan at chromium.org>  2010-10-06 09:04:59 PST ---
(From update of attachment 69872)
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><iframe></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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.