Bug 115884

Summary:

Frame flattening prevents <HTML> in <OBJECT> from having scrollbars

Product:

WebKit

Reporter:

Jaehun Lim <ljaehun.lim>

Component:

Frames

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, kenneth, rniwa, zalan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2	none
Patch	none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion	none
Patch	none
Patch	none
Patch	none

Jaehun Lim

Reported 2013-05-09 18:01:00 PDT

WebKit does not support scrolling on a big <HTML> document in a small <OBJECT> when frame flattening is enabled.

Attachments
Patch (5.06 KB, patch) 2013-05-09 18:42 PDT, Jaehun Lim	no flags	Details Formatted Diff Diff
Patch (8.46 KB, patch) 2013-05-13 21:27 PDT, Jaehun Lim	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (895.44 KB, application/zip) 2013-05-13 23:58 PDT, Build Bot	no flags	Details
Patch (8.69 KB, patch) 2013-05-14 01:12 PDT, Jaehun Lim	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (522.59 KB, application/zip) 2013-05-14 02:00 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (534.49 KB, application/zip) 2013-05-14 02:22 PDT, Build Bot	no flags	Details
Patch (8.40 KB, patch) 2013-05-15 02:00 PDT, Jaehun Lim	no flags	Details Formatted Diff Diff
Patch (8.45 KB, patch) 2013-05-15 20:12 PDT, Jaehun Lim	no flags	Details Formatted Diff Diff
Patch (8.43 KB, patch) 2013-05-16 00:22 PDT, Jaehun Lim	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Jaehun Lim

Comment 1 2013-05-09 18:42:49 PDT

Created attachment 201309 [details] Patch

Benjamin Poulain

Comment 2 2013-05-09 22:28:09 PDT

Comment on attachment 201309 [details] Patch You should have a ref test in order to verify scrollbar rendering in addition to the scrolling behavior. Why do you handle embedded html this way instead of flattening it like frames? Which use case do you have in mind?

zalan

Comment 3 2013-05-10 00:41:42 PDT

I think the changelog is a bit misleading. It sounds like you are turning off frame flattening for <object> while flattening for <object> has never been supported. This just fixes that scrollbars get visible on _non_flattened_ <object> content so that users can scroll around. lgtm except the ref test that Benjamin mentioned.

Jaehun Lim

Comment 4 2013-05-10 02:20:23 PDT

(In reply to comment #2) > (From update of attachment 201309 [details]) > You should have a ref test in order to verify scrollbar rendering in addition to the scrolling behavior. I'll add ref tests for scrollbar rendering. > > Why do you handle embedded html this way instead of flattening it like frames? Which use case do you have in mind? I asked this issue to flattening guys on irc. Our conclusion is that the frame flattening is just for <frame> and <iframe>. <object> is not a flatten-able element. And the frame flattening should go away eventually, we don't need to expand its targets any more. How do you think ?

Jaehun Lim

Comment 5 2013-05-10 02:21:34 PDT

(In reply to comment #3) > I think the changelog is a bit misleading. It sounds like you are turning off frame flattening for <object> while flattening for <object> has never been supported. This just fixes that scrollbars get visible on _non_flattened_ <object> content so that users can scroll around. I'll make changelog more clear. > lgtm except the ref test that Benjamin mentioned. I got it.

Antonio Gomes

Comment 6 2013-05-10 05:52:58 PDT

Comment on attachment 201309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201309&action=review > Source/WebCore/ChangeLog:11 > + <object> can contain HTML document. But frame flattening prevents this document > + from creating scrollbars like <iframe> or <frame>. So we can't scroll HTML document > + in <object> when frame flattening is enabled. Frame flattening should be applied > + only to <frame> and <iframe>. The absence of scrollbars should not prevent scrolling. Where in your codepath scrolling bails out due to absence of scrollbars? > Source/WebCore/page/FrameView.cpp:540 > + if ((owner->hasTagName(frameTag) || owner->hasTagName(iframeTag)) Maybe a helper would be more "elegant"?

Jaehun Lim

Comment 7 2013-05-10 06:57:31 PDT

(In reply to comment #6) > (From update of attachment 201309 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201309&action=review > > > Source/WebCore/ChangeLog:11 > > + <object> can contain HTML document. But frame flattening prevents this document > > + from creating scrollbars like <iframe> or <frame>. So we can't scroll HTML document > > + in <object> when frame flattening is enabled. Frame flattening should be applied > > + only to <frame> and <iframe>. > > The absence of scrollbars should not prevent scrolling. Where in your codepath scrolling bails out due to absence of scrollbars? FrameView::avoidScrollbarCreation() is used in ScrollView::updateScrollbars() to decide scrollbar status. If frame flattening is enabled, avoidScrollbarCreation() returns true always, so ScrollView::setHas{Horizontal|Vertical}Scrollbar() is not called properly. It could block normal scroll operations. > > > Source/WebCore/page/FrameView.cpp:540 > > + if ((owner->hasTagName(frameTag) || owner->hasTagName(iframeTag)) > > Maybe a helper would be more "elegant"? I'll try. Thanks.

Jaehun Lim

Comment 8 2013-05-10 07:14:29 PDT

(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 201309 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=201309&action=review > > > > > Source/WebCore/ChangeLog:11 > > > + <object> can contain HTML document. But frame flattening prevents this document > > > + from creating scrollbars like <iframe> or <frame>. So we can't scroll HTML document > > > + in <object> when frame flattening is enabled. Frame flattening should be applied > > > + only to <frame> and <iframe>. > > > > The absence of scrollbars should not prevent scrolling. Where in your codepath scrolling bails out due to absence of scrollbars? > > FrameView::avoidScrollbarCreation() is used in ScrollView::updateScrollbars() to decide scrollbar status. If frame flattening is enabled, avoidScrollbarCreation() returns true always, so ScrollView::setHas{Horizontal|Vertical}Scrollbar() is not called properly. It could block normal scroll operations. (Oops. I clicked 'Commit' by mistake.) This patch makes avoidScrollbarCreation() return true only for <frame> and <iframe> when flattening is enabled. <object> tag is excepted. > > > > > > Source/WebCore/page/FrameView.cpp:540 > > > + if ((owner->hasTagName(frameTag) || owner->hasTagName(iframeTag)) > > > > Maybe a helper would be more "elegant"? > > I'll try. Thanks.

Jaehun Lim

Comment 9 2013-05-13 21:27:06 PDT

Created attachment 201674 [details] Patch

Build Bot

Comment 10 2013-05-13 23:58:10 PDT

Comment on attachment 201674 [details] Patch Attachment 201674 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/470097 New failing tests: fast/frames/flattening/scrolling-in-object.html

Build Bot

Comment 11 2013-05-13 23:58:12 PDT

Created attachment 201683 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2

Jaehun Lim

Comment 12 2013-05-14 01:12:41 PDT

Created attachment 201686 [details] Patch

Build Bot

Comment 13 2013-05-14 02:00:00 PDT

Comment on attachment 201686 [details] Patch Attachment 201686 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/460989 New failing tests: fast/frames/flattening/scrolling-in-object.html

Build Bot

Comment 14 2013-05-14 02:00:04 PDT

Created attachment 201688 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2

Build Bot

Comment 15 2013-05-14 02:22:07 PDT

Comment on attachment 201686 [details] Patch Attachment 201686 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/472106 New failing tests: fast/frames/flattening/scrolling-in-object.html

Build Bot

Comment 16 2013-05-14 02:22:10 PDT

Created attachment 201691 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.2

Jaehun Lim

Comment 17 2013-05-15 02:00:13 PDT

Created attachment 201807 [details] Patch

Antonio Gomes

Comment 18 2013-05-15 09:18:54 PDT

Comment on attachment 201807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201807&action=review LayoutTests/fast/frames/flattening/scrolling-in-object-expected.html ?! Should it be scrolling-in-object-expected.txt? > Source/WebCore/page/FrameView.cpp:519 > +static bool requiresFrameFlattening(Frame* frame) I think supportsFrameFlattenning is more appropriated.

zalan

Comment 19 2013-05-15 09:26:47 PDT

Comment on attachment 201807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201807&action=review > Source/WebCore/page/FrameView.cpp:522 > + HTMLFrameOwnerElement* owner = frame->ownerElement(); You dont check if the frame is NULL here, while you do it in frameFlatteningEnabled(). Any particular reason? (should be symmetric unless there's a good reason for not to.)

Jaehun Lim

Comment 20 2013-05-15 20:12:25 PDT

Created attachment 201915 [details] Patch

Jaehun Lim

Comment 21 2013-05-15 20:14:57 PDT

Comment on attachment 201807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201807&action=review >> Source/WebCore/page/FrameView.cpp:519 >> +static bool requiresFrameFlattening(Frame* frame) > > I think supportsFrameFlattenning is more appropriated. Fixed. >> Source/WebCore/page/FrameView.cpp:522 >> + HTMLFrameOwnerElement* owner = frame->ownerElement(); > > You dont check if the frame is NULL here, while you do it in frameFlatteningEnabled(). Any particular reason? (should be symmetric unless there's a good reason for not to.) Added null checking codes.

Jaehun Lim

Comment 22 2013-05-15 20:18:43 PDT

(In reply to comment #18) > (From update of attachment 201807 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201807&action=review > > LayoutTests/fast/frames/flattening/scrolling-in-object-expected.html ?! > > Should it be scrolling-in-object-expected.txt? I verifed failed->passed after this patch. Is it not enough ? .txt is needed? > > > Source/WebCore/page/FrameView.cpp:519 > > +static bool requiresFrameFlattening(Frame* frame) > > I think supportsFrameFlattenning is more appropriated.

zalan

Comment 23 2013-05-15 23:53:53 PDT

Comment on attachment 201915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201915&action=review > LayoutTests/fast/frames/flattening/scrolling-in-object.html:11 > + if (window.eventSender) { check for window.testRunner too > LayoutTests/fast/frames/flattening/scrolling-in-object.html:22 > + <body style="margin:0" onload="setTimeout('runTest()', 100);"> any reason why it needs to by async? we should not slow down layouttest unless there's a reason to do so. if you are relying on a layout to be finished, force sync layout instead.

Jaehun Lim

Comment 24 2013-05-16 00:20:00 PDT

Comment on attachment 201915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201915&action=review >> LayoutTests/fast/frames/flattening/scrolling-in-object.html:11 >> + if (window.eventSender) { > > check for window.testRunner too Fixed. >> LayoutTests/fast/frames/flattening/scrolling-in-object.html:22 >> + <body style="margin:0" onload="setTimeout('runTest()', 100);"> > > any reason why it needs to by async? we should not slow down layouttest unless there's a reason to do so. if you are relying on a layout to be finished, force sync layout instead. I tried "sync". At that time, no problems in efl port but failed in mac and mac-wk2. Please, see my previous patches those were failed. Scrolling in <object> was not working at all in mac ports. After using "setTimeout('runTest()', 100)", tests are passed. I don't know why. I'm not familiar with mac. :(

Jaehun Lim

Comment 25 2013-05-16 00:22:20 PDT

Created attachment 201931 [details] Patch

WebKit Commit Bot

Comment 26 2013-05-16 20:13:40 PDT

Comment on attachment 201931 [details] Patch Clearing flags on attachment: 201931 Committed r150234: <http://trac.webkit.org/changeset/150234>

WebKit Commit Bot

Comment 27 2013-05-16 20:13:45 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.