Bug 115884 - Frame flattening prevents <HTML> in <OBJECT> from having scrollbars
Summary: Frame flattening prevents <HTML> in <OBJECT> from having scrollbars
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-09 18:01 PDT by Jaehun Lim
Modified: 2013-05-16 20:13 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jaehun Lim 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.
Comment 1 Jaehun Lim 2013-05-09 18:42:49 PDT
Created attachment 201309 [details]
Patch
Comment 2 Benjamin Poulain 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?
Comment 3 zalan 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.
Comment 4 Jaehun Lim 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 ?
Comment 5 Jaehun Lim 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.
Comment 6 Antonio Gomes 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"?
Comment 7 Jaehun Lim 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.
Comment 8 Jaehun Lim 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.
Comment 9 Jaehun Lim 2013-05-13 21:27:06 PDT
Created attachment 201674 [details]
Patch
Comment 10 Build Bot 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
Comment 11 Build Bot 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
Comment 12 Jaehun Lim 2013-05-14 01:12:41 PDT
Created attachment 201686 [details]
Patch
Comment 13 Build Bot 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
Comment 14 Build Bot 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
Comment 15 Build Bot 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
Comment 16 Build Bot 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
Comment 17 Jaehun Lim 2013-05-15 02:00:13 PDT
Created attachment 201807 [details]
Patch
Comment 18 Antonio Gomes 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.
Comment 19 zalan 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.)
Comment 20 Jaehun Lim 2013-05-15 20:12:25 PDT
Created attachment 201915 [details]
Patch
Comment 21 Jaehun Lim 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.
Comment 22 Jaehun Lim 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.
Comment 23 zalan 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.
Comment 24 Jaehun Lim 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. :(
Comment 25 Jaehun Lim 2013-05-16 00:22:20 PDT
Created attachment 201931 [details]
Patch
Comment 26 WebKit Commit Bot 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>
Comment 27 WebKit Commit Bot 2013-05-16 20:13:45 PDT
All reviewed patches have been landed.  Closing bug.