Bug 169094 – [GTK] Rendering glitches in HiDPI in long GitHub Gist pages when focusing the comments textarea

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Adrian Perez]

Steps to reproduce:
 1. Open https://gist.github.com/aperezdc/157dabed39648865b692022ec37b44bf
 2. Scroll to the bottom.
 3. Click on the textarea to write a comment.

The area of the screen occupied by the textarea will get garbled and include
fragments of the previous screenfuls of the webpage. I can reproduce the issue
in Epihany both with WebKitGTK+ releases 2.14.5 (latest stable) and 2.15.91
(latest development), and also with MiniBrowser built from trunk.

[Adrian Perez]

Created attachment 303231 [details]
Screenshot of Epiphany running 2.15.91

[Adrian Perez]

Important data point:

 - The issue happens when using a scale-factor of 2x (HiDPI).
 - With no scaling (the scale-factor is 1x) the bug is *not reproducible*.

Could this be related to bug #168128 somehow?

[Miguel Gomez]

I'll give this a look as soon as I can.

[Gwang Yoon Hwang]

This bug was reported to occur only at the HiDPI rendering case. However, when
I made a minimal test case, the test runner with a 1.0 scaled Weston instance
could reproduce this problem.

This bug happens when we are drawing the border of the inline box which has a
complex border (such as dashed, dotted..) with a border radius property.

When we made a change inside of text box, for example, typing space, we are 
going to invalidate the area of the inline box.

While rendering decorations of the render box,
RenderBoxModelObject::paintBorder defines a clip out region to prepare the inner
border to avoid the background bleeding out behind the border.

We are facing a problem from now on. 

The RenderBoxModelObject is going to make a clip to render each side of the
border area. But when we apply this clipping to the cairo context, it seems
cairo context looses the previous clipped out area, which was defined to avoid
bleeding.

It is okay when we draw dashed border side since it is a stroke. But when we
draw a solid border side, it uses fill operation which would fill the whole
quad of the border side with a border color.

I tried to make simple cairo problem to reproduce the problem but it is not so
successful so far.

[Gwang Yoon Hwang]

Created attachment 306618 [details]
If you see the red when you input the space, it faiils.

[Michael Catanzaro]

(In reply to Gwang Yoon Hwang from comment #5)
> Created attachment 306618 [details]
> If you see the red when you input the space, it faiils.

That is definitely bug #126124, but it doesn't look anything like the screenshot in comment #1. Are these the same issue?

[Gwang Yoon Hwang]

(In reply to Michael Catanzaro from comment #6)
> (In reply to Gwang Yoon Hwang from comment #5)
> > Created attachment 306618 [details]
> > If you see the red when you input the space, it faiils.
> 
> That is definitely bug #126124, but it doesn't look anything like the
> screenshot in comment #1. Are these the same issue?

Ah.. It is my mistake. It was #126124.
I was confused this with #126124. I will update that bug instead of this.
(this bug was my next target. :p)

[Gwang Yoon Hwang]

Comment on attachment 306618 [details]
If you see the red when you input the space, it faiils.

wrong test cast for this bug.

[Gwang Yoon Hwang]

Created attachment 309892 [details]
Patch

[Gwang Yoon Hwang]

(In reply to Gwang Yoon Hwang from comment #9)
> Created attachment 309892 [details]
> Patch

I made a patch and a ref-test for it.
However, I have a problem with a ref-test.
Since this bug break every renderings after it appears, I need to restart
webprocess after loading the test case not to break ref results and other test cases.
Is there any API to reset webprocess before loading expected.html?

[Gwang Yoon Hwang]

Created attachment 309894 [details]
Patch

[Gwang Yoon Hwang]

(In reply to Gwang Yoon Hwang from comment #10)
> (In reply to Gwang Yoon Hwang from comment #9)
> > Created attachment 309892 [details]
> > Patch
> 
> I made a patch and a ref-test for it.
> However, I have a problem with a ref-test.
> Since this bug break every renderings after it appears, I need to restart
> webprocess after loading the test case not to break ref results and other
> test cases.
> Is there any API to reset webprocess before loading expected.html?

No, it was enough to use non-accelerated composited mode to reset the update atlas.
Updated patch is ready for review.

[Carlos Garcia Campos]

(In reply to Gwang Yoon Hwang from comment #12)
> (In reply to Gwang Yoon Hwang from comment #10)
> > (In reply to Gwang Yoon Hwang from comment #9)
> > > Created attachment 309892 [details]
> > > Patch
> > 
> > I made a patch and a ref-test for it.
> > However, I have a problem with a ref-test.
> > Since this bug break every renderings after it appears, I need to restart
> > webprocess after loading the test case not to break ref results and other
> > test cases.
> > Is there any API to reset webprocess before loading expected.html?
> 
> No, it was enough to use non-accelerated composited mode to reset the update
> atlas.
> Updated patch is ready for review.

We should ensure tests are independent to each other, if the update atlas cache, or any other coord graphics cache might affect following tests we should ensure we clear everything when resetting tests to consistent values, adding internal API if needed. Maybe this is the reason why we had so many flaky tests when we ran all the tests with AC mode forced.

[Zan Dobersek]

Comment on attachment 309894 [details]
Patch

What's the 'limit of Pixman' here? 32768, as in r212431?

[Gwang Yoon Hwang]

(In reply to Carlos Garcia Campos from comment #13)
> (In reply to Gwang Yoon Hwang from comment #12)
> > (In reply to Gwang Yoon Hwang from comment #10)
> > > (In reply to Gwang Yoon Hwang from comment #9)
> > > > Created attachment 309892 [details]
> > > > Patch
> > > 
> > > I made a patch and a ref-test for it.
> > > However, I have a problem with a ref-test.
> > > Since this bug break every renderings after it appears, I need to restart
> > > webprocess after loading the test case not to break ref results and other
> > > test cases.
> > > Is there any API to reset webprocess before loading expected.html?
> > 
> > No, it was enough to use non-accelerated composited mode to reset the update
> > atlas.
> > Updated patch is ready for review.
> 
> We should ensure tests are independent to each other, if the update atlas
> cache, or any other coord graphics cache might affect following tests we
> should ensure we clear everything when resetting tests to consistent values,
> adding internal API if needed. Maybe this is the reason why we had so many
> flaky tests when we ran all the tests with AC mode forced.

I agree, maybe we need some codes to detect same pixman failure when running AC tests.
Also, I will create another bug to reset the update atlas for root compositing layer.

[Gwang Yoon Hwang]

(In reply to Zan Dobersek from comment #14)
> Comment on attachment 309894 [details]
> Patch
> 
> What's the 'limit of Pixman' here? 32768, as in r212431?

Yes, same here. PIXMAN_MAX_INT aka 32768.

[WebKit Commit Bot]

Comment on attachment 309894 [details]
Patch

Clearing flags on attachment: 309894

Committed r216859: <http://trac.webkit.org/changeset/216859>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.