Bug 126124 - [cairo] Entering text into forms on github.com creates a trapezoid artifact
Summary: [cairo] Entering text into forms on github.com creates a trapezoid artifact
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL: http://github.com
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-12-21 13:24 PST by Michael Catanzaro
Modified: 2019-06-14 01:07 PDT (History)
10 users (show)

See Also:


Attachments
screenshot of artifact (91.32 KB, image/png)
2013-12-21 13:39 PST, Michael Catanzaro
no flags Details
another artifact with a triangular shape (12.95 KB, image/png)
2016-09-30 02:59 PDT, Jérémy Lal
no flags Details
If you see the red when you input the space, it faiils. (798 bytes, text/html)
2017-04-09 23:25 PDT, Gwang Yoon Hwang
no flags Details
Screenshot of WinCairo WebKitLegacy (9.47 KB, image/png)
2018-12-09 18:28 PST, Fujii Hironori
no flags Details
Patch (2.95 KB, patch)
2019-06-13 06:27 PDT, Carlos Garcia Campos
mcatanzaro: review-
Details | Formatted Diff | Diff
Patch (4.12 KB, patch)
2019-06-13 09:05 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2013-12-21 13:24:06 PST
When typing in text boxes on github.com, sometimes a blue trapezoid artifact is created in the background of the text field.  This does not seem to occur in other browsers.

Unfortunately, the trapezoid disappears whenever I attempt to take a screenshot, and I'm not entirely sure what triggers it. However, a reliable reproducer -- if you have a GitHub account -- is to find some text field, hit the Enter key once or twice, then Ctrl+A to select all. The blue trapezoid appears underneath the selection, and remains if you hit the down arrow key to deselect everything.

webkitgtk3-2.2.3-1.fc20
epiphany-3.10.2-1.fc20
Comment 1 Michael Catanzaro 2013-12-21 13:39:50 PST
Created attachment 219855 [details]
screenshot of artifact

Just kidding: the blue trapezoid appeared to disappear immediately before the screenshots were taken, but it actually does appear in the screenshots. Here's one.
Comment 2 Michael Catanzaro 2015-12-13 11:09:10 PST
Can also be gray (GNOME #759376).
Comment 3 Jérémy Lal 2016-09-30 02:59:59 PDT
Created attachment 290312 [details]
another artifact with a triangular shape
Comment 4 Cédric Bellegarde 2017-02-27 01:22:49 PST
Can confirm this issue but I think composent is wrong as it doesn't happen on Safari. Should be moved to GTK.
Comment 5 Gwang Yoon Hwang 2017-04-09 23:25:27 PDT
Created attachment 306662 [details]
If you see the red when you input the space, it faiils.

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.
Comment 6 Gwang Yoon Hwang 2017-04-19 05:39:52 PDT
It is a bug of Cairo while handling clipping regions.

I made unit test cases for Cairo and made a bug:
https://bugs.freedesktop.org/show_bug.cgi?id=100722

I also made a small optimization patch which avoids the this condition,
https://bugs.webkit.org/show_bug.cgi?id=170988

But it just removes this problem when there is no scrolling area.
Comment 7 Michael Catanzaro 2017-04-19 09:14:32 PDT
OK, thanks Yoon! It looks like there are no further changes to be made in WebKit, so closing this now.
Comment 8 Sergio Villar Senin 2017-04-27 04:24:29 PDT
Why not keeping this open till the cairo bug is not fixed? I find it interesting for tracking purposes. We could use this very same bug later when updating the dependencies with the cairo version.
Comment 9 Michael Catanzaro 2017-04-27 08:06:48 PDT
Please no, only keep WebKit bugs open if a source code change to WebKit is required. The GitHub issue is fixed anyway.
Comment 10 Sergio Villar Senin 2017-04-28 02:52:05 PDT
(In reply to Michael Catanzaro from comment #9)
> Please no, only keep WebKit bugs open if a source code change to WebKit is
> required. The GitHub issue is fixed anyway.

That might be you, that is not a general policy and different people do different things.

Secondly, I don't know what you mean with fixed, but the Cairo patch is still waiting for review so still far from landing and being added to a release we could depend on.
Comment 11 Michael Catanzaro 2017-04-28 08:59:40 PDT
> Secondly, I don't know what you mean with fixed, but the Cairo patch is
> still waiting for review so still far from landing and being added to a
> release we could depend on.

Yoon committed a patch to work around the problem last week.
Comment 12 Cédric Bellegarde 2017-05-10 04:02:26 PDT
Just upgraded to webkit2gtk 2.16.2 and issue is always present.

Happen less I think but happen everytime I post a comment on github.
Comment 13 Adrian Perez 2017-05-10 07:27:12 PDT
(In reply to Cédric Bellegarde from comment #12)
> Just upgraded to webkit2gtk 2.16.2 and issue is always present.
> 
> Happen less I think but happen everytime I post a comment on github.

The shortlist of patches merged into the 2.16.2 release is here:

  https://trac.webkit.org/wiki/WebKitGTK/2.16.x

It does not seem to include the fix for this bug.
Comment 14 Adrian Perez 2017-05-10 07:28:44 PDT
(CC'ing Carlos García, who usually makes our releases.)

Carlos, do you think it would be feasible to include this fix in
an upcoming 2.16.x release?
Comment 15 Gwang Yoon Hwang 2017-05-10 07:35:26 PDT
(In reply to Adrian Perez from comment #14)
> (CC'ing Carlos García, who usually makes our releases.)
> 
> Carlos, do you think it would be feasible to include this fix in
> an upcoming 2.16.x release?

(In reply to Cédric Bellegarde from comment #12)
> Just upgraded to webkit2gtk 2.16.2 and issue is always present.
> 
> Happen less I think but happen everytime I post a comment on github.

I patched to skip border paintings in certain conditions and that patch reduces artifacts a bit.
But to fix this problem, we need to fix a clipping bug in Cairo or we need to change a way to render box decorations, which is quite different scope of work.

This bug does not include a "fix" for this bug.
Comment 16 Carlos Garcia Campos 2017-05-11 00:09:05 PDT
(In reply to Adrian Perez from comment #13)
> (In reply to Cédric Bellegarde from comment #12)
> > Just upgraded to webkit2gtk 2.16.2 and issue is always present.
> > 
> > Happen less I think but happen everytime I post a comment on github.
> 
> The shortlist of patches merged into the 2.16.2 release is here:
> 
>   https://trac.webkit.org/wiki/WebKitGTK/2.16.x

No no no. This is not the list of patches merges, this is the list of patches people request me to merge. You should add commits there once they land in trunk and you think they should be backported to stable branch. The list of commits actually merged is this:

http://trac.webkit.org/log/webkit/releases/WebKitGTK/webkit-2.16

> It does not seem to include the fix for this bug.

And it does include the workaround for this:

http://trac.webkit.org/changeset/216373/webkit/releases/WebKitGTK/webkit-2.16
Comment 17 Michael Catanzaro 2018-12-08 18:13:34 PST
(In reply to Michael Catanzaro from comment #11)
> Yoon committed a patch to work around the problem last week.

Anyway it clearly doesn't work; the trapezoid appears just like it did five years ago.

The bug has migrated: https://gitlab.freedesktop.org/cairo/cairo/issues/297

I wonder if this affects WinCairo?
Comment 18 Fujii Hironori 2018-12-09 18:28:41 PST
Created attachment 356938 [details]
Screenshot of WinCairo WebKitLegacy

(In reply to Michael Catanzaro from comment #17)
> I wonder if this affects WinCairo?

It's relatively easy to reproduce within WinCairo WK1 window with Comment 5 test case and GitHub textarea.
It's much difficult to reproduce within WinCairo WK2 window, but I'm able to reproduce it with GitHub textarea.
Comment 19 Carlos Garcia Campos 2019-06-13 06:23:58 PDT
I'm not so sure this is a cairo bug, even though it could be fixed in cairo. I think it's actually a limitation, since the case is handled in the code. See:

https://gitlab.freedesktop.org/cairo/cairo/blob/master/src/cairo-clip-polygon.c#L49

_cairo_clip_get_polygon() calls can_convert_to_polygon() and returns early with CAIRO_INT_STATUS_UNSUPPORTED if it fails. can_convert_to_polygon() simply checks that all paths in the chain have the same antialiasing. So, mixing antialiasing modes in the same clip is not actually supported by cairo. In the case of rectangle clips we are already ignoring the current antialiasing to not do any antialiasing. We could do the opposite for clips receiving a path, we want to enforce antialiasing in that case since the path might contain curves. Doing that we ensure all calls to clip with a path use the same antialiasing, which is the case of the github bug.
Comment 20 Carlos Garcia Campos 2019-06-13 06:27:45 PDT
Created attachment 372047 [details]
Patch
Comment 21 Michael Catanzaro 2019-06-13 08:35:25 PDT
Comment on attachment 372047 [details]
Patch

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

Thanks so much for fixing this!

You forgot the ChangeLog entry.

> Source/WebCore/platform/graphics/cairo/CairoOperations.cpp:1276
> -    cairo_antialias_t savedAntialiasRule = cairo_get_antialias(cr);
> -    cairo_set_antialias(cr, CAIRO_ANTIALIAS_NONE);
> -    cairo_clip(cr);
> +//    cairo_antialias_t savedAntialiasRule = cairo_get_antialias(cr);
> +//    cairo_set_antialias(cr, CAIRO_ANTIALIAS_NONE);
> +//    cairo_clip(cr);
> +    doClipWithAntialias(cr, CAIRO_ANTIALIAS_NONE);

You also forgot to remove the original code. Try again. :)
Comment 22 Carlos Garcia Campos 2019-06-13 09:05:21 PDT
(In reply to Michael Catanzaro from comment #21)
> Comment on attachment 372047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372047&action=review
> 
> Thanks so much for fixing this!
> 
> You forgot the ChangeLog entry.
> 
> > Source/WebCore/platform/graphics/cairo/CairoOperations.cpp:1276
> > -    cairo_antialias_t savedAntialiasRule = cairo_get_antialias(cr);
> > -    cairo_set_antialias(cr, CAIRO_ANTIALIAS_NONE);
> > -    cairo_clip(cr);
> > +//    cairo_antialias_t savedAntialiasRule = cairo_get_antialias(cr);
> > +//    cairo_set_antialias(cr, CAIRO_ANTIALIAS_NONE);
> > +//    cairo_clip(cr);
> > +    doClipWithAntialias(cr, CAIRO_ANTIALIAS_NONE);
> 
> You also forgot to remove the original code. Try again. :)

Oops, I actually submitted the wrong diff, let me upload the right one.
Comment 23 Carlos Garcia Campos 2019-06-13 09:05:44 PDT
Created attachment 372057 [details]
Patch
Comment 24 Carlos Garcia Campos 2019-06-14 01:03:00 PDT
Committed r246431: <https://trac.webkit.org/changeset/246431>
Comment 25 Radar WebKit Bug Importer 2019-06-14 01:07:08 PDT
<rdar://problem/51739186>