| 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: | P2 | ||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Jaehun Lim
2013-05-09 18:01:00 PDT
Created attachment 201309 [details]
Patch
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?
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. (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 ? (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 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"? (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. (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. Created attachment 201674 [details]
Patch
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 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
Created attachment 201686 [details]
Patch
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 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 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 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
Created attachment 201807 [details]
Patch
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 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.) Created attachment 201915 [details]
Patch
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. (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 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 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. :( Created attachment 201931 [details]
Patch
Comment on attachment 201931 [details] Patch Clearing flags on attachment: 201931 Committed r150234: <http://trac.webkit.org/changeset/150234> All reviewed patches have been landed. Closing bug. |