Bug 61558 - iframe with scrolling=no incorrectly autoscrollable
Summary: iframe with scrolling=no incorrectly autoscrollable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 96539
  Show dependency treegraph
 
Reported: 2011-05-26 14:10 PDT by Peter Kasting
Modified: 2012-09-12 12:29 PDT (History)
14 users (show)

See Also:


Attachments
Patch to fix scrolling of frame content with "scrolling=no" (5.16 KB, patch)
2011-10-14 02:21 PDT, Swapna
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Patch to fix scrolling of frame content with "scrolling=no" - II (5.28 KB, patch)
2011-10-14 05:33 PDT, Swapna
no flags Details | Formatted Diff | Diff
Patch to fix scrolling of frame content with "scrolling=no" - III (5.30 KB, patch)
2011-10-18 22:47 PDT, Swapna
tonikitoo: review-
Details | Formatted Diff | Diff
Patch to fix scrolling of frame content with "scrolling=no", after incorporating review comments. (5.49 KB, patch)
2012-02-02 02:20 PST, Swapna
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2011-05-26 14:10:16 PDT
Load LayoutTests/fast/events/autoscroll-with-non-scrollable-parent.html .  Click in the <input> inside the iframe and drag downwards.  Notice the frame contents scroll, even though the iframe is marked scrolling=no.  I guess this is some kind of bug in RenderLayer::scrollRectToVisible().  Firefox gets this right.

Amusingly, the actual test text (visible onscreen) claims the test is checking for this bug, but the test source doesn't actually check this.
Comment 1 Mustafizur Rahaman (rahaman) 2011-06-13 08:44:50 PDT
As per discussion in webkit mailing list, I started the discussion 

Hi,

As per w3cschools (http://www.w3schools.com/tags/att_iframe_scrolling.asp ), the scrolling attribute of an iFrame element is expected to behave as per following
Attribute Values
Value 	Description
auto 	Scrollbars appear if needed (this is default)
yes 	Scrollbars are always shown (even if they are not needed)
no 	Scrollbars are never shown (even if they are needed)

My question is: When scrolling="no" & still if scroll bar is needed (i.e. if the content is larger than the iframe.), what would happen if i drag the content inside the iFrame? Will the iFrame content be scrolled OR would it not allow the user to scroll?

(https://lists.webkit.org/pipermail/webkit-dev/2011-June/017069.html)

-------------------------------------------
Antonio Gomes replied 

What does happen on other browsers? I know firefox does not scroll it (just tested), but maybe Opera and IE are good to be tested too.

Particularly speaking, I do not think it should scroll. Btw, it is a similar case to if user uses the find-in-page feature, and the word is offscreen the scrolling=no iframe. But it is a valid discussion, I think ...

(https://lists.webkit.org/pipermail/webkit-dev/2011-June/017071.html)

---------------------------------------------
Then Darin commented

HTML5 defines this scrolling attribute. It defines it in terms of a CSS overflow property on the frame below. The value scrolling="no" turns into overflow: hidden. Such overflow areas should not be scrolled.

As I understand it, generally speaking, if we have something like a find feature or autoscrolling code that scrolls outside the normal mechanism, it still should not scroll such an area and needs some other way to deal with content that is hidden by being off the edge of a scrollable area. This can happen with things like large margin values (particularly negative ones) as well as simply overflowing the space provided.

(https://lists.webkit.org/pipermail/webkit-dev/2011-June/017072.html)

Let's have the further discussion be continued here
Comment 2 Mustafizur Rahaman (rahaman) 2011-06-13 08:52:30 PDT
(In reply to comment #1)

> 
> What does happen on other browsers? I know firefox does not scroll it (just tested), but maybe Opera and IE are good to be tested too.

IE8 allows scrolling where as Opera-11 does not allow scrolling of the content.
Comment 3 Swapna 2011-10-14 02:21:35 PDT
Created attachment 110984 [details]
Patch to fix scrolling of frame content with "scrolling=no"

Hi,
As per the spec I came to know that,if iframe attribute is scrolling="no" then it should n't allow scroll of frame content.
As per my observation, when I load the mentioned test case, it is not showing any scroll bar(correct behaviour).
But when we click & drag on the content of iframe, content is getting scrolled.

In function  RenderLayer::scrollRectToVisible scroll is applied on frame content for  mouse click & drag  event.
But here before applying scroll on frame content, need to check for frame's scrolling mode.
So if we add the check for frame's scrolling mode before applying scroll( see the patch), the issue is getting resolved.

And in the test case, if frame content scrolled is "zero" then it is printing log as "FAILED".
But it should print "PASSED" as per the description provided in test case.
So I changed the test case accordingly (see the patch).
Can any one please review the patch?
Comment 4 Gyuyoung Kim 2011-10-14 02:26:06 PDT
Comment on attachment 110984 [details]
Patch to fix scrolling of frame content with "scrolling=no"

Attachment 110984 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10066467
Comment 5 Early Warning System Bot 2011-10-14 02:28:50 PDT
Comment on attachment 110984 [details]
Patch to fix scrolling of frame content with "scrolling=no"

Attachment 110984 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10056506
Comment 6 WebKit Review Bot 2011-10-14 02:51:02 PDT
Comment on attachment 110984 [details]
Patch to fix scrolling of frame content with "scrolling=no"

Attachment 110984 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10062472
Comment 7 Swapna 2011-10-14 05:33:04 PDT
Created attachment 110999 [details]
Patch to fix scrolling of frame content with "scrolling=no"  - II

Hi,
This is the patch after resolving earlier patch's build error.
Can any one please review this patch?
Comment 8 WebKit Review Bot 2011-10-14 05:36:23 PDT
Attachment 110999 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderLayer.cpp:65:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Gustavo Noronha (kov) 2011-10-14 08:01:32 PDT
Comment on attachment 110984 [details]
Patch to fix scrolling of frame content with "scrolling=no"

Attachment 110984 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10060569
Comment 10 Swapna 2011-10-18 22:47:50 PDT
Created attachment 111565 [details]
Patch to fix scrolling of frame content with "scrolling=no"  - III

Hi,
This is the patch after resolving style error in previous patch (i.e, Attachment 110999 [details]).
Can any one please review this patch along with the analysis provided in Comment #3.
Comment 11 Antonio Gomes 2011-10-20 06:54:03 PDT
Comment on attachment 111565 [details]
Patch to fix scrolling of frame content with "scrolling=no"  - III

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

> Source/WebCore/rendering/RenderLayer.cpp:1502
> +                HTMLFrameElement* frameElt = 0;
> +                Element* ownerElement = frameView->frame() ? frameView->frame()->ownerElement() : 0;
> +
> +                if (ownerElement && (ownerElement->hasTagName(frameTag) || ownerElement->hasTagName(iframeTag))) 
> +                    frameElt = static_cast<HTMLFrameElement*>(ownerElement);
> +
> +                if (frameElt->scrollingMode() != ScrollbarAlwaysOff) {

it seems like "frameElt" can be 0 here if the "if" above fails.

frameElt is also not a recommended variable name.

also you are checking ownerElement three times.

> Source/WebCore/rendering/RenderLayer.cpp:1504
> +                    LayoutRect r = getRectToExpose(viewRect, rect, alignX, alignY);

'r' is not a recommended variable name, althoug I know you are just moving existing code to within an "if" block.
Comment 12 Swapna 2011-10-20 22:54:06 PDT
>> also you are checking ownerElement three times.
Hi Thanks for reviewing. 
could you please make me understand how ownerElemnt is checked three times, so that i can resubmit the patch incorporating changes for all the comments.
Comment 13 Antonio Gomes 2011-10-21 06:54:52 PDT
Comment on attachment 111565 [details]
Patch to fix scrolling of frame content with "scrolling=no"  - III

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

>> Source/WebCore/rendering/RenderLayer.cpp:1502
>> +                if (frameElt->scrollingMode() != ScrollbarAlwaysOff) {
> 
> it seems like "frameElt" can be 0 here if the "if" above fails.
> 
> frameElt is also not a recommended variable name.
> 
> also you are checking ownerElement three times.

unless we are seeing two different 'ownerElement's objects, it is being checked three times:

> if (renderer()->document() && renderer()->document()->ownerElement() && renderer()->document()->ownerElement()->renderer()) {

> Element* ownerElement = frameView->frame() ? frameView->frame()->ownerElement() : 0;
Comment 14 Antonio Gomes 2011-10-21 12:20:30 PDT
Comment on attachment 111565 [details]
Patch to fix scrolling of frame content with "scrolling=no"  - III

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

> Source/WebCore/rendering/RenderLayer.cpp:1495
>      } else if (!parentLayer && renderer()->isBox() && renderBox()->canBeProgramaticallyScrolled()) {
>          if (frameView) {
>              if (renderer()->document() && renderer()->document()->ownerElement() && renderer()->document()->ownerElement()->renderer()) {

any alternativa way of fixing this would be making canBeProgramaticallyScrolled also take case of this check.
Comment 15 Swapna 2012-02-02 02:20:34 PST
Created attachment 125103 [details]
Patch to fix scrolling of frame content with "scrolling=no", after incorporating review comments.

Patch after incorporating review comments.

Hi Antonio,
> any alternativa way of fixing this would be making canBeProgramaticallyScrolled also take case of this check.
The attribute "scrolling" is specific to iframe & frame. Hence this needs to be handled along with iframe/frame scrolling logic. 
More over, we can differentiate mainframe & frame element using ownerElement only. And it may not be correct to access ownerelement in  canBeProgramaticallyScrolled().
Comment 16 WebKit Review Bot 2012-02-04 01:20:14 PST
Comment on attachment 125103 [details]
Patch to fix scrolling of frame content with "scrolling=no", after incorporating review comments.

Clearing flags on attachment: 125103

Committed r106730: <http://trac.webkit.org/changeset/106730>
Comment 17 WebKit Review Bot 2012-02-04 01:20:20 PST
All reviewed patches have been landed.  Closing bug.