iframe with scrolling=no incorrectly autoscrollable

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Peter Kasting]

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.

[Mustafizur Rahaman (rahaman)]

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

[Mustafizur Rahaman (rahaman)]

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

[Swapna]

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?

[Gyuyoung Kim]

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

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[Swapna]

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?

[WebKit Review Bot]

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.

[Gustavo Noronha (kov)]

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

[Swapna]

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.

[Antonio Gomes]

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.

[Swapna]

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

[Antonio Gomes]

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;

[Antonio Gomes]

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.

[Swapna]

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

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.