Bug 57785 – iframe and frameset scaling is broken

• Component: New Bugs
• Status: RESOLVED FIXED

[Fady Samuel]

Frameset scaling is broken

[Fady Samuel]

Created attachment 88119 [details]
Patch

[Fady Samuel]

This patches addresses the following issues:

#1 iframes and frameset backgrounds and scroll areas are doubly scaled when they are styled with -webkit-transform.

#2 frameset does not respect -webkit-transform-origin when scaled.

I will post a new patch shortly with updated layout tests.

[Fady Samuel]

Created attachment 88694 [details]
Patch

[Fady Samuel]

Created attachment 88715 [details]
Patch

[Chris Marrin]

Comment on attachment 88715 [details]
Patch

Please add a couple of 3D transform tests. Looking at the code, I'm not convinced the right thing will happen when 3D transforms are present.

[Chris Marrin]

I actually see that you are only doing scaling and no other transform 2D or 3D (translate or rotate). You really should test for those cases.

[Simon Fraser (smfr)]

This is all very fragile code. You need to test Mac WebKit1, Mac WebKit2 and other platforms, test hit testing on iframes and plugins etc etc.

[Fady Samuel]

I'm working on another iteration of this. Additionally, I have fixed some hit testing bugs and will upload a patch that includes those in another bug.

For the time being, I have confirmed that these tests seem to render correctly on Chromium Linux, Chromium Mac AND WebKit Mac's minibrowser! The tests seem to have problems in the WebKit Layout tests on Mac however.

What differs between the layout tests and the Minibrowser that might account for this?

(In reply to comment #7)
> This is all very fragile code. You need to test Mac WebKit1, Mac WebKit2 and other platforms, test hit testing on iframes and plugins etc etc.

[Simon Fraser (smfr)]

Minibrowser is using WebKit2, which is viewless (no native widgets for iframes). DRT is using WebKit1, which has native views for iframes.

[Fady Samuel]

(In reply to comment #9)
> Minibrowser is using WebKit2, which is viewless (no native widgets for iframes). DRT is using WebKit1, which has native views for iframes.


So I have figured out how to position the RenderFrame correctly, and in fact it seems to render correctly, but a good chunk of it appears to be clipped off.  This appears to be a WebKit1 Mac specific issue. Where does clipping happen in WebKit1 Mac? Thanks.

[Fady Samuel]

(In reply to comment #10)
> (In reply to comment #9)
> > Minibrowser is using WebKit2, which is viewless (no native widgets for iframes). DRT is using WebKit1, which has native views for iframes.
> 
> 
> So I have figured out how to position the RenderFrame correctly, and in fact it seems to render correctly, but a good chunk of it appears to be clipped off.  This appears to be a WebKit1 Mac specific issue. Where does clipping happen in WebKit1 Mac? Thanks.

It seems the clipping on WebKit1 Mac is determined by WebFrameView::visibleRect in Source/WebKit/mac/WebView/WebFrameView.mm which does not correctly take into account transforms.

[Fady Samuel]

(In reply to comment #9)
> Minibrowser is using WebKit2, which is viewless (no native widgets for iframes). DRT is using WebKit1, which has native views for iframes.

This patch (copied and pasted changelog) sees to address a similar problem on WebKit1 Mac but it doesn't address framesets. Continuing to investigate.

[Fady Samuel]

(In reply to comment #12)
> (In reply to comment #9)
> > Minibrowser is using WebKit2, which is viewless (no native widgets for iframes). DRT is using WebKit1, which has native views for iframes.
> 
> This patch (copied and pasted changelog) sees to address a similar problem on WebKit1 Mac but it doesn't address framesets. Continuing to investigate.

Sorry, I somehow pressed commit instead of copying and pasting:


commit 448c931f6d9fc828515a8ba489335af221a2622b
Author: [e-mail] <[e-mail]@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date:   Mon Jan 17 01:07:24 2011 +0000

    2011-01-16  Simon Fraser  <[e-mail]>
    
            Reviewed by Dan Bernstein.
    
            Issues with iframes and plugins when the WebView is scaled.
            <rdar://problem/6213380>
    
            When _scaleWebView has been called on a WebView, iframes
            in WebKit1 render and hit-test incorrectly, and plug-ins don't scale up.
            This is caused by AppKit NSViews not playing nicely with the scale
            applied through style.
    
            Work around most of these issues by adjusting the bounds size
            of widgets to allow iframe contents to paint with the correct scale,
            and fix various places in the code where we relied on coordinate
            transforms via NSViews (which ignore CSS transforms).
    
            * WebCore.exp.in:
            * platform/Widget.cpp:
            (WebCore::Widget::setBoundsSize):
            * platform/Widget.h:
            * platform/mac/WidgetMac.mm:
            (WebCore::Widget::setBoundsSize):
            (WebCore::Widget::paint):
            * rendering/RenderLayerCompositor.cpp:
            (WebCore::RenderLayerCompositor::shouldPropagateCompositingToEnclosingIFrame):
            * rendering/RenderWidget.cpp:
            (WebCore::RenderWidget::setWidgetGeometry):
            (WebCore::RenderWidget::setWidget):
            (WebCore::RenderWidget::updateWidgetPosition):
            * rendering/RenderWidget.h:
    
    git-svn-id: http://svn.webkit.org/repository/webkit/trunk@75897 268f45cc-cd09-0410-ab3c-d52691b4dbfc

[Simon Fraser (smfr)]

The underlying issue here is that the Widget hierarchy is ignorant of transforms. Anywhere we get a widget's position or bounds is doing to do the wrong thing when there are CSS transforms above it.

The right approach would be to eliminate geometry from Widget entirely.

[Simon Fraser (smfr)]

<rdar://problem/9673843>

[Fady Samuel]

Created attachment 103434 [details]
Patch

[Fady Samuel]

(In reply to comment #9)
> Minibrowser is using WebKit2, which is viewless (no native widgets for iframes). DRT is using WebKit1, which has native views for iframes.

(In reply to comment #16)
> Created an attachment (id=103434) [details]
> Patch

Simon, this has been tested on WebKit2 Mac, and Chromium Linux platforms (included layout tests). It does not fix the problem on WebKit1 Mac. Is this current state of the patch acceptable? I need to fix the changelog, but before I take time to do that, I'll let you take a look at this patch. Thanks.

Hit testing issues have been fixed in RenderFrameSet.cpp, bool RenderFrameSet::userResize(MouseEvent* evt). 

For the purposes of this bug, I will remove those changes but I decided to post everything together so you know a fix is on its way, I don't expect you to give me an r+ for this. Rather, I'd just like your OK for all but the hit testing part, and I'll move the hit testing part to another bug report tomorrow, and add hit testing layout tests. 

Thanks,

[WebKit Review Bot]

Comment on attachment 103434 [details]
Patch

Attachment 103434 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9337479

New failing tests:
fast/overflow/scrollRevealButton.html

[Simon Fraser (smfr)]

I think the patch is going in the right direction.

[Fady Samuel]

Created attachment 103493 [details]
Patch

[WebKit Review Bot]

Comment on attachment 103493 [details]
Patch

Attachment 103493 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9332831

New failing tests:
fast/overflow/scrollRevealButton.html

[Fady Samuel]

(In reply to comment #21)
> (From update of attachment 103493 [details])
> Attachment 103493 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/9332831
> 
> New failing tests:
> fast/overflow/scrollRevealButton.html

Sigh..I did not break this test, it was broken from before..

[Fady Samuel]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 103493 [details] [details])
> > Attachment 103493 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/9332831
> > 
> > New failing tests:
> > fast/overflow/scrollRevealButton.html
> 
> Sigh..I did not break this test, it was broken from before..

*bump*

[WebKit Review Bot]

Comment on attachment 103493 [details]
Patch

Rejecting attachment 103493 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
: Unexpected image and text mismatch : (2)
  fast/overflow/scrollRevealButton.html = IMAGE+TEXT
  svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE



Full output: http://queues.webkit.org/results/9351320

[WebKit Review Bot]

Comment on attachment 103493 [details]
Patch

Rejecting attachment 103493 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
: Unexpected image and text mismatch : (2)
  fast/overflow/scrollRevealButton.html = IMAGE+TEXT
  svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE



Full output: http://queues.webkit.org/results/9371203

[WebKit Review Bot]

Comment on attachment 103493 [details]
Patch

Rejecting attachment 103493 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:
: Unexpected image and text mismatch : (2)
  fast/overflow/scrollRevealButton.html = IMAGE+TEXT
  svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE



Full output: http://queues.webkit.org/results/9352636

[Fady Samuel]

(In reply to comment #26)
> (From update of attachment 103493 [details])
> Rejecting attachment 103493 [details] from commit-queue.
> 
> Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2
> 
> Last 500 characters of output:
> : Unexpected image and text mismatch : (2)
>   fast/overflow/scrollRevealButton.html = IMAGE+TEXT
>   svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT
> 
> Regressions: Unexpected image mismatch : (5)
>   fast/text/atsui-multiple-renderers.html = IMAGE
>   fast/text/international/danda-space.html = IMAGE
>   fast/text/international/thai-baht-space.html = IMAGE
>   fast/text/international/thai-line-breaks.html = IMAGE
>   platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE
> 
> 
> 
> Full output: http://queues.webkit.org/results/9352636

It looks like the only broken test that may potentially be my fault is   fast/overflow/scrollRevealButton.html...investigating this now. 

If it's not my fault, I'll manually land this patch.

[Simon Fraser (smfr)]

*** Bug 64992 has been marked as a duplicate of this bug. ***

[Fady Samuel]

Comment on attachment 103493 [details]
Patch

Trying to submit to the commit queue on more time. I still believe this doesn't break anything new but we'll see.

[WebKit Review Bot]

Comment on attachment 103493 [details]
Patch

Rejecting attachment 103493 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
svg-fonts-word-spacing.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE

Regressions: Unexpected text diff mismatch : (1)
  http/tests/inspector/resource-har-conversion.html = TEXT



Full output: http://queues.webkit.org/results/9404300

[Fady Samuel]

(In reply to comment #30)
> (From update of attachment 103493 [details])
> Rejecting attachment 103493 [details] from commit-queue.
> 
> Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2
> 
> Last 500 characters of output:
> svg-fonts-word-spacing.html = IMAGE+TEXT
> 
> Regressions: Unexpected image mismatch : (5)
>   fast/text/atsui-multiple-renderers.html = IMAGE
>   fast/text/international/danda-space.html = IMAGE
>   fast/text/international/thai-baht-space.html = IMAGE
>   fast/text/international/thai-line-breaks.html = IMAGE
>   platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE
> 
> Regressions: Unexpected text diff mismatch : (1)
>   http/tests/inspector/resource-har-conversion.html = TEXT
> 
> 
> 
> Full output: http://queues.webkit.org/results/9404300

I've manually confirmed that all these tests are broken in HEAD on chromium-linux. Will commit this manually.

[WebKit Review Bot]

Comment on attachment 103493 [details]
Patch

Rejecting attachment 103493 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
: Unexpected image and text mismatch : (2)
  fast/overflow/scrollRevealButton.html = IMAGE+TEXT
  svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE



Full output: http://queues.webkit.org/results/9402437

[WebKit Review Bot]

Comment on attachment 103493 [details]
Patch

Rejecting attachment 103493 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
: Unexpected image and text mismatch : (2)
  fast/overflow/scrollRevealButton.html = IMAGE+TEXT
  svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE



Full output: http://queues.webkit.org/results/9403949

[Adam Barth]

> I've manually confirmed that all these tests are broken in HEAD on chromium-linux. Will commit this manually.

Dollars to donuts this patch will break a test.

[Fady Samuel]

(In reply to comment #34)
> > I've manually confirmed that all these tests are broken in HEAD on chromium-linux. Will commit this manually.
> 
> Dollars to donuts this patch will break a test.

Sigh, these tests seem to break on chromium-linux regardless...I don't really know how to proceed...

[Adam Barth]

> Sigh, these tests seem to break on chromium-linux regardless...I don't really know how to proceed...

It seems likely that your patch breaks fast/overflow/scrollRevealButton.html.  Do you have a Linux system?  If so, you can try to reproduce the failure locally.

[Fady Samuel]

(In reply to comment #36)
> > Sigh, these tests seem to break on chromium-linux regardless...I don't really know how to proceed...
> 
> It seems likely that your patch breaks fast/overflow/scrollRevealButton.html.  Do you have a Linux system?  If so, you can try to reproduce the failure locally.

I believe I tried that exact test locally without my change yesterday and it was still failing...I'll retest again today to confirm.

[Adam Barth]

> I believe I tried that exact test locally without my change yesterday and it was still failing...I'll retest again today to confirm.

Keep in mind that tests can fail in different ways.

You're welcome, of course, to land you patch, but I suspect the main tree will tell you the same thing.

[Fady Samuel]

(In reply to comment #38)
> > I believe I tried that exact test locally without my change yesterday and it was still failing...I'll retest again today to confirm.
> 
> Keep in mind that tests can fail in different ways.
> 
> You're welcome, of course, to land you patch, but I suspect the main tree will tell you the same thing.

Curiouser and curiouser, the test seems to be passing with and without my change now...cleaning out my output directory and doing a full build for sanity now.

[Fady Samuel]

Created attachment 104226 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 104226 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104226&action=review

> LayoutTests/fast/frames/frame-set-scaling-centered.html:4
> +    <frame src="data:text/html,<body bgcolor='green'></body>">
> +    <frame src="data:text/html,<body bgcolor='green'></body>">

All these tests would look cleaner with border:none style on the frames

[Fady Samuel]

(In reply to comment #41)
> (From update of attachment 104226 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104226&action=review
> 
> > LayoutTests/fast/frames/frame-set-scaling-centered.html:4
> > +    <frame src="data:text/html,<body bgcolor='green'></body>">
> > +    <frame src="data:text/html,<body bgcolor='green'></body>">
> 
> All these tests would look cleaner with border:none style on the frames

If layout tests are still failing (review bot will tell us soon) then I'll fix them in this patch. If all tests are passing, I still have a hit testing fix coming soon so I'll add a slew of new tests and I'll clean-up existing scaling tests at that point.

[WebKit Review Bot]

Comment on attachment 104226 [details]
Patch

Clearing flags on attachment: 104226

Committed r93287: <http://trac.webkit.org/changeset/93287>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.