652650 - Render dashed borders along border-radius curve

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[fantasai]

Splitting out from bug 382721. This bug report is about rendering dashes along the border-radius curve.

Comment 1

[Anil Shanbhag]

Attachment: Proposed way to render dashed corners

Comment 2

[Anil Shanbhag]

Proposed way to render dashed corners:

	Dash length is taken 2*(border width).In the event of having different border width resulting in different dash lengths , the largest of them is taken as dash length[common dash length]. Gap length is calculated but is limited to [0.5*dashLength,dashLength) .
	
	To calculate the gap length . First 'side' here on refers to part of side minus the corner paths plus the part of corners [demarcated by angle a , tan(a)= ratio of border widths] adjoining to the side. Length of side is length of path of center along the side. Each side ends on either side in half a dash.Gap length is calculated such that we have exactly we have exactly n dashes , n+1 gaps and half dash on each end. Having the constrain n is integer , gapWidth in [0.5*dashLength,dashLength) and that lower value of gap width given precedence[in case two cases possible] we get symmetrically placed set of dashes along the curve.    

	Sample Calculation : 
Case : 
border-width:50px; width:800px; height: 800px; border-radius:100px; 
side without corner paths = 800 - 2*50 = 700px
angle of demarcation tan(a) = 50/50 = 1 => a = pi/4
curved path to be added  = 2*[75 * pi/4] = 118px
total length = 818px
dash length= 2*50 = 100px
818 = 2*50 + (n+1)x + n*100 [x -> gap width]
n = 4 ,gap width = 64px

Comment 3

[Anil Shanbhag]

After working on the above logic , created a prototype implementation using pycairo . 

The main class file at : 
https://github.com/anilshanbhag/border/blob/master/rectangle.py

Testcases evaluated against at:
https://github.com/anilshanbhag/border/blob/master/TESTCASES
And the resulting output :
https://github.com/anilshanbhag/border/tree/master/sample%20output

If there are any suggestions/corrections , do let me know :)

Comment 4

[Zack Weinberg (:zwol)]

I generally like the way the dashes are being drawn around the curve there.  Two exceptions: in t3 it looks a little weird because you've got a curved corner going into a zero-width border, with only part of it visible due to the dashing.  I think you need to adjust the dash spacing in that case so that more of the corner is visible (not sure how much would be appropriate).  And in t9, this isn't strictly a problem with the dashes going around the *curve*, but the antialiasing is making ugly smears.  You need to pixel-snap the dash boundaries for the straight part of the line when the dash gap is that short.  Actually, I think it'd be best to fix this problem by making the dash gap bigger -- from a distance it's hard to tell it's a dashed line at all.

I can't tell whether t21 is the desired rendering or not because I'm not sure what it's supposed to be drawing.

There is also a problem with where you're drawing the *color* transition.  Read http://dev.w3.org/csswg/css3-background/#corner-transitions carefully, but those rules are not sufficient for a good appearance, and it's quite possible that the paragraph beginning "The center of color and style transitions between adjoining borders is ..." is just plain wrong.  (If you implemented that rule, then it IS wrong.)  Specifically: In t8, t17, t18, t19, and t20, when the inner boundary is a point rather than a curve, the color-transition line absolutely must begin at that point, but in your rendering it doesn't.  In t23, t24, and t25, the color-transition line should be much closer to the visual middle of the quarter-ellipse.  Also, make sure that under no circumstances can the color-transition line appear to go "backward" (with the outer endpoint closer to the middle of that side than the inner endpoint).

I recommend you try just making the color-transition line run from the innermost to the outermost points of the corner's bounding box.  fantasai might know a case where *that's* not the right line, but I think it's more likely to be the right line under more circumstances.

Comment 5

[fantasai]

Ok, so I drew out mockups of various options based on Anil's implementation of the spec:
  http://lists.w3.org/Archives/Public/www-archive/2011Jul/0005.html

It's clear to me that the spec (t17-ray) is wrong. Zack's suggestion (t17-diagonal) is good, but from from the handful people I've showed it to, I got a unanimous vote for #4 (t17-tangent) as the prettiest of the set. That one takes the point at the 45deg tangent to the outer curve and connects that to the inner corner. Still need to investigate options for what happens when there's an inner curve, though.

Comment 6

[Zack Weinberg (:zwol)]

Yeah, I like t17-tangent best too, and that rule makes a lot of sense as where the line "should" be.  Maybe we should use it on the inside curve too, when there is one?

Comment 7

[fantasai]

We've got at least three options there.
  a) Make the same calculation on the inside curve, connect the 2 points.
  b) Take the perpendicular to the tangent of the outer curve
  c) Take the perpendicular to the tangent, then adjust the angle of the line
     until it's equally off-perpendicular to both the inner and outer curves.

Comment 8

[fantasai]

Ok, so Anil and I discussed it and he's going to prototype taking the point on the outer curve as determined by the tangent, and from there connecting to the closest point on the inner curve. We'll use the same method for the straight side of dashes (they're rather crooked in e.g. t23).

Comment 9

[Anil Shanbhag]

I have changed the implementation - now using t17-tangent [ see comment 5] 

Also changed the the end points choosing : Now measuring along inner curve [ if any ] and taking normal to outer curve

Comment 10

[fantasai]

Normal or shortest-distance? (They are not the same.)

Comment 11

[Zack Weinberg (:zwol)]

These are generally an improvement, but in t13 I think it's picking color transition points that are too close to the middle of the top and bottom sides.  OTOH that's a bit of an extreme case, maybe we don't mind.

Comment 12

[Anil Shanbhag]

@fantasai : Well it is the shortest distance between the point choosen on outer curve and inner curve AND as a result it is a normal to the inner curve

Comment 13

[fantasai]

@zwol: Yeah, but I think the way to adjust that would be to adjust how the angle ratio is calculated, not how we map that angle to a position on the curve. I think it's clear we have the latter part right, as demonstrated by the 45deg case. The boundaries here are that equal thickness should result in the 45deg tangent intersection, and zero thickness on either side should give the entire arc to the side that has thickness. Right now the angle is chosen as the fraction of 90deg given by the thickness ratio--perhaps we need a formulation that biases more towards the middle?

Comment 14

[Anil Shanbhag]

One thing that i am encountering trouble at is most of the calculations use modular arithmetic and array type referencing
Eg: borderRadii[i][i%2]

this translates into
if  . mBorderRadii[i].width()
else . mBorderRadii[i].height()

Any better way ? Am i missing something ?

Comment 15

[fantasai]

Hey Bas, for the curved dashed borders, would it be better to draw the dashes individually or to draw the solid curve and then clip out the gaps?

Comment 16

[Bas Schouten (:bas.schouten)]

The best way is composing a path of all dashes, and then filling that in one go.

Comment 17

[Anil Shanbhag]

Attachment: Draws dashes along corner - colors in one go .

Comment 18

[fantasai]

Comment on attachment 563106 [details] [diff] [review]
Draws dashes along corner - colors in one go .

       // If we don't have anything complex going on in this corner,
       // then we can just fill the corner with a solid color, and avoid
       // the potentially expensive clip.
-      if (simpleCornerStyle &&
+      if (noCompositeColorCorner &&
           IsZeroSize(mBorderRadii[corner]) &&
           IsSolidCornerStyle(mBorderStyles[sides[0]], corner))
       {

Since DrawSolidCorner can handle border radius <= corner size, you should
change the IsZeroSize() to check mInnerRadii.

Also change IsSolidCornerStyle to check both sides, since there's no longer
a both-sides-same check here. Otherwise this testcase fails:
  data:text/html;charset=utf-8,<!DOCTYPE html>%0D%0A<style>%0D%0A  p { width%3A 200px%3B height%3A 100px%3B%0D%0A      border-width%3A 20px%3B%0D%0A      border-radius%3A 10px%3B%0D%0A      border-color%3A orange blue%3B%0D%0A      border-style%3A double solid%3B%0D%0A    }%0D%0A<%2Fstyle>%0D%0A%0D%0A<p>%0D%0A%0D%0A

Note that since DrawSolidCorner can now handle two-color corners, you no
longer need to pass in the corner to IsSolidCornerStyle().

-        mContext->NewPath();
-        DoCornerSubPath(corner);
-        mContext->SetColor(MakeBorderColor(mBorderColors[sides[0]],
-                                           mBackgroundColor,
-                                           BorderColorStyleForSolidCorner(mBorderStyles[sides[0]], corner)));
-        mContext->Fill();
-        continue;
+        DrawSolidCorner(corner);
       }

You're missing the "continue;" statement, which is causing the code
to fall through below, giving bad results and breaking this testcase:
  data:text/html;charset=utf-8,<!DOCTYPE html>%0D%0A<style>%0D%0A  p { height%3A 100px%3B width%3A 200px%3B border-width%3A 20px%3B border-color%3A orange blue%3B border-radius%3A 0px%3B border-style%3A dashed%3B }%0D%0A<%2Fstyle>%0D%0A<p>

       if (simpleCornerStyle) {
         // we don't need a group for this corner, the sides are the same,
         // but we weren't able to render just a solid block for the corner.
-        DrawBorderSides(sideBits);
+
+        if(mBorderStyles[sides[0]] == NS_STYLE_BORDER_STYLE_DASHED &&
+           mBorderStyles[sides[0]] == NS_STYLE_BORDER_STYLE_DASHED &&
+           mInnerRadii[corner].width && mInnerRadii[corner].height) {
+          DrawDashedCorner(corner);
+        } else {
+          DrawBorderSides(sideBits);
+        }
+
+      } else if (mBorderStyles[sides[0]] == NS_STYLE_BORDER_STYLE_DASHED &&
+           mBorderStyles[sides[0]] == NS_STYLE_BORDER_STYLE_DASHED &&
+           mInnerRadii[corner].width && mInnerRadii[corner].height) {
+          DrawDashedCorner(corner);

You're checking the same side twice here instead of checking both sides.
This causes this testcase to break:
  data:text/html;charset=utf-8,<!DOCTYPE html>%0D%0A<style>%0D%0A  p { height%3A 100px%3B width%3A 200px%3B border-width%3A 20px%3B border-color%3A orange blue%3B border-radius%3A 60px%3B border-style%3A dashed solid%3B }%0D%0A<%2Fstyle>%0D%0A<p>
Note: In the first case, you actually only need to check one side because
simpleCornerStyle is only true if both sides have the same style.


The DoSideClipSubPath code is what handles different-style corner junctions.
Its transition line code hasn't been updated, so
  data:text/html;charset=utf-8,<!DOCTYPE html>%0D%0A<style>%0D%0A  p { height%3A 100px%3B width%3A 200px%3B border-width%3A 10px 20px%3B border-color%3A orange blue%3B border-radius%3A 60px%3B border-style%3A dashed%3B }%0D%0A<%2Fstyle>%0D%0A<p>
and
  data:text/html;charset=utf-8,<!DOCTYPE html>%0D%0A<style>%0D%0A  p { height%3A 100px%3B width%3A 200px%3B border-width%3A 10px 20px%3B border-color%3A orange blue%3B border-radius%3A 60px%3B border-style%3A double dashed%3B }%0D%0A<%2Fstyle>%0D%0A<p>
have different transition angles. Probably they should share the same calculations.
It looks like your transition angle calculations are a little backwards, though:
I think you're finding the correct point on the outer curve, but you're taking the
perpendicular from the outer curve. You need to find the perpendicular from the
inner curve. Here's a testcase that shows the awkwardness more clearly:
  data:text/html;charset=utf-8,<!DOCTYPE html>%0D%0A<style>%0D%0A  p { height%3A 100px%3B width%3A 200px%3B border-width%3A 40px 20px%3B border-color%3A orange blue%3B border-radius%3A 60px%3B border-style%3A dashed }%0D%0A<%2Fstyle>%0D%0A<p>
The line is not the shortest distance between the two curves. (Let me know if this
doesn't make sense and I'll try to explain it in more detail.)

=== nitpicky ===

Stray changes and formatting:
  Stray to .hgignore

Stray indentation/spacing changes:

   nsCSSBorderRenderer::ComputeInnerRadii(const gfxCornerSizes& aRadii,
  -                                       const gfxFloat *aBorderSizes,
  -                                       gfxCornerSizes *aInnerRadiiRet)
  +                                      const gfxFloat *aBorderSizes,
  +                                      gfxCornerSizes *aInnerRadiiRet)


  -        for (int cornerSide = 0; cornerSide < 2; cornerSide++) {
  -          mozilla::css::Side side = mozilla::css::Side(sides[cornerSide]);
  -          PRUint8 style = mBorderStyles[side];
  ...
  +          for (int cornerSide = 0; cornerSide < 2; cornerSide++) {
  +            mozilla::css::Side side = mozilla::css::Side(sides[cornerSide]);
  +            PRUint8 style = mBorderStyles[side];
  ...
   // Setup the stroke style for a given side

  -  void SetupStrokeStyle(mozilla::css::Side aSize);
  +  void SetupStrokeStyle (mozilla::css::Side aSize);

   const twoFloats gradientCoeff[4] = { { -1, +1 },
-                                       { -1, -1 },
-                                       { +1, -1 },
-                                       { +1, +1 } };
+                                      { -1, -1 },
+                                      { +1, -1 },
+                                      { +1, +1 } };

Missing spaces after commas:
  -    ComputeInnerRadii(aBorderRadii, aBorderSizes, &innerRadii);
  +    ComputeInnerRadii(aBorderRadii,aBorderSizes,&mInnerRadii);

Unused code:

  +struct DashGroup {
  +  gfxFloat length;
  +  gfxFloat gap;
  +};

Extraneous code:

                                  mCompositeColors[sides[1]] == NULL &&
                                  AreBorderSideFinalStylesSame(sideBits);

+      PRBool noCompositeColorCorner = mCompositeColors[sides[0]] == NULL &&
+                                      mCompositeColors[sides[1]] == NULL;

Since we're computing this, we can simplify the code a bit by removing
the simpleCornerStyle definition (which is now only used once) and replacing
  if (simpleCornerStyle) {
with
  if (noCompositeColorCorner && AreBorderSideFinalStylesSame(sideBits)) {

=== end nitpicky ===

You need more tests so that every code path here is tested, not just the all-dashed-borders ones. I don't know how to make them all into reftests, though.
Bas/roc/zwol, do you have suggestions?

Comment 19

[fantasai]

Attachment: review comments from a previous revision of the patch

Attaching the interesting parts of my previous review of Anil's patch, since they might be helpful to understanding the current version's code flow.

Comment 20

[fantasai]

Comment on attachment 563106 [details] [diff] [review]
Draws dashes along corner - colors in one go .

Bas, can you take a look and make sure the graphics handling is going in the right direction? There's basically 3 methods of corner join in nsCSSBorderRenderer: same-color fill, gradient fill, and clipped-buffered fill. We reorganized the code a bit to use the gradient fill in more cases.

Comment 21

[Florian Bender]

Would be nice to have a feedback and then un-bitrot this one*. I'd really like to see this very long-standing and well visible issue finally fixed. 

* or un-bitrot and then feedback/review?

Comment 22

[Tooru Fujisawa [:arai]]

Fixed as a part of bug 382721.