19963 - [BORDERS] dotted and dashed borders don't interact well with other types

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

DESCRIPTION:  Dotted and dashed borders don't interact too well with other
types.  They probably need to be beveled, and the widths could be adjusted so
the ends always come out nicely.

I may have time to write this code myself.  Assign it to me if you want.

STEPS TO REPRODUCE:
 * load attached test case

ACTUAL RESULTS:
 * the dotted/dashed borders show in the gap behind double (and presumably
    transparent) borders
 * at some widths, the top border of the first paragraph has end dots that
   aren't as wide as the border they connect with.  The end dots should always
   have minimum length of max(width of border, width of adjacent border, width
   of other adjacent border)

EXPECTED RESULTS:
 * not the above

DOES NOT WORK CORRECTLY ON:
 * Linux, mozilla, 1999-11-22-08-M12

WORKS CORRECTLY ON:
 * Opera 3.6, mostly

ADDITIONAL INFORMATION:
See also bug 19601.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: test case described in bug.

Comment 2

[Patrick C. Beard]

Again, isn't this part of CSS rendering?

Comment 3

[troy]

I'm sure this is a DUP of an existing bug

Comment 4

[dcone (gone)]

This is not a duplicate.. the border styles are not inter-acting correctly at
the corners.. probably need some clipping, or something to keep the styles from
polluting each other...

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that the logic used (in nsCSSRendering::MakeSide) to decide whether to
bevel is quite complicated, and it might be easier to incorporate the drawing of
dashed sides into that function (i.e., call a dashed-side drawing function from
there rather than from PaintBorder).

Comment 6

[dcone (gone)]

This bug has been marked future because we have determined that it is not 
critical for netscape 6.0.  If you feel this is an error, or if it blocks your 
work in some way -- please attach your concern to the bug for reconsideration.

Comment 7

[Hixie (not reading bugmail)]

This does not destroy a CSS feature, and while it would be nice for it to be 
fixed, I think we can indeed wait for the 5.1 release. It looks ugly, but that
is about it.

We should probably mention this in the release notes' "bugs we are aware of but
which we did not have time to fix before losing all market share" section... ;-)

Marking polish, css1, testcase, relnote.

Comment 8

[Henri Sivonen (:hsivonen)]

This bug is easy to understand if an author happens to find it. Relnoting this
is, in my opinion, in the "Yes, we know. Don't report duplicate bugs." category
rather than in the "Things authors really need to know" category.

Comment 9

[Thomas]

I check the example in Mozilla 1.8 a6 and IE6 and tehre is always some overlap.
Funny though... in Mozilla the dots are square and in IE the dots are round :)

Comment 10

[Kae Verens]

Attachment: proposed replacement for the second DrawDashedSides function in nsCSSRendering.cpp

I think this will satisfactorily fix this bug. It works well on the test-case.

The attachment is a replacement for the second DrawDashedSides function in
nsCSSRendering.cpp. I basically used the original logic, but expanded it to use
bevelled corners where necessary.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's really rather hard to deal with replacements instead of diffs.  Any chance you could tell us what version of Mozilla's source you started from?  Then we could turn the code you attached into a diff and review it.  (Not sure why I didn't ask that explicitly earlier.)

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: previous attachment as a diff against current trunk

This is what the previous attachment looks like as a diff against the current trunk.  I'm guessing this code hasn't changed much.

In any case, I'm a little skeptical of this patch:  it adds a *lot* of code that's pretty much completely undocumented, and adds it to code that's generally already more complicated than it needs to be.

It seems like the ideal way to do this would be to reuse the existing function that draws a border side to draw the two ends (always past the end of the bevel), and then draw the dashes in the middle based on splitting up the remaning space evenly so that each dash is as close as possible to a standard length.

Comment 13

[Kae Verens]

I think the original code looks so complicated as it is very optimised, based on the original (slightly faulty) algorithm.

The code I submitted was the first C I had written in a few years, so was probably more complex than needed - nevertheless, my tests with that code gave better results than the vanilla code.

I was working on curved dashes as well, later on, and came up with basically the same method as you describe (draw the ends first, then the dashes). Unfortunately, I have not translated that code into C yet. See here for an example: http://verens.com/demos/borders/dashed-curve.html

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: the approach I'd like to take

This stubs out the approach I'd like to see -- integrating dashed/dotted further within the border-drawing code, for a start, and then making DrawDashedSizes (and perhaps a separate DrawDottedSides) to only occupy half the corner.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(see also the last paragraph of comment 12)

Comment 16

[Pam Greene]

Attachment: Proposed end result of both bugs

Considering this issue along with bug 1515 (using round dots in dotted borders), I note that ellipses at corners look odd to me, and drawing only half of a dot is downright strange.  I propose that we aim for the following behavior:

Double, single, and dashed borders are all considered "lines".
* At a line-dot corner, draw the line out to the end and inset the first dot.
* At a line-line corner, draw and bevel both lines.
* At a dot-dot corner, draw the larger dot and inset the smaller one.  If both dots are the same size, draw the dot from the shorter side and inset the other one.
* At a line-empty or dot-empty corner, do the obvious thing.
* Now fill in dotted borders nicely spaced between the corners.

For examples of these rules in action, see the attached mockup.  For comparison, IE appears to do what we do now, missing dashed-line bevels and all, except that instead of square dots colliding at the corners they have circles nicely spaced there.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For what it's worth, Vlad rewrote the border code (except for the painting of borders on border-collapsed tables, I think) since this bug was last touched; it's at least partly still present, though, and there are some other remaining regressions from that rewrite, such as the fact that we no longer space the dots and dashes so that they're symmetric and line up at each corner.  (But we do use round dots now.)  There are a bunch of other related bugs floating around; Vlad may know them better.

Comment 18

[Zack Weinberg (:zwol)]

Attachment: demo of all interesting combinations

Comment 19

[Zack Weinberg (:zwol)]

Attachment: current rendering of attachment #323434

Comment 20

[Zack Weinberg (:zwol)]

Attachment: proposed corrected rendering of attachment #323434

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think dashed borders should probably be:
 * symmetric (per-side)
 * always have a dash extending a little bit out from the corner

Comment 22

[Zack Weinberg (:zwol)]

yeah, there are clearly still problems.  I worked up a five-by-five cross of the interesting possibilities from the spec (the various "3D" borders are not interesting for this bug, 'cos they should be, and are, treated just like "solid" at corners).  I also attached the current (latest moz-central) rendering of the cross, and a proposal for how it ought to be rendered, incorporating Ms. Greene's suggestions plus a few of my own ideas.  Note that right now we do a crummy job on "border: dotted" all by itself, never mind how it interacts with other patterns! The things that IMO ought to be changed go like this.  "Solid-type" borders are solid, double, inset, outset, groove, and ridge.

* If a dotted border adjoins a solid-type border, the solid-type wins: it fills up the corner.

* If a dotted border adjoins another dotted border or no border, the dot spacing is adjusted to ensure that there is a complete dot at each corner involved.

* If a dotted border adjoins another dotted border with a different dot radius, the larger dot gets the corner.  (Not shown in my example, but is in Pam Greene's.)

* If a dotted border adjoins another dotted border with the same radius but a different color, I'm not sure what the right behavior is.  I drew this case beveled in my corrected example, but that might be very hard to implement, and perhaps there's something else that would look better.

* If a dashed border adjoins a solid-type border, a bevel is drawn if and only if a dash extends out of the corner.  Otherwise, the solid-type wins as for dotted borders.  (This rule is much easier to understand by example: look at columns 2 and 3 of row 4 of both the current and proposed corrected renderings.)

* If a dashed border adjoins a dotted border, the same principle applies; see column 5 row 4 and row 4 column 5.

* Column 3 row 1 shows yet another special case, for dash against "none" where the nonexistent border has been stretched into existence by table alignment: the dash is drawn out to the edge of the border zone.  (Yeah, 's confusing.)

Comment 23

[Zack Weinberg (:zwol)]

whups, crossed wires.  I kinda like the way row 4 column 4 looks in the current rendering, and all else follows from that, but I can definitely see the case for making it look like four picture corners instead.  Is that what you were suggesting?

If we did that, dashed borders would always get beveled against solid-types and win against dotted, which would probably be much less complicated to code.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #23)
> whups, crossed wires.  I kinda like the way row 4 column 4 looks in the current
> rendering, and all else follows from that, but I can definitely see the case
> for making it look like four picture corners instead.  Is that what you were
> suggesting?

Yes.  I think that's also what we did in 1.8.

Comment 25

[Zack Weinberg (:zwol)]

ff2 does do picture corners, although not well IMO.

Comment 26

[Zack Weinberg (:zwol)]

this is polish, but I'm raising the priority from P3 to P2 and nominating for 1.9.1 because dotted border corners currently look *terrible*.

Comment 27

[Zack Weinberg (:zwol)]

Attachment: new proposed corrected rendering, with "picture corner" dashes

new mockup - having drawn it out, I am now definitely for "picture-corner" dashes.

Comment 28

[Damon Sicore (:damons)]

wanted1.9.1+

Comment 29

[Vital]

I am conjecturing that the most frequent practical use case where users are unhappy about the current rendering is the dot-dot corner of the equal dot sizes.
I would recommend to attack and solve it first and in the process to collect more feedback for the rest of the cases.

The solution is (seemingly) simple, it has been proposed several times (see #16) and is implemented in IE6-8 and Chrome:
place dots in the corners first and then space the dots evenly in between them.

Some "cheating" will be needed, because in many cases the dot+spacing will not divide evenly into (container dimensions plus/minus some fixed offset).

Please, please, please do not cheat at the corners.  The corners are way too obvious. One "bigger" cheat should be divided into several 1px cheats and placed a little off the center somewhere. 

Picture-corners could be used but then for the thicker borders a larger number of corner pictures will be needed in order to cover all varying dot spacings (about 16 ((4x4)) pics ((3 dots each)) for a 3px border).

Pictures could be even used for the cheats

Comment 30

[Zack Weinberg (:zwol)]

Regrettably, I am not going to be able to land this for 1.9.1.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That doesn't mean it shouldn't have the wanted1.9.1+ flag; we might also look at bugs with that flag for the next cycle.

Comment 32

[arno renevier]

Hi,
I'm currently trying to provide a patch for this bug, and I've problem with the proposed same radius dotted-dotted corner (two halves circles):

- the code is already complex, and it makes it yet more complex.
- there is a blank line left between the two halves. For some reason, drawing two halve circles with cairo leaves a blank line [1]
- and more important: with a two pixel width corner, colors are not really separated, and corner looks like a gray point.

What do you think of something like: horizontal border "wins"; or border wins at end of border in clockwise direction (on top-right corner, top border wins; on bottom-right corner, right border wins) ?


[1]:  cairo_arc(cr, 100, 100, 5, kPi / 4.0, 5.0 * kPi / 4.0);
            [...]
        cairo_arc(cr, 100, 100, 5, 5.0 * kPi / 4.0, kPi / 4.0);

Comment 33

[arno renevier]

Attachment: dotted dotted corner for different widths

Comment 34

[Zack Weinberg (:zwol)]

I don't see a problem with the half circles in the example upload, except for the two-pixel width corner?  But, in general, you have to draw the corners separately from the sides, with a special algorithm to get the seams perfect:

 - create a scratch surface (->PushGroup())
 - set the drawing mode to OPERATOR_ADD
 - set the clipping path to a triangle covering half of the corner rectangle
 - draw the *entire* contribution of that side to the corner
 - set the clipping path to the other triangle
 - draw the *entire* contribution of that side
 - pop the scratch surface and splat it into the corner using OPERATOR_OVER

In particular, for dotted borders you want to be drawing a full circle for each side's contribution to the corner.  There's code that does something like this already in nsCSSBorderRenderer::DrawBorders() but it really wants to be broken up a bit and taught to draw only the corner, not the entire side (this is just wasteful, as the clipping region kills everything but the corner anyway).

As for the two pixel corners, yes, we should switch to something other than the half circles at that point.  I don't think it matters exactly what algorithm is used, but horizontal-wins might wind up looking tidier.

You pasted another sample on IRC which looked pretty good except that dashed-dashed wasn't using diagonal seams, and that looked really strange.

Comment 35

[arno renevier]

Attachment: patch v1

Hi,

Here is a patch with a DrawDottedSide function. It computes spacing needed to make first/last dot match start/end of border. I wasn't aware that clipping was needed, so I made it draw full side: it knows where to begin and where to stop.

That makes it a bit complex but it seems to have a mostly correct rendering[1]. When investigating the clipping option, I discovered it could create problems with dotted borders of different widths. If you still think using clipping is best, I'll update my patch.

Comment 36

[Zack Weinberg (:zwol)]

(In reply to comment #35)
> Here is a patch with a DrawDottedSide function. It computes spacing needed to
> make first/last dot match start/end of border. I wasn't aware that clipping was
> needed, so I made it draw full side: it knows where to begin and where to stop.
> That makes it a bit complex but it seems to have a mostly correct rendering[1].

... missing footnote?

> When investigating the clipping option, I discovered it could create problems
> with dotted borders of different widths.

Per comment #16, when the borders are different widths, we shouldn't try to split the corner dot at all?

> If you still think using clipping is best, I'll update my patch.

Since the split dots are being such a headache and *anything* would be better than what we do now, would you mind posting a patch that doesn't ever split them?  Then we can compare.

Some notes on the patch itself ...

+const float kPi      = 3.1415926535897932384626433832795f;

Why can't you use M_PI from <math.h>?

I approve of separating out dotted sides to their own function.  Have you made any substantial changes to dashed-line rendering?

This error message is now incorrect, and should maybe just become an NS_ASSERTION at the top of the function:

+  if (style != NS_STYLE_BORDER_STYLE_DASHED) {
     SF("DrawDashedSide: style: %d!!\n", style);
     NS_ERROR("DrawDashedSide called with style other than DASHED or DOTTED; someone's not playing nice");
     return;
   }

Why are you drawing each dot as a separate circle rather than using a dotted line with careful setting of the spacing parameters?  That should be much more efficient.

Comment 37

[arno renevier]

Attachment: patch v1.1

patch updated according M_PI and NS_ASSERTION comments.

(In reply to comment #36)
> (In reply to comment #35)
> 
> ... missing footnote?

Yes, I wanted to talk about a problem with 2px wide dotted border: 
setting 
dash[0] = 0.0; dash[1] = spacing; 
does not work, it makes dot blur. So, we need to use 
dash[0] = dotWidth; dash[1] = dotWidth;
That's how things are done currently. So, spacing is not correctly set, and
dotted corner are not correct. But that's not worse than current behaviour.

> 
> Since the split dots are being such a headache and *anything* would be better
> than what we do now, would you mind posting a patch that doesn't ever split
> them?  Then we can compare.

I'll do that.

> I approve of separating out dotted sides to their own function.  Have you made
> any substantial changes to dashed-line rendering?

Yes, spacing is computed to have a dash at start of side and a dash at end of
side.

> 
> Why are you drawing each dot as a separate circle rather than using a dotted
> line with careful setting of the spacing parameters?  That should be much more
> efficient.


I *am* using a dotted line. I'm only drawing a circle if there is one dot on
the border. I'm also drawing half-circles at corners.

Comment 38

[arno renevier]

I tried to provide some reftests but did not succeed. See two following attachments: they look the same with my patch, but reftests fails, I don't know why.

Comment 39

[arno renevier]

Attachment: xhtml+svg file I tried to use as a reftest reference.

Comment 40

[arno renevier]

Attachment: xhtml file renders similar to xhtml+svg file, but reftest fails.

Comment 41

[arno renevier]

Attachment: patch v1.2

In this patch, dots are not splitted:
if there is a dotted-dotter corner (same size), dot is drawn at beginning of border but not at end.
That means:
on top-left corner, color is color of border-top
or top-right corner, color is color of border-right
on bottom-right corner, color is color of border-bottom
on bottom-left corner, color is color of border-left

Comment 42

[Zack Weinberg (:zwol)]

(In reply to comment #38)
> I tried to provide some reftests but did not succeed. See two following
> attachments: they look the same with my patch, but reftests fails, I don't know
> why.

I ran this with your 1.2 patch and, using a graphics program, found that the edges of the circles are ever-so-slightly different between test and reference; the colors are for instance #ffc3c3 versus #ffa0a0 for the same pixel.  This wouldn't be visible to the eye.  Looks like another thing where SVG doesn't antialias circles exactly the same way HTML does.  Or perhaps where a circle drawn with cairo_arc() isn't antialiased exactly the same as a circle drawn as part of a dashed-with-round-endcaps line.

Will look at your 1.1 and 1.2 patches in more detail later today, but I have to go now.

Comment 43

[arno renevier]

Comment on attachment 365244 [details] [diff] [review]
patch v1.1

I'm asking review so the bug stays in your radar :)

Comment 44

[Zack Weinberg (:zwol)]

In view of how much simpler the code is I think I prefer the no-split-circles version.  I would make one more rendering suggestion, though: when two dotted borders meeting at a corner are the same width, always have the horizontal border draw the corner dot, instead of what you're doing now.  If you look at attachment 323434 [details] with your patch 1.2 applied, the bottom right-hand sample looks, um, strange, and I think it would be less strange with that rule.

Also, what is the behavior when two dotted borders of unequal widths meet?  I could not figure it out from the code.

Also, in comment 21 through comment 27, dbaron and I agreed that dashed sides ought to look like "picture corners" at the endpoints - you're not doing that, but maybe we should spin that off to another bug?

More kibitzing on the patch itself follows.  I'm pretty happy with this, but you need review from an actual layout peer to land, keep in mind.

   switch (aStyle) {
-    case NS_STYLE_BORDER_STYLE_DOTTED:
+    case NS_STYLE_BORDER_STYLE_SOLID:
     case NS_STYLE_BORDER_STYLE_DASHED:
-    case NS_STYLE_BORDER_STYLE_SOLID:

Please leave the NS_STYLE_BORDER_STYLE_SOLID line where it was.

   // clip to the rectangle formed by the given side; a specific
   // overlap algorithm is used; see the function for details.
   // this is currently used for dashing.
-  SIDE_CLIP_RECTANGLE
+  SIDE_CLIP_RECTANGLE,
+
+  // clip to the rectangle formed by the given side and extend maximally; do
+  // not overlap; see the function for details. This is currently used for
+  // dotting.
+  SIDE_CLIP_RECTANGLE_LONG

1) Word wrap your comments to match surrounding text, please.

2) Your new comment is really unclear and so is the old one for
   SIDE_CLIP_RECTANGLE.  And I have no idea which function "the
   function" is.  I'm tempted to ask that you rename these enumerators
   SIDE_CLIP_FOR_DASHES and SIDE_CLIP_FOR_DOTS, and change the
   comments to just

  // clip appropriate for a <dashed/dotted> side; see <name of function>

that would actually be more helpful.

+  // if we touch a dotted border, make inner lines as width as outter line

"as width as" is ungrammatical; did you mean "as wide as"? "outter" should
have only one T.

 
 void
+nsCSSBorderRenderer::DrawDottedSide(PRUint8 aSide)
+{
+ // ...
+  PRUint8 style = mBorderStyles[aSide];
+ // ...

+  if (borderWidth == 0.0)
+    return;
+
+  if (style == NS_STYLE_BORDER_STYLE_NONE ||
+      style == NS_STYLE_BORDER_STYLE_HIDDEN)
+    return;

Why is this function even called if either of these are true?

+  if (style != NS_STYLE_BORDER_STYLE_DOTTED) {
+    SF("DrawDashedSide: style: %d!!\n", style);
+    NS_ERROR("DrawDottedSide called with style other than DOTTED; someone's not playing nice");
+    return;
+  }

This should be a NS_PRECONDITION at the very top, assuming you can get
rid of the calls when borderWidth == 0 or style is NONE or HIDDEN.

+  if (aSide == NS_SIDE_TOP) {
+    borderLength = end.x - start.x;
+  } else if (aSide == NS_SIDE_RIGHT) {
+    borderLength = end.y - start.y;
+  } else if (aSide == NS_SIDE_BOTTOM) {
+    borderLength = start.x - end.x;
+  } else if (aSide == NS_SIDE_LEFT) {
+    borderLength = start.y - end.y;
+  }

I think this would be much more comprehensible like so:

  if (aSide == NS_SIDE_TOP || aSide == NS_SIDE_BOTTOM) {
    borderLength = end.x - start.x;
  } else {
    borderLength = end.y - start.y;
  }
  if (borderLength < 0) {
    borderLength = -borderLength;
  }
    
+    if (aSide == NS_SIDE_TOP) {
+      start.y += borderWidth / 2.0;
+      end.y += borderWidth / 2.0;
+      start.x += borderLength / 2.0;
+    } else if (aSide == NS_SIDE_RIGHT) {
+      start.x -= borderWidth / 2.0;
+      end.x -= borderWidth / 2.0;
+      start.y += borderLength / 2.0;
+    } else if (aSide == NS_SIDE_BOTTOM) {
+      start.y -= borderWidth / 2.0;
+      end.y -= borderWidth / 2.0;
+      start.y -= borderLength / 2.0;
+    } else if (aSide == NS_SIDE_LEFT) {
+      start.x += borderWidth / 2.0;
+      end.x += borderWidth / 2.0;
+      start.x -= borderLength / 2.0;
+    }

I know what you're trying to do here but the code makes my head hurt.
Please try to find a clearer way to express it, like what I did above.
It would help a lot if you only assigned each variable once.

+
+    if (aSide == NS_SIDE_TOP) {
+      start.y += borderWidth / 2.0;
+      end.y += borderWidth / 2.0;
+
+      start.x += (borderWidth / 2.0);
+      if (!prevSideExtends) {
+        start.x += spacing * NS_ceil(mBorderCornerDimensions[C_TL].width / spacing);
+      }
+      if (!nextSideExtends) {
+        end.x -= spacing * NS_ceil(mBorderCornerDimensions[C_TR].width / spacing);
+      }
+    } else if (aSide == NS_SIDE_RIGHT) {
...

Similarly for this code.


+  if (aSide == NS_SIDE_TOP) {
+    printf("numDots: %d spacing: %f -> %f %f\n", numDots, spacing, start.x, start.y);
+  }

Make this an SF().

Comment 45

[arno renevier]

(In reply to comment #44)
> In view of how much simpler the code is I think I prefer the no-split-circles
> version. 

OK, I have used this version as a basis for further work

> I would make one more rendering suggestion, though: when two dotted
> borders meeting at a corner are the same width, always have the horizontal
> border draw the corner dot, instead of what you're doing now.  If you look at
> attachment 323434 [details] with your patch 1.2 applied, the bottom right-hand sample
> looks, um, strange, and I think it would be less strange with that rule.

I've fixed that now. In comment 32, you suggested that "horizontal-wins might wind up looking tidier". So, I have choosen another algorithm: a border wins at its end, but not at it's start. It means, for dotted border with the same width:
- we draw dot of top border in top-right corner
- we draw dot of left border in bottom-right corner
- we draw dot of bottom border in bottom-left corner
- we draw dot of left border in top-left corner.


> Also, what is the behavior when two dotted borders of unequal widths meet?  I
> could not figure it out from the code.

That is handled with prevSideExtends and nextSideExtends. I've added a comment to better explain.

> Also, in comment 21 through comment 27, dbaron and I agreed that dashed sides
> ought to look like "picture corners" at the endpoints - you're not doing that,
> but maybe we should spin that off to another bug?

Sorry I missed that. That's fixed in my new patch.

> 
> More kibitzing on the patch itself follows.  I'm pretty happy with this, but
> you need review from an actual layout peer to land, keep in mind.

thanks for you comment, I've fixed those things.

Comment 46

[arno renevier]

Attachment: patch v2

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 373805 [details] [diff] [review]
patch v2

Vlad should also review this.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, have you thought about how to test this?  It seems like you may not want to test exact rendering, but there are probably important concepts that you do want to test, for example:
 * dashed and dotted borders are symmetric about the center of the border
 * dashed borders are the same as solid borders within some space near the corner
and probably a bunch more.  Many of these could be tested with reftests (and -moz-transform and use of other elements to cover pieces of things), or perhaps with mochitests, drawWindow (perhaps using WindowSnapshot.js), and flipping drawn images around.

Comment 49

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 373805 [details] [diff] [review]
patch v2


>+
>+  // if we touch a dotted border, make inner lines as width as outter line

Not sure I understand the purpose of this block (also comment needs some fixes.. "make inner lines as wide as outer line" maybe?).  This is so that when we inset the border width, we leave the sides that are dotted untouched, so that the adjacent non-dotted sides always extend into the dotted side's corner?  If so, this needs a longer comment explaining this :)

>+  NS_FOR_CSS_SIDES (i) {
>+    PRBool prevIsDotted   = (mBorderStyles[PREV_SIDE(i)] == NS_STYLE_BORDER_STYLE_DOTTED);
>+    PRBool nextIsDotted   = (mBorderStyles[NEXT_SIDE(i)] == NS_STYLE_BORDER_STYLE_DOTTED);
>+    if (!IsZeroSize(mBorderRadii[i])) {
>+      continue;
>+    }
>+    for (unsigned int count = 0; count < borderColorStyleCount; count++) {
>+      if (nextIsDotted) {
>+        borderWidths[count][NEXT_SIDE(i)] = 0.0;
>+      }
>+      if (prevIsDotted) {
>+        borderWidths[count][PREV_SIDE(i)] = 0.0;
>       }
>     }
>   }




> void
> nsCSSBorderRenderer::DrawDashedSide(PRUint8 aSide)
> {

>+  dashWidth = gfxFloat(borderWidth * DOT_LENGTH * DASH_LENGTH);
>+  PRUint32 numDash = NS_ceil (((borderLength / dashWidth) + 1.0) / 2.0);
>+  spacing = borderLength / (2 * numDash - 1);
>+
>+  mContext->SetLineCap(gfxContext::LINE_CAP_BUTT);
>+  dash[0] = spacing;
>+  dash[1] = spacing;
>+  mContext->SetDash(dash, 2, 0.0);

This is kinda bad; computing spacing in this way virtually guarantees that it's not going to be an integer, which means that we're going to lose tons of performance when drawing dashed borders since each individual dash isn't going to be pixel aligned (when not zoomed, at least).  There's a testcase for this in a bug somewhere, but it's easy enough to create -- just make a page with a hundred or so large dashed divs.

Ideally we'd distribute extra pixels of space at some point in the border, ideally symmetrically in some way.


> void
>+nsCSSBorderRenderer::DrawDottedSide(PRUint8 aSide)
>+{

DrawDottedSide looks fine; snice we're drawing dots already, the integer-alignment issues don't come into play.

>   PRIntn dashedSides = 0;
>+  PRIntn dottedSides = 0;

Is there a reason to keep track of these separately?  Given that the code is identical except for which function to call to draw them, seems like you should just rename the variable dashedOrDottedSides and then just check the style to determine which draw function to call.  Doing so avoids the need to have multiple chunks of largely identical code.

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Any chance of an updated patch here?

Comment 51

[Zack Weinberg (:zwol)]

Attachment: patch v3 (addressing vlad's comments)

Here's a patch updated to latest trunk and addressing vlad's comments.

(In reply to comment #49)
> >+  // if we touch a dotted border, make inner lines as width as outter line
>
> Not sure I understand the purpose of this block (also comment needs
> some fixes.. "make inner lines as wide as outer line" maybe?).  This
> is so that when we inset the border width, we leave the sides that
> are dotted untouched, so that the adjacent non-dotted sides always
> extend into the dotted side's corner?  If so, this needs a longer
> comment explaining this :)

Yes, that's what it's doing.  I had to stare at it for a long time,
write a testcase, and file two new bugs, to be sure I got it :)

I've expanded the comment a bunch and fixed a similar problem in the
composite-colors code.

> >+  dashWidth = gfxFloat(borderWidth * DOT_LENGTH * DASH_LENGTH);
> >+  PRUint32 numDash = NS_ceil (((borderLength / dashWidth) + 1.0) / 2.0);
> >+  spacing = borderLength / (2 * numDash - 1);
>
> This is kinda bad; computing spacing in this way virtually
> guarantees that it's not going to be an integer, which means that
> we're going to lose tons of performance when drawing dashed borders
> since each individual dash isn't going to be pixel aligned (when not
> zoomed, at least).  There's a testcase for this in a bug somewhere,
> but it's easy enough to create -- just make a page with a hundred or
> so large dashed divs.  Ideally we'd distribute extra pixels of space
> at some point in the border, ideally symmetrically in some way.

What I've done in this revision is compute dashWidth the same as is
done here but then take its floor(), and then just set the dash
pattern to [dashWidth, dashWidth].  I then draw the dashed border from
both ends to the middle, rather than from one end to the other.  This
should get us pixel-aligned dashes.  If there's a dash that's a
different length than all the others, it's in the middle, and the
border has reflection symmetry, so it looks like we meant to do that.
And if you have a box that stretches with the window and you resize
the window smoothly, the dashes do not jump around.

It is a little inconsistent with what dots do, though.  Let me know if
you don't like it.

> >   PRIntn dashedSides = 0;
> >+  PRIntn dottedSides = 0;
>
> Is there a reason to keep track of these separately?  Given that the
> code is identical except for which function to call to draw them,
> seems like you should just rename the variable dashedOrDottedSides
> and then just check the style to determine which draw function to
> call.  Doing so avoids the need to have multiple chunks of largely
> identical code.

Done.

Comment 52

[Zack Weinberg (:zwol)]

Attachment: patch v3a: better dash behavior

I found some corner case problems with v3.  This revision addresses them for dashes, only.  Please play with dashed borders and see if you can make it do dumb things like having one gap that's much bigger than all the others.

The biggest remaining problem is that a border with small dots (one to four pixels wide, I think) is not always drawn with a dot exactly at either end of the line.  This is more important than it might sound, because a focus ring is drawn as a one-pixel dotted border, so that means the patch doesn't yet make them look right.

Comment 53

[Gábor Stefanik]

Could you please produce a tryserver build for this one?

Comment 54

[Zack Weinberg (:zwol)]

Attachment: patch v4

I finally got around to revising this again to improve the dotted line behavior.  It may not be perfect yet (I saw some odd glitches in testing) but I think this is now definitely an improvement over what we have.

Comment 55

[Zack Weinberg (:zwol)]

Tryserver builds are at http://build.mozilla.org/tryserver-builds/[e-mail]-try-f8077fed17ff

but only for Linux and Windows -- MacOS builds don't seem to have happened.

Comment 56

[Jeff Muizelaar [:jrmuizel]]

What's the testing situation going to be for this? It would be nice to have reftests for all the behavior but I don't know how easy that would be to do.

Comment 57

[Zack Weinberg (:zwol)]

Currently there are no tests for this.  I think SVG references would be possible for a lot of the behavior, but they would be tedious to write, and I expect they would be fragile (spurious failures due to aliasing, e.g.)

Comment 58

[fantasai]

How about shrinking the SVG reference down by 1 or 2 px, making it red, and placing it under the border? Then you can compare that to a copy of the test without the underlaid reference: the expectation is to not have any red.

Comment 59

[Zack Weinberg (:zwol)]

That might do for at least some of the testing, but how would you detect regression to the current (bad) rendering of

data:text/html;charset=utf-8,%3C%21doctype%20html%3E%3Chtml%3E%3Chead%3E%3Cstyle%3E%0D%0A%20%20div%20%7B%20width%3A%20200px%3B%20height%3A%20200px%3B%20margin%3A%2020px%3B%0D%0A%20%20%20%20%20%20%20%20%20%20border%3A%2010px%20dotted%20black%3B%20%7D%0D%0A%3C%2Fstyle%3E%3C%2Fhead%3E%0D%0A%3Cbody%3E%3Cdiv%3E%3C%2Fdiv%3E%3C%2Fbody%3E%3C%2Fhtml%3E

?

Comment 60

[Zack Weinberg (:zwol)]

I was hoping Bugzilla would linkify that.  Here's the readable version:

<!doctype html><html><head><style>
  div { width: 200px; height: 200px; margin: 20px;
          border: 10px dotted black; }
</style></head>
<body><div></div></body></html>

Comment 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You can also do the inverse of what fantasai suggested:  draw the SVG on top, slightly larger.  So that case should be easy to test:
  test:
     (1) your example, in red
     (2) green SVG circles on top of it
     (3) two or four white boxes to cover up the sides and just show the corners
  reference:
     (2) and (3) above

Comment 62

[Zack Weinberg (:zwol)]

I no longer work for Mozilla, I am deassigning myself from bugs I have no intention of working on as a volunteer.

Comment 63

[Jeff Muizelaar [:jrmuizel]]

Attachment: Mask for on top of the border rendering

Comment 64

[Jeff Muizelaar [:jrmuizel]]

Attachment: Mask for under the border painting

This two images should make it pretty easy to make two reftests:

test #1:
 test: paint the border test case under over.png
 ref: just over.png

test #2:
 test: paint the border test case over under.png
 ref: paint the border test case

This should take of the the antialiasing differences

Comment 65

[fantasai]

Zack, do you think you can wrap this up and get it checked in sometime soon? We've got a GSoC candidate who will be working on border painting, so it'd be good to not have this patch floating indeterminately in Bugzilla. :)

Comment 66

[Zack Weinberg (:zwol)]

Attachment: patch v5

Here is an updated patch which applies cleanly to current trunk and has some test cases - maybe not enough test cases, and none of them in fact *pass* at the moment due to a combination of actual errors in the dot/dash placement code, rounding errors, and bugs in the test cases (which are fiendishly complicated).  However, I've spent as much time as I can justify spending on this patch this week.  I might cycle back to it but I'm not promising to do so anytime soon.

fantasai: It might now be a reasonable starting point for your GSoC student to try to finish this, I think (it wasn't as was).

Comment 67

[Robin Whittleton]

This should be fixed by Tooru Fujisawa’s work in bug #382721 when it lands.

Comment 68

[Tooru Fujisawa [:arai]]

Fixed as a part of bug 382721.