1401653 - wr-text: implement decoration fusion(?)

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[Aria Beingessner [:Gankra]]

These two reftests fail with webrendest layers-free. They appear to be testing some sort of "decoration fusion" behaviour, that I don't really understand yet.

layout/reftests/text-decoration/complex-decoration-style-quirks.html
layout/reftests/text-decoration/complex-decoration-style-standards.html

Comment 1

[Aria Beingessner [:Gankra]]

Hey jonathan, any idea what these tests are about?

Comment 2

[Jonathan Kew [:jfkthame]]

It looks like they're testing how decorations (particularly "complex" ones -- dotted, dashed or wavy lines) are drawn across element boundaries, to check that we don't get discontinuities in what should be a continuous line, and that the decorations are offset appropriately in response to position:relative and shadow offsets.

What do the failures you're seeing look like? Do there seem to be glitches at the boundaries between various <span>s in the testcases?

Comment 3

[Aria Beingessner [:Gankra]]

Attachment: decorations.png

The decorations seem to have overlapping ranges, and get all messed up. It's possible we need to be drawing the decorations so that this overlap is invisible? That or my code to grab the bounds of the lines is wrong (very possible).

Comment 4

[Jonathan Kew [:jfkthame]]

The overlaps are one issue, but in addition the various parts seem to be out of phase with each other.

Comment 5

[Aria Beingessner [:Gankra]]

Well each decoration starts at the same position, which seemed to be what gecko did. Is the period of the decoration required to satisfy some specific relationship with its height (thickness) to make this Just Work?

Comment 6

[Aria Beingessner [:Gankra]]

Finally figured out what's going on here. Gecko is being Very Smart in its line drawing code, and we're breaking the assumptions there. Basically gecko tries to align periodic patterns to a period-multiple offset from some parent element. Then applies clips to keep lines from smashing into eachother. This creates the illusion of a single coherent line across elements and style changes.

This not working with WR is a mix of three issues:

* Upstream wr doesn't currently match up its wavy/dashed line period with gecko's
* We don't push/pop clips for TextDrawTarget (before this was hard, now it's easy)
* I didn't actually pass the right dimensions for wavy lines from gecko to wr (so if we did apply the clip, we get bad results)

Comment 7

[Aria Beingessner [:Gankra]]

Attachment: Bug 1401653 - fixup webrender text-decoration bindings.

This does 3 things that were all a bit too intermixed to split out cleanly:

1. Teaches TextDrawTarget to handle rectangular clips (while also completely
forbidding other ones). This is necessary to handle how gecko "overdraws"
decorations with clips to create the illusion of continuous lines when they're
actually made out of multiple lines, possibly from different display items
with different lines. Previously gecko *was* handing these clips to
TextDrawTarget to use these clips, but we were just ignoring them.

This is also necessary work to support partial glyphs natively (which apply
rectangular clips to glyphs). Also note that this currently causes a bug
in webrender if combined with zero-blur shadows, but it's not a regression
since we already mishandle clipped decorations. I will work on fixing this
upstream.

2. Changes the intermediate representation of lines from the old webrender
format to a rect-based one. This is in preperation for webrender adopting
that format in a future update.

3. Changes the way wavy lines are processed, correcting some errors in the
old wavy line bindings that lead to them being positioned incorrectly. Also
introduces a wavyLineThickness property that the will be required in a
future webrender update. Wavy lines are unlike any other line, so it's
ultimately desirable to distinguish them.

The net result of these changes is that a companion upstream change (webrender#1923)
will make decoration rendering nearly identical to gecko, and much nicer.
However the clipped shadows issue will need to be seperately resolved before
actually closing this issue.

Review commit: https://reviewboard.mozilla.org/r/192638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192638/

Comment 8

[Aria Beingessner [:Gankra]]

I was already working on this before it was marked as P3.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8921639 [details]
Bug 1401653 - fixup webrender text-decoration bindings.

https://reviewboard.mozilla.org/r/192638/#review197830

Comment 10

[Aria Beingessner [:Gankra]]

Try build to make sure there's no collateral damage (I don't expect any):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22551950c875ce73e9137ab8fefbdb49e3c94184

Comment 11

[Aria Beingessner [:Gankra]]

Try looks good.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

You should upgrade your cbindgen to .26 or newer; I think a bunch of the generated changes in this patch are artifacts of your using the older cbindgen.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5055d2703f2f
fixup webrender text-decoration bindings. r=jrmuizel

Comment 14

[Attila Craciun [:arny]]

Backed out 1 changesets (bug 1401653) for failing no matching function for call in src/layout/generic/TextDrawTarget.h:143:43 r=backout on a CLOSED TREE.

https://hg.mozilla.org/integration/autoland/rev/d461464bb7abace2d7343db7d95e5bfd8ca6b99c

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5055d2703f2f2a95674934d263c56eb874f949a6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable

Comment 15

[Aria Beingessner [:Gankra]]

API change landed before me, fixed (added two Nothings to the call)

Comment 16

[Aria Beingessner [:Gankra]]

Comment on attachment 8921639 [details]
Bug 1401653 - fixup webrender text-decoration bindings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192638/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27ac538d3d8b
fixup webrender text-decoration bindings. r=jrmuizel

Comment 18

[Attila Craciun [:arny]]

https://hg.mozilla.org/mozilla-central/rev/27ac538d3d8b

Comment 19

[Aria Beingessner [:Gankra]]

Whoops, forgot to reopen this bug -- all the work in gecko is done, but upstream webrender work needs to get done to fix one of the cases in these tests and make them actually pass.

Specifically https://github.com/servo/webrender/issues/1928 needs to get fixed.

Fixing the linked issue will now also fix text-shadow/decorations-multiple-zorder.html since Bug 1399310 just landed, making us use clipped decorations in more places.

Comment 20

[Marco Mucci [:MarcoM]]

Deprioritized to P3 during triage - moving to Reserve Backlog.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

Gankro, what's the status of this bug?

Comment 22

[Aria Beingessner [:Gankra]]

bugs fixed; tests just need minor fuzzing, looks like

Comment 23

[Aria Beingessner [:Gankra]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac770b9bee47605d299c36e1edbb0f0def8f7e76

Comment 24

[Aria Beingessner [:Gankra]]

opening new bug for test expectations