Bug 55737

Summary: RenderBox::canBeProgramaticallyScrolled() incorrectly returns 'true' for (i)frames with attribute 'scrolling=no'
Product: WebKit Reporter: Daniel Sievers <sievers>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: jamesr
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch none

Description Daniel Sievers 2011-03-03 18:37:00 PST
Also see bug 55257.
Comment 1 Daniel Sievers 2011-03-03 18:38:59 PST
The problem is that checking 'node()->isDocumentNode()' is too permissive. The frame might have the attribute scrolling=no set and will never be scrollable.

Also see bug 55257, where this would lead to creation of an unwanted compositing layer.
Comment 2 Daniel Sievers 2011-03-03 19:05:25 PST
Created attachment 84681 [details]
Patch
Comment 3 James Robinson 2011-03-03 23:25:13 PST
Doing some quick testing it seems that iframes with scrolling=no are programmatically scrollable in Firefox nightlies + the latest Opera stable.  My testcase:

<!DOCTYPE html>
<iframe src="data:text/html;charset=utf-8,<!DOCTYPE html><script>setTimeout('window.scroll(0,10)',500)</script><div style='height:500px'>lala</div>" scrolling=no></iframe>

Are you sure we want to change this?
Comment 4 Daniel Sievers 2011-03-04 10:08:11 PST
I see, in that case it's probably ok to allow layers in bug 55257 for that case also. Thanks!

(In reply to comment #3)
> Doing some quick testing it seems that iframes with scrolling=no are programmatically scrollable in Firefox nightlies + the latest Opera stable.  My testcase:
> 
> <!DOCTYPE html>
> <iframe src="data:text/html;charset=utf-8,<!DOCTYPE html><script>setTimeout('window.scroll(0,10)',500)</script><div style='height:500px'>lala</div>" scrolling=no></iframe>
> 
> Are you sure we want to change this?
Comment 5 Darin Adler 2011-03-04 16:01:55 PST
Comment on attachment 84681 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

Why no tests? We normally require tests for all bug fixes.
Comment 6 Darin Adler 2011-03-04 16:02:26 PST
Comment on attachment 84681 [details]
Patch

I see. This bug was closed. Clearing the review flag, then.