Bug 33909 – Intro text at Star Wars demo is clipped

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Adam Roben (:aroben)]

Created attachment 47045 [details]
screenshot

To reproduce:

1. Go to http://www.gesteves.com/experiments/starwars.html?nouacheck=1
2. Wait

The "A long time ago in a galaxy far, far away..." text is clipped. See screenshot.

[Adam Roben (:aroben)]

<rdar://problem/7560979>

[Enrica Casucci]

Created attachment 50962 [details]
Patch

[WebKit Review Bot]

Attachment 50962 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/win/GraphicsLayerCACF.cpp:62:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/win/GraphicsLayerCACF.cpp:63:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 50962 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 56128)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> [phone]  Enrica Casucci  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Intro text at Star Wars demo is clipped.
> +        <rdar://problem/7560979>
> +        <https://bugs.webkit.org/show_bug.cgi?id=33909>
> +        
> +        Test: compositing/repaint/opacity-animation.html
> +
> +        The origin of the dirty rectangle needs to be flipped.

I'd like to see a bit more verbage here, describing that we flip the coords for drawing, so we have to flip invalidate rects too.

> Index: WebCore/platform/graphics/win/GraphicsLayerCACF.cpp
> ===================================================================

> +    virtual void setNeedsDisplay(const CGRect* dirtyRect)

I think this method should only to rect flipping in the situation where the drawing code does flipping (i.e. check the m_owner->contentsOrientation()).

> +        CGRect layerBounds = bounds();

If dirtyRect is null and we're not showing the repaint counter, this call is wasting cycles.

> +        if (m_owner && m_owner->showRepaintCounter()) {
> +            CGRect repaintCounterRect = layerBounds;
> +            repaintCounterRect.size.width = 48;     // We assume a maximum of 4 digits.
> +            repaintCounterRect.size.height = 25;    // We use 22 as font size.
> +            repaintCounterRect.origin.y = layerBounds.size.height - (layerBounds.origin.y + repaintCounterRect.size.height);
> +            WKCACFLayer::setNeedsDisplay(&repaintCounterRect);
> +        }
> +        if (dirtyRect) {
> +            CGRect flippedDirtyRect = *dirtyRect;
> +            flippedDirtyRect.origin.y = layerBounds.size.height - (flippedDirtyRect.origin.y + flippedDirtyRect.size.height);
> +            WKCACFLayer::setNeedsDisplay(&flippedDirtyRect);
> +            return;
> +        }
> +        WKCACFLayer::setNeedsDisplay(dirtyRect);

I think this would be clearer with an 'else'.

> Index: LayoutTests/compositing/repaint/opacity-animation.html
> ===================================================================
> --- LayoutTests/compositing/repaint/opacity-animation.html	(revision 0)
> +++ LayoutTests/compositing/repaint/opacity-animation.html	(revision 0)

I think this testcase could be vastly simplified; it should end up as a green box on success, and a red or partially red box on failure.

r- to check contentsOrientation(), and I'd like to see the other stuff fixed too.

> > +            return;
> > +        }
> > +        WKCACFLayer::setNeedsDisplay(dirtyRect);
> 
> I think this would be clearer with an 'else'.

If this is referring to the last statement, then WebKit style is no “else” after “return”.

[Enrica Casucci]

Created attachment 50973 [details]
Patch2

I think I've addressed all of your comments except for the return, because I've changed the code.

[Enrica Casucci]

Created attachment 50977 [details]
patch3

[Enrica Casucci]

Created attachment 51039 [details]
Patch4

[Simon Fraser (smfr)]

Comment on attachment 51039 [details]
Patch4

> Index: LayoutTests/compositing/repaint/layer-repaint.html
> ===================================================================
> --- LayoutTests/compositing/repaint/layer-repaint.html	(revision 0)
> +++ LayoutTests/compositing/repaint/layer-repaint.html	(revision 0)
> @@ -0,0 +1,40 @@
> +<!DOCTYPE html>
> +
> +<html>
> +<head>
> +  <style type="text/css" media="screen">
> +    .container {
> +        border: 1px solid black;
> +        height: 200px;
> +        width: 200px;
> +        -webkit-transform: translateZ(0);
> +    }
> +    
> +    #tester {
> +        margin: 50px;

I think this box needs to not be centered to show the bug, so something like margin: 10px 50px 90px 50px;

> +        height: 100px;
> +        width: 100px;
> +        background-color: red;
> +    }
> +  </style>
> +  <script type="text/javascript" charset="utf-8">
> +    function doTest() {
> +        setTimeout("changeBackground();", 500);

We normally try to make tests run as fast as possible; an extra 0.5 secs is undesirable. It would be good to know if setTimeout( , 0) is enough here.

r=me with those changes to the test.

[Enrica Casucci]

(In reply to comment #9)
> (From update of attachment 51039 [details])
> > Index: LayoutTests/compositing/repaint/layer-repaint.html
> > ===================================================================
> > --- LayoutTests/compositing/repaint/layer-repaint.html	(revision 0)
> > +++ LayoutTests/compositing/repaint/layer-repaint.html	(revision 0)
> > @@ -0,0 +1,40 @@
> > +<!DOCTYPE html>
> > +
> > +<html>
> > +<head>
> > +  <style type="text/css" media="screen">
> > +    .container {
> > +        border: 1px solid black;
> > +        height: 200px;
> > +        width: 200px;
> > +        -webkit-transform: translateZ(0);
> > +    }
> > +    
> > +    #tester {
> > +        margin: 50px;
> 
> I think this box needs to not be centered to show the bug, so something like
> margin: 10px 50px 90px 50px;
> 
> > +        height: 100px;
> > +        width: 100px;
> > +        background-color: red;
> > +    }
> > +  </style>
> > +  <script type="text/javascript" charset="utf-8">
> > +    function doTest() {
> > +        setTimeout("changeBackground();", 500);
> 
> We normally try to make tests run as fast as possible; an extra 0.5 secs is
> undesirable. It would be good to know if setTimeout( , 0) is enough here.
> 
> r=me with those changes to the test.

My bad, didn't copy the latest version over from Windows that had exactly what you pointed out.

[Enrica Casucci]

Committed revision 56169.
Thanks Simon for helping me on this :-)