Summary: | iframe with scrolling=no incorrectly autoscrollable | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peter Kasting <pkasting> | ||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | adele, darin, dglazkov, fsamuel, gustavo, hyatt, kbolisetty, mustaf.here, sarap.karthik, spottabathini, tonikitoo, vswap65, webkit.review.bot, xan.lopez | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 96539 | ||||||||||||
Attachments: |
|
Description
Peter Kasting
2011-05-26 14:10:16 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 (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. 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 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 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 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 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?
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 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 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 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. >> 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 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 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. 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 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> All reviewed patches have been landed. Closing bug. |