hr with 1px border-width and dashed style is not evenly spaced

ASSIGNED
Assigned to

Status

()

Core
Layout
ASSIGNED
2 months ago
3 days ago

People

(Reporter: karlcow, Assigned: arai)

Tracking

55 Branch
Points:
---
Bug Flags:
webcompat ?

Firefox Tracking Flags

(firefox55 affected)

Details

(Whiteboard: [webcompat])

Attachments

(11 attachments)

(Reporter)

Description

2 months ago
Created attachment 8857769 [details]
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.
(Reporter)

Updated

2 months ago
Flags: webcompat?
Whiteboard: [webcompat]
cc :arai since she fixed bug 382721 so may have some insight on what should happen here.
(Assignee)

Comment 2

2 months ago
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.
(Assignee)

Comment 3

a month ago
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.
Assignee: nobody → arai.unmht
(Assignee)

Comment 4

a month ago
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.
(Assignee)

Comment 5

18 days ago
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
.--->.
(Assignee)

Comment 6

18 days ago
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.
(Assignee)

Comment 7

18 days ago
to be clear, because of that adjustment, the border length of top/bottom borders differ.
(Assignee)

Comment 8

18 days ago
Created attachment 8874242 [details]
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.
(Assignee)

Comment 9

18 days ago
Created attachment 8874264 [details]
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.
(Assignee)

Comment 10

18 days ago
Created attachment 8874266 [details]
patched mBorderCornerDimensions twiddling for 1px-wide border

this is what the patch does
(Assignee)

Comment 11

12 days ago
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
(Assignee)

Updated

12 days ago
Status: NEW → ASSIGNED
(Assignee)

Comment 12

11 days ago
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.
(Assignee)

Comment 13

11 days ago
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...
(Assignee)

Comment 14

11 days ago
Created attachment 8876478 [details]
object-fit-fill-png-001-ref.html result
(Assignee)

Comment 15

11 days ago
Created attachment 8876479 [details]
object-fit-fill-png-001c.html result
(Assignee)

Comment 16

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

Comment 17

11 days ago
Created attachment 8876504 [details]
the color of the anti-aliased part of the dashed line

here's the difference
(Assignee)

Comment 18

11 days ago
looks like the fact that the border is rendered twice is not related.
the failure happens even if I skip the first rendering.
(Assignee)

Comment 19

11 days ago
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.
(Assignee)

Comment 20

11 days ago
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
(Assignee)

Comment 21

11 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c15a8e03c1516ef6b6c29a25e21ebfe8846470dd
(Assignee)

Comment 22

10 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c15f651802e5c65e5dd1b707a3308fd131073bd0&group_state=expanded
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60e964a95d5cd7cfa63f6ad57fa4949827cbfab4&group_state=expanded
(Assignee)

Comment 23

8 days ago
Created attachment 8877800 [details] [diff] [review]
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.
Attachment #8877800 - Flags: review?(jwatt)
(Assignee)

Comment 24

8 days ago
Created attachment 8877801 [details] [diff] [review]
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.
Attachment #8877801 - Flags: review?(jwatt)
(Assignee)

Comment 25

8 days ago
Created attachment 8877802 [details] [diff] [review]
Part 3: Update border dashed testcase.

updated the testcase mask
Attachment #8877802 - Flags: review?(jwatt)
(Assignee)

Comment 26

8 days ago
Created attachment 8877804 [details] [diff] [review]
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...
Attachment #8877804 - Flags: review?(jwatt)
You need to log in before you can comment on or make changes to this bug.