1356114 - Make opposite CSS dashed borders symmetrical in various cases where asymmetry is ugly

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Karl Dubost💡 :karlcow]

Attachment: hr test firefox safari

When we use border to created a dashed hr, the border is drawn unevenly around the box, creating a wavy effect, not beautiful to the eye. This issue happens only for borders at 1px. When the width is larger the drawing is regular and evenly spaced out.


Tested with Firefox Nightly 55.0a1 (2017-04-10) (64-bit)
Created a test case http://codepen.io/webcompat/pen/VbwGaW
This is a spin-off of the Webcompat issue https://webcompat.com/issues/5616

Simple markup to reproduce
<hr style="width: 200px; border-width: 1px; border-style: dashed;">

Screenshot attached.
Firefox on the left, Safari on the right.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

cc :arai since she fixed bug 382721 so may have some insight on what should happen here.

Comment 2

[Tooru Fujisawa [:arai]]

for width<2px dashed border, we simply draw dashed path along the border, in the following direction, and it doesn't guarantee symmetricity (just for performance reason), in nsCSSBorderRenderer::DrawDashedOrDottedSide.

.--->.
^    |
|    |
|    v
.<---.

maybe we could change the direction to the following, so that both horizontal lines (or both vertical lines) are rendered samely, if there's no radius.

.--->.
|    |
|    |
v    v
.--->.

or the following to achieve symmetricity too.

.<-.->.
^     ^
|     |
.     .
|     |
v     v
.<-.->.


or perhaps, we could tweak spacing of dashed pattern to make the border symmetric, in nsCSSBorderRenderer::SetupDashedOptions.

Comment 3

[Tooru Fujisawa [:arai]]

looks like there's some bug inside nsCSSBorderRenderer::SetupDashedOptions and it doesn't allocate enough length for the segment at the end of the top border.
I'll look into it.

Comment 4

[Tooru Fujisawa [:arai]]

to be clear, nsCSSBorderRenderer::SetupDashedOption is already trying to calculate the dashed line segments' length so that it looks symmetric, and I thought it's something related to precision error because of the dashed segment is small compared to border length, but apparently there's simpler issue in the calculation.

Comment 5

[Tooru Fujisawa [:arai]]

this part seems to be suspicious

https://dxr.mozilla.org/mozilla-central/rev/130efc657df7e7fe291cc42307f3eb3cb0484dfc/layout/painting/nsCSSRenderingBorders.cpp#1873-1880
>   if (borderWidth < 2.0f) {
>     // Round start to draw dot on each pixel.
>     if (IsHorizontalSide(aSide)) {
>       start.x = round(start.x);
>     } else {
>       start.y = round(start.y);
>     }
>   }

it tries to to align the start of the dashed line on the pixel boundary, so that the dashed line is rendered without anti-aliased gray pixel, but because of the asymmetricity how each border is rendered, the affected point differs between opisite sides

top border:    .--->.
               ^
               |
               this point is aligned

bottom border: .<---.
                    ^
                    |
                    this point is aligned

I'll try using the following way at first.

.--->.
|    |
|    |
v    v
.--->.

Comment 6

[Tooru Fujisawa [:arai]]

nah, the asymmetricity comes from here:

https://dxr.mozilla.org/mozilla-central/rev/130efc657df7e7fe291cc42307f3eb3cb0484dfc/layout/painting/nsCSSRenderingBorders.cpp#3381-3402
>     /*
>      * If we have a 1px-wide border, the corners are going to be
>      * negligible, so don't bother doing anything fancy.  Just extend
>      * the top and bottom borders to the right 1px and the left border
>      * to the bottom 1px.  We do this by twiddling the corner dimensions,
>      * which causes the right to happen later on.  Only do this if we have
>      * a 1.0 unit border all around and no border radius.
>      */
> 
>     NS_FOR_CSS_FULL_CORNERS(corner) {
>       const mozilla::Side sides[2] = { mozilla::Side(corner), PREV_SIDE(corner) };
> 
>       if (!IsZeroSize(mBorderRadii[corner]))
>         continue;
> 
>       if (mBorderWidths[sides[0]] == 1.0 && mBorderWidths[sides[1]] == 1.0) {
>         if (corner == eCornerTopLeft || corner == eCornerTopRight)
>           mBorderCornerDimensions[corner].width = 0.0;
>         else
>           mBorderCornerDimensions[corner].height = 0.0;
>       }
>     }

This algorithm doesn't work properly with dashed/dotted.

Comment 7

[Tooru Fujisawa [:arai]]

to be clear, because of that adjustment, the border length of top/bottom borders differ.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: current mBorderCornerDimensions twiddling for 1px-wide border

this is what happens now.
the length of top/bottom borders differ because of this, and the dashed pattern also differ.
I think we could just suppress this when horizontal or vertical border for the given corner is dashed or dotted.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: WIP rendering result

try is running:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52abc1c2982e15078c61dde84348d73ec74e67cb

it would hit failure on some test that relies on previous behavior (I just should update those tests), and I need to add testcase for this change.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: patched mBorderCornerDimensions twiddling for 1px-wide border

this is what the patch does

Comment 11

[Tooru Fujisawa [:arai]]

now investigating the failure for object-fit-fill-png-001c.html etc
(I'm not sure why it happens at this point...)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=619f0c7e4e760b7271e87a03e22160ef98ac44bc

Comment 12

[Tooru Fujisawa [:arai]]

hmm, I don't see any difference in the parameter while rendering dashed border for object-fit-fill-png-001c.html and object-fit-fill-png-001-ref.html, but the rendering result has differences in the gray color (#c0c0c0 vs #bfbfbf)...
and I don't see the issue when I open the file with built binary...
I'll try building it locally.

Comment 13

[Tooru Fujisawa [:arai]]

the only difference I see is that, the border is rendered twice for object-fit-fill-png-001c.html.
but I cannot figure out how it results in #c0c0c0 vs #bfbfbf, instead of twice darker color, also appears only in specific part...

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: object-fit-fill-png-001-ref.html result

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: object-fit-fill-png-001c.html result

Comment 16

[Tooru Fujisawa [:arai]]

the issue is reproducible with local build, but only in reftest.
when I open those files in normal session, the issue doesn't happen.

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: the color of the anti-aliased part of the dashed line

here's the difference

Comment 18

[Tooru Fujisawa [:arai]]

looks like the fact that the border is rendered twice is not related.
the failure happens even if I skip the first rendering.

Comment 19

[Tooru Fujisawa [:arai]]

interestingly, the reftest fails even if I use the same content (object-fit-fill-png-001-ref.html) for both TEST and REFERENCE.
so I think there's some trick happening while rendering the dashed line.

Comment 20

[Tooru Fujisawa [:arai]]

I'm about to allow fuzziness for now, since I think this is not actually a regression from this change, but just discovered by the change.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a16e5ac6eeb3ecc5af63c65aa98b547b86cadf25

Comment 21

[Tooru Fujisawa [:arai]]

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

Comment 22

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c15f651802e5c65e5dd1b707a3308fd131073bd0&group_state=expanded
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60e964a95d5cd7cfa63f6ad57fa4949827cbfab4&group_state=expanded

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Adjust the border corner dimension so that the border becomes symmetric.

the issue is described in comment #6, comment #8,
and the solution is described in https://bug1356114.bmoattachments.org/attachment.cgi?id=8874266

so, allocating same area for both sides solves the asymmetricity issue.

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Start and end with full dashed segment if the box is has 2px width or height.

also, tweaked the line ends for those cases, to start and end with full dashed segment.

Comment 25

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Update border dashed testcase.

updated the testcase mask

Comment 26

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Allow fuzziness for dashed line in reftest.

then, I hit possible skia bug that the anti-aliased color becomes different for exactly same parameters for dashed line.

I've added fuzzy-if for affected testcases, but this is not covering all testcases that uses dashed border (there are so many test cases that uses dashed border even if it's not about border), and I'm not sure whether files covered by this patch is sufficient or not...

Comment 27

[Jonathan Watt [:jwatt]]

Astley, what was the logic behind marking this P1? The asthetics of 1px borders on <hr> seem a bit of an edge case so I haven't given this high priority so far.

Comment 28

[Astley Chen (inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #27)
> Astley, what was the logic behind marking this P1? The asthetics of 1px
> borders on <hr> seem a bit of an edge case so I haven't given this high
> priority so far.

Generally speaking, for webcompat(author/user facing) issues I will give them at least P2 as we will address them sooner or later based on priorities in our work queue.
For this bug, sorry I originally meant to mark it as P2, my bad.

I agree that this bug seems minor and probably an edge case, given that there are patches waiting for review, it would be great if you have spare time to progress it or just direct to other reviewers if you are busy.

Comment 29

[Tooru Fujisawa [:arai]]

Attachment: Part 1.1: Update comment for border corner dimension for 1px borders.

As discussed on IRC, updated the comment above the Part 1 change.

Comment 30

[Tooru Fujisawa [:arai]]

try run on current central
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=21886336d8d72a2ee5231c086890f4fd025e473b&group_state=expanded

there are some tests that fails with similar fuzziness.
(that might imply that the similar fuzziness can start happenning after landing this, with (almost-)unrelated change


also try run with removing non-symmetric case:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d245465cfa392aa559ff43fba5b0bfb3167f79ed&group_state=expanded

Comment 31

[Tooru Fujisawa [:arai]]

also try run only with Part 1 to see when the fuzziness started happening

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

Comment 32

[Tooru Fujisawa [:arai]]

apparently the fuzziness started happening in Part 2.

Comment 33

[Tooru Fujisawa [:arai]]

the set of fuzzy testcases differs for each run in comment #30, but all of them are object-fit-*-png-*.html, that shares same structure.
I think I should add fuzzy-if to more tests (maybe all tests that shares same structure...?)

Comment 34

[Jonathan Watt [:jwatt]]

(In reply to Tooru Fujisawa [:arai] from comment #29)
> Created attachment 8893461 [details] [diff] [review]
> Part 1.1: Update comment for border corner dimension for 1px borders.
> 
> As discussed on IRC, updated the comment above the Part 1 change.

Thanks!

Comment 35

[Tooru Fujisawa [:arai]]

for future reference, the issue doesn't happen on retina display (since the fast path for 1px border width is not used).
similar issue can be reproduced on retina display by zooming out, to make the border width thinner.

Comment 36

[Tooru Fujisawa [:arai]]

try run without removing |borderWidth <= 1.0f| condition in Part 2
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96d42b95ae38c19eaa26163afd31187a4b563eff

I hope this can avoid the fuzziness.

Comment 37

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Adjust the border corner dimension so that the border becomes symmetric.

Merged 1.1 and also removed old behavior

Comment 38

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Start and end with full dashed segment if the box is has 2px width or height.

Reverted the change to not-thin box.
so the not-thin box keeps non-symmetric but the case in comment #0 (thin box) gets fixed.

this doesn't cause fuzziness :)

Comment 39

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Update border dashed testcase.

updated reference mask

Comment 40

[Tooru Fujisawa [:arai]]

I'll file followup bug for symmetricity for not-thin box later.

Comment 41

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Update border dashed testcase.

forgot to refresh the queue

Comment 42

[Jonathan Watt [:jwatt]]

Comment on attachment 8893730 [details] [diff] [review]
Part 1: Adjust the border corner dimension so that the border becomes symmetric.

Review of attachment 8893730 [details] [diff] [review]:
-----------------------------------------------------------------

Please change the comment to:

  Bug 1356114 - Part 1: Change the code that optimizes away CSS border corner painting to ensure opposite borders have the same length. r=jwatt

  See https://bugzilla.mozilla.org/show_bug.cgi?id=1356114#c8
  for a diagram describing the old behavior.

::: layout/painting/nsCSSRenderingBorders.cpp
@@ +3382,5 @@
> +    // going to be negligible, so don't bother doing anything fancy. Just
> +    // extend the borders along  the longer sides to keep the borders on
> +    // opposite sides the same length, so that dashed/dotted lines become
> +    // symmetrical. We do this by twiddling the corner dimensions, which causes
> +    // the right to happen later on.

Please change this comment to:

    // The corner is going to have negligible size if its two adjacent border
    // sides are only 1px wide and there is no border radius.  In that case we
    // skip the overhead of painting the corner by setting the width or height
    // of the corner to zero, which effectively extends one of the corner's
    // adjacent border sides.  We extend the longer adjacent side so that
    // opposite sides will be the same length, which is necessary for opposite
    // dashed/dotted sides to be symmetrical.

@@ +3385,5 @@
> +    // symmetrical. We do this by twiddling the corner dimensions, which causes
> +    // the right to happen later on.
> +    //
> +    //   if width > height
> +    //     +--+--------+--+    +--------------+

Please add a bit more width to these first two border diagrams to make them wider.

@@ +3408,5 @@
> +    //     |  |        |  |    |  |        |  |
> +    //     |  |        |  |    |  |        |  |
> +    //     +--+--------+--+    |  +--------+  |
> +    //     |  |        |  |    |  |        |  |
> +    //     +--+--------+--+    +--+--------+--+

After these diagrams please append:

    // Note that if we have different border widths we could end up with
    // opposite sides of different length.  For example, if the left and
    // bottom borders are 2px wide instead of 1px, we will end up doing
    // something like:
    //
    //     +----+------------+--+    +----+---------------+
    //     |    |            |  |    |    |               |
    //     +----+------------+--+    +----+------------+--+
    //     |    |            |  |    |    |            |  |
    //     |    |            |  | => |    |            |  |
    //     |    |            |  |    |    |            |  |
    //     +----+------------+--+    +----+------------+--+
    //     |    |            |  |    |    |            |  |
    //     |    |            |  |    |    |            |  |
    //     +----+------------+--+    +--------------------+
    //
    // XXX Should we only do this optimization if |allBordersSameStyle| is true?
    //
    // XXX In fact is this optimization even worth the complexity it adds to
    // the code?  1px wide dashed borders are not overly common, and drawing
    // corners for them is not that expensive.

Comment 43

[Jonathan Watt [:jwatt]]

Comment on attachment 8893730 [details] [diff] [review]
Part 1: Adjust the border corner dimension so that the border becomes symmetric.

Review of attachment 8893730 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/painting/nsCSSRenderingBorders.cpp
@@ +3408,5 @@
> +    //     |  |        |  |    |  |        |  |
> +    //     |  |        |  |    |  |        |  |
> +    //     +--+--------+--+    |  +--------+  |
> +    //     |  |        |  |    |  |        |  |
> +    //     +--+--------+--+    +--+--------+--+

Sorry, that should be allBordersSameWidth, not allBordersSameStyle.

Comment 44

[Jonathan Watt [:jwatt]]

Comment on attachment 8893731 [details] [diff] [review]
Part 2: Start and end with full dashed segment if the box is has 2px width or height.

This code is very specific to part 1, what with the very specific border width and rect size checks. At the very least this would need a decent comment to explain what this code is doing because it will not be clear at all other devs that encounter this code.

However, there's way too much code in this file for special cases, and I would really prefer to avoid adding more code for special cases if it's not necessary. (We've been experiencing how much of a headache that code can create while trying to work on this bug the last few days.)

As far as I can tell the purpose of the code above your change is to make us use a half dash when the dashes at the ends of the line will be connecting onto a corner, and use a full dash when they will not. It seems like that's exactly what we would want to do in the case there is no corner (it's empty), which happens when the adjacent side has zero width or when we apply the tweak in part 1. So if we made that more general change we would, I think, improve the behavior and make it more consistent while also fixing the specific case we care about here.

So I'd propose changing that instead of this patch you change the code to be something like this:

    if (mBorderRadii[GetCCWCorner(aSide)].IsEmpty() &&
        (mBorderCornerDimensions[GetCCWCorner(aSide)].IsEmpty() ||
         mBorderStyles[PREV_SIDE(aSide)] == NS_STYLE_BORDER_STYLE_DOTTED ||
         // XXX why this <=1 check?
         borderWidth <= 1.0f)) {
      fullStart = true;
    }

    if (mBorderRadii[GetCWCorner(aSide)].IsEmpty() &&
        (mBorderCornerDimensions[GetCWCorner(aSide)].IsEmpty() ||
         mBorderStyles[NEXT_SIDE(aSide)] == NS_STYLE_BORDER_STYLE_DOTTED)) {
      fullEnd = true;
    }

That replaces the:

  mBorderWidths[PREV_SIDE(aSide)] == 0.0f

check with the more general check:

  mBorderCornerDimensions[GetCWCorner(aSide)].IsEmpty()

which covers both the adjacent side being zero width, but also the corner being zero size.

Also a comment like the following above those code blocks to explain what this code is doing would be great, since it took me a little while to figure it out:

    // If either end of the side is not connecting onto a corner then we want a
    // full dash at that end.
    //
    // Note that in the case that a corner is empty, either the adjacent side
    // has zero width, or else DrawBorders() set the corner to be empty
    // (it does that if the adjacent side has zero length and the border widths
    // of this and the adjacent sides are thin enough that the corner will be
    // insignificantly small).

I have pushed your patches with these changes to part 2 to Try and, other than part 3 needing tweaked again, it seems to pass:

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

I'm going to mark this attached patch as r-, but if you think of/uncover any problems with my proposal then let me know.

Comment 45

[Jonathan Watt [:jwatt]]

Note also that your latest version of part 1 which removed the special branch to take the old code path has fixed the following tests to start passing on Linux x64 Stylo:

  layout/reftests/flexbox/flexbox-dyn-insertAroundDiv-3.xhtml
  layout/reftests/flexbox/flexbox-dyn-insertAroundSpan-3.xhtml 

So you'll need to remove the:

  fails-if(gtkWidget&&styloVsGecko)

annotation from:

https://hg.mozilla.org/mozilla-central/annotate/52285ea5e54c73d3ed824544cef2ee3f195f05e6/layout/reftests/flexbox/reftest.list#l46

I guess that change can go in part 3.

Comment 46

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Check border corner dimension instead of adjacent corner width when tweaking start/end segment size for dashed border.

Thanks, I agree with this way :)
it fixes both thin and not-thin cases.

Comment 47

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Update border dashed testcase.

updated test masks and also known-fail

Comment 48

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Change the code that optimizes away CSS border corner painting to ensure opposite borders have the same length. r=jwatt

Comment 49

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Change the code that optimizes away CSS border corner painting to ensure opposite sides have the same length. r=jwatt

fixed comments

Comment 50

[Jonathan Watt [:jwatt]]

Comment on attachment 8894084 [details] [diff] [review]
Part 2: Check border corner dimension instead of adjacent corner width when tweaking start/end segment size for dashed border.

Review of attachment 8894084 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message should make sense to someone in the context of all commits landing in the codebase. It should also focus on the overall aim of the change rather than its mechanics. Please make the commit message "Consistently make the ends of dashed CSS borders a full dash if not connecting to a corner".

Comment 51

[Jonathan Watt [:jwatt]]

Thanks, Tooru. I really appreciate your time and patience the last few days as we worked through various issues on IRC (especially given our timezone difference)! I'm much happier now we've managed to remove some of the unnecessary and confusing special cases rather than adding more of them, not to mention avoiding adding fuzz to lots of reftests. I think the code better for it. Hopefully you feel it was worth it too. :)

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a69f05ac539a
Part 1: Change the code that optimizes away CSS border corner painting to ensure opposite sides have the same length. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/11dfd8e3c520
Part 2: Consistently make the ends of dashed CSS borders a full dash if not connecting to a corner. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/647143deab13
Part 3: Update border dashed testcase. r=jwatt

Comment 53

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a69f05ac539a8e39cfdee3d559da4d0cc962f7ed
Bug 1356114 - Part 1: Change the code that optimizes away CSS border corner painting to ensure opposite sides have the same length. r=jwatt

https://hg.mozilla.org/integration/mozilla-inbound/rev/11dfd8e3c5202e7055338f9f5d84c7890cad2a9d
Bug 1356114 - Part 2: Consistently make the ends of dashed CSS borders a full dash if not connecting to a corner. r=jwatt

https://hg.mozilla.org/integration/mozilla-inbound/rev/647143deab1374cf8ae938f7c9a35fd634dbab8a
Bug 1356114 - Part 3: Update border dashed testcase. r=jwatt

Comment 54

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a69f05ac539a
https://hg.mozilla.org/mozilla-central/rev/11dfd8e3c520
https://hg.mozilla.org/mozilla-central/rev/647143deab13