reftests/writing-mode/tables/vertical-border-collapse-2.html still fails with webrender after bug 1393907

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Blocks 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments)

Looks like we don't render a bunch of the collapsed borders, I'll take a look.
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Probably? I'm finishing up a patch, are you working on that one?
Flags: needinfo?(a.beingessner)
nope, go ahead
Flags: needinfo?(a.beingessner)
Duplicate of this bug: 1461054
The previous border-collapse beveling implementation assumed that there would
only be one beveled border per side in the whole table, which is... not true at all.

So a bunch of borders ended up clobbering other values in mBevelBorders and
never getting painted.

I'm actually somewhat scarily surprised that only this reftest seems to fail
without this patch...

Here we reuse most of the existing one-off beveling / border rendering support
in nsCSSRendering, and convert the Gecko bevels into a WebRender display list
using rects and borders. This is only remotely possible thanks to Gecko not
supporting dotted / dashed beveled borders :)

This would slightly easier and presumably also more efficient with a triangle
display item in WR instead of (ab)using the border display item to render the
bevel, but this is probably relatively edge-casey so maybe not worth it... In
any case I've left a TODO comment there, that can be a nice followup if we deem
it worth it.

Anyway, I'm _so_ sorry for the border trick, I was this (||) close to go and
rewrite our border collapsing code, but after a few tries I realized it'd take
me a whole lot of time (instead of the day that this has taken me).
Flags: needinfo?(emilio)
Comment on attachment 9005709 [details]
Properly support beveling bc borders in WR.

Alexis Beingessner [:Gankro] has approved the revision.
Attachment #9005709 - Flags: review+
Looks like I need to fix the snapping if I want to land this... Oh well, will look at that on Monday.

Also, this actually fixes a fair amount of tests which used to render black borders and now actually render the borders they're supposed to render.
So I talked with Nical and turns out that it's going to be hard / impossible to draw the borders antialiased without the issues that can be seen in this try run:

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

Looks like Gecko doesn't anti-alias the bevel to avoid that, so it seems reasonable to avoid anti-aliasing it rather than rewriting the border-collapsing code, maybe...
Blocks: 1488294
Posted file GitHub Pull Request
Finally got time to look into this again. I need non-antialising corners to implement this, but I got a patch that's green.
See Also: → 1440192
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/198999ea1b79
Properly support beveling bc borders in WR. r=Gankro
https://hg.mozilla.org/mozilla-central/rev/198999ea1b79
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1521066
You need to log in before you can comment on or make changes to this bug.