Bug 154283 – [GTK] scroll with transparent background not repainted after scrollY >= 32768

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Jérémy Lal]

Using webkit2gtk 2.10.6
This can be tested with MiniBrowser on http://expressjs.com/en/4x/api.html

Setting document.documentElement.style.background = "white" hides the issue.

[Michael Catanzaro]

Nice bug report, this is pretty serious.

(I know we have a duplicate report of this somewhere, but I failed to find it.)

[Carlos Garcia Campos]

This is a known issue of cairo, we are hitting its coordinate space limit. When the scrollY >= 32768 the current transformation matrix has a y coordinate that is out of the coordinate space. This document paints the background using an image that is drawn with drawTiledImage. At that scroll point we simply don't render anything as background and the target surface ends up with a transparent background. Forcing the background to white fixes the problem because in that case we just fill the surface with white before rendering the contents. We should be able to workaround this by translating coordinates, but we need someone with more experience in graphics to help here.

[Gwang Yoon Hwang]

Created attachment 271986 [details]
Patch

[Gwang Yoon Hwang]

Created attachment 272084 [details]
Patch

[Gwang Yoon Hwang]

Ah, I omitted a multiplication from my local changes. That's why my previous patch didn't work.

I updated the patch and it works. :)

[Gwang Yoon Hwang]

Not fixed yet when we are using threaded compositor.

[Carlos Garcia Campos]

(In reply to comment #6)
> Not fixed yet when we are using threaded compositor.

That's not a priority at the moment, so please file a bug report for the threaded compositor blocking the meta one.

[Carlos Garcia Campos]

Comment on attachment 272084 [details]
Patch

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

Thanks! this patch fixes the test cases I was using, but seems to break other things. For example, go to www.amazon.com and move the cursor to "Hello, sign in Your Account". See the background of the Sign in button in the drop down menu, it's even worse if you hover it. I've seen similar effects in other websites.

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:209
> +    cairo_matrix_t ctm;
> +    double dx = 0, dy = 0;
> +    cairo_get_matrix(cr, &ctm);
> +    cairo_matrix_transform_point(&ctm, &dx, &dy);
> +
> +    if (dx < 0)
> +        dx = std::floor(std::abs(dx / tileRect.width())) * tileRect.width();
> +    if (dy < 0)
> +        dy = std::floor(std::abs(dy / tileRect.height())) * tileRect.height();
> +    cairo_translate(cr, dx, dy);

We should add a comment here explaining we are doing this here because of the cairo limitation.

[Michael Catanzaro]

Comment on attachment 272084 [details]
Patch

Needs to not break Amazon :)

[Carlos Garcia Campos]

It seems that the problem is when the destination rectangle is not at 0, 0 because we translate to dx, dy, but the destination rectangle is for the previous CTM. So, doing 

cairo_rectangle(cr, destRect.x() - dx, destRect.y() - dy, destRect.width(), destRect.height()); 

seems to fix the translation problem, but now the problem is that there's also a phase, so the pattern matrix is also translated. If I adjust the pattern matrix it works, but then again we can end up with a pattern matrix out of the cairo limits, so I guess I'm basically undoing the previous thing. the think is that I don't manage to make both cases work, so I'm lost again.

[Gwang Yoon Hwang]

Created attachment 277862 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #10)
> It seems that the problem is when the destination rectangle is not at 0, 0
> because we translate to dx, dy, but the destination rectangle is for the
> previous CTM. So, doing 
> 
> cairo_rectangle(cr, destRect.x() - dx, destRect.y() - dy, destRect.width(),
> destRect.height()); 
> 
> seems to fix the translation problem, but now the problem is that there's
> also a phase, so the pattern matrix is also translated. If I adjust the
> pattern matrix it works, but then again we can end up with a pattern matrix
> out of the cairo limits, so I guess I'm basically undoing the previous
> thing. the think is that I don't manage to make both cases work, so I'm lost
> again.

I made a new patch and I believe I fixed for most of the cases.

I found out that I didn't handle dx and dy if it is positive. Also, I didn't handle scale component of CTM.
And as you pointed out, we need to adjust destRect, too.
For the phase, I don't think we need to adjust the phase unless the size of tile exceeds the limit of Pixman.
Can you share the case which has a problem with a phase?

[Gwang Yoon Hwang]

(In reply to comment #12)
> (In reply to comment #10)
> > It seems that the problem is when the destination rectangle is not at 0, 0
> > because we translate to dx, dy, but the destination rectangle is for the
> > previous CTM. So, doing 
> > 
> > cairo_rectangle(cr, destRect.x() - dx, destRect.y() - dy, destRect.width(),
> > destRect.height()); 
> > 
> > seems to fix the translation problem, but now the problem is that there's
> > also a phase, so the pattern matrix is also translated. If I adjust the
> > pattern matrix it works, but then again we can end up with a pattern matrix
> > out of the cairo limits, so I guess I'm basically undoing the previous
> > thing. the think is that I don't manage to make both cases work, so I'm lost
> > again.
> 
> I made a new patch and I believe I fixed for most of the cases.
> 
> I found out that I didn't handle dx and dy if it is positive. Also, I didn't
> handle scale component of CTM.
> And as you pointed out, we need to adjust destRect, too.
> For the phase, I don't think we need to adjust the phase unless the size of
> tile exceeds the limit of Pixman.
> Can you share the case which has a problem with a phase?

Ah, I found out the problem at the border-image.

[Michael Catanzaro]

This can also be reproduced on http://backbonejs.org/.

[Miguel Gomez]

Created attachment 301043 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 301043 [details]
Patch

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

wow, thank you very much for working on this! Let's try this. Please consider adding a test in a follow up patch

> Source/WebCore/ChangeLog:19
> +        No new tests.

Sounds like we could add a new test for this?

[Miguel Gomez]

Created attachment 301047 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 301047 [details]
Patch

Rejecting attachment 301047 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 301047, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3050167

[Miguel Gomez]

Created attachment 301051 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 301051 [details]
Patch

Clearing flags on attachment: 301051

Committed r211967: <http://trac.webkit.org/changeset/211967>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jérémy Lal]

Happy to confirm the bug is no longer visible in the case i reported.

[Carlos Garcia Campos]

Reverted r211967 for reason:

Caused rendering issues in HiDPI

Committed r212346: <http://trac.webkit.org/changeset/212346>

[Miguel Gomez]

Created attachment 301744 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 301744 [details]
Patch

Let's try again!

[WebKit Commit Bot]

Comment on attachment 301744 [details]
Patch

Clearing flags on attachment: 301744

Committed r212431: <http://trac.webkit.org/changeset/212431>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.