Closed Bug 635222 Opened 9 years ago Closed 9 years ago

[css3-images] Radial gradients show the wrong color when there are 2 100% color stops at the end

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: lea, Assigned: dbaron)

References

(Depends on 1 open bug, )

Details

(Keywords: testcase)

Attachments

(3 files)

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110217 Firefox/4.0b12pre
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110214 Firefox/4.0b12pre

See the testcase in Webkit and Gecko. Webkit gets it right, as the background should be white. Gecko displays a black background.
If the black color stop is given a position of 101%, Gecko shows white, just like it should. If the white is 99% instead of 100% Gecko's rendering is also correct.

Reproducible: Always

Actual Results:  
Black background

Expected Results:  
White background
Could you attach as a reduced html testcase to this bug please.
Version: unspecified → Trunk
After discussing it with Tab on www-style [1], I should add that Webkit's behavior is also buggy and should also be reported. The expected result is not a solid color, but an ellipse with a solid edge.

Ed, I provided a reduced testcase, just not as an attachment.

     [1]: http://lists.w3.org/Archives/Public/www-style/2011Feb/0538.html
The SVG specification http://www.w3.org/TR/SVG/pservers.html#GradientStops says...

If two gradient stops have the same offset value, then the latter gradient stop controls the color value at the overlap point

The -moz-radial-gradient implementation is consistent with SVG's gradients so I would think this is the desired behavious and this bug is invalid.
SVG gradients don't have size/shape keywords though, so that's irrelevant.

Also, Tab Atkins is the editor of Image Values and he said it's not the desired behavior.
Also, what you're quoting doesn't in any way imply that the whole gradient should turn into a solid color. It just means that at the 100% point, the color should be black.
Per https://developer.mozilla.org/en/CSS/-moz-radial-gradient

Rendering of color-stops in CSS gradients follows the same rules as color-stops in SVG gradients.

You've only got two stops so if you ignore the first as the latter gradient colour supersedes it then you get all black.
Perhaps you should ask the SVG wg what they think?
Marking new (to make sure people get bugmail) and ccing Tab.  Tab, see above about the inconsistency with SVG gradients.
Status: UNCONFIRMED → NEW
Ever confirmed: true
@Robert:

So what you're suggesting is that even in cases like linear-gradient(white 50%, black 50%) it should also be all black? That's inconsistent with the spec, the implementations, SVG and authors' needs.

Also, as I said above, SVG gradients don't have size and shape keywords as far as I know, so you can't really compare it to what SVG is doing. I don't see any inconsistency here.
Your examples don't use size and shape keywords. All we're talking about is what happens if you have two stops at the same offset. SVG and CSS gradients need to be consistent here and the SVG specification defines how stops work.

Looking at the SVG spec example I can see a case for arguing that all white is the right answer.

If you construct some SVG examples is there browser consistency?
> Your examples don't use size and shape keywords. 

Huh? All 3 of my examples use the contain shape/size keyword.
There is no conflict with SVG gradients - SVG requires the same behavior here that CSS does.

If you look at the *rest* of the bullet point that Comment 3 quotes, you see:

> In particular:
>   <stop offset="0" stop-color="white"/>
>   <stop offset=".2" stop-color="red"/>
>   <stop offset=".2" stop-color="blue"/>
>   <stop offset="1" stop-color="black"/>
> will have approximately the same effect as:
>   <stop offset="0" stop-color="white"/>
>   <stop offset=".1999999999" stop-color="red"/>
>   <stop offset=".2" stop-color="blue"/>
>   <stop offset="1" stop-color="black"/>
> which is a gradient that goes smoothly from white 
> to red, then abruptly shifts from red to blue, and
> then goes smoothly from blue to black.

This case is correctly handled when there is at least one color-stop at a different position.  The error occurs when all of the color-stops overlap.
Attached file reporter's testcase
The first place we do any manipulation of stop positions, as far as I can tell, is in http://hg.mozilla.org/mozilla-central/file/26c7b580d5a2/layout/base/nsCSSRendering.cpp#l1925 , and it doesn't look like it drops any stops, so I suspect the bug here is in cairo.
Severity: minor → normal
OS: Mac OS X → All
Hardware: x86 → All
I see the problem on both Linux and Windows.  The reporter and bz both seem to see it on Mac, so it's cross-platform.
Summary: Radial gradients show the wrong color when there are 2 100% color stops at the end → [css3-images] Radial gradients show the wrong color when there are 2 100% color stops at the end
The underlying problem goes away if I add a stop earlier in the gradient.

I think the basic problem is that this code in PaintGradient is broken (in particular, things go wrong if the if-condition is true):

  double lastStop = stops[stops.Length() - 1].mPosition;
  // Cairo gradients must have stop positions in the range [0, 1]. So,
  // stop positions will be normalized below by subtracting firstStop and then
  // multiplying by stopScale.
  double stopScale;
  double stopDelta = lastStop - firstStop;
  if (stopDelta < 1e-6 || lineLength < 1e-6 ||
      (aGradient->mShape != NS_STYLE_GRADIENT_SHAPE_LINEAR &&
       (radiusX < 1e-6 || radiusY < 1e-6))) {
    // Stops are all at the same place. Map all stops to 0.0.
    // For radial gradients we need to fill with the last stop color,
    // so just set both radii to 0.
    stopScale = 0.0;
    radiusX = radiusY = 0.0;
    lastStop = firstStop;
  } else {
    stopScale = 1.0/stopDelta;
  }
Yup, that seems to be it.  You need to separate out the case when there's only one stop (in which case the true clause of the existing code is appropriate) and the case where the first and last stop are different but on top of each other.
Comment on attachment 513497 [details]
reporter's testcase

This testcase isn't sensible, both because the body has zero height and because the default size is 'ellipse cover' which means that nothing outside of the 100% position is visible.

I have a patch that fixes the other testcase, and flips this one to white (as it should be), but I still need to write automated tests.
(Re: comment 16 and comment 17) That code was actually largely ok, it was code below that was the problem; the patch is:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/c74387b3e74b/radial-gradient-all-stops

Given how explicitly the bit at the bottom of the patch was intending this effect, I'm guessing the spec used to say something different here?
No, it never did.  However, the spec does talk about when the starting and ending point of the gradient-line are at the same position, in which case you'd have a degenerate gradient and just use the last color-stop.  The start and end of the gradient-line aren't the same thing as the first and last color-stop, but I can see how a brain-**** could lead one to make the mistake when writing that section of code.
Attached patch patchSplinter Review
Attachment #513658 - Flags: review?(roc)
The cases we actually had tests for, which I actually broke with the earlier version of the patch, were these cases:
  # In certain circumstances the given parameters may define a degenerate
  # shape - a circle or ellipse with a radius of 0. In these instances the
  # gradient image is just a solid color equal to the color of the last
  # color-stop in the rule.
That particular degenerate case is *more* degenerate than the all-stops-at-the-same-position case, but we were treating them the same.
https://hg.mozilla.org/projects/birch/rev/ab94c1fd24cd

... and it turns out the test has a one pixel difference in radius on Mac only, for some reason.  I probably should debug it, but it doesn't seem high priority (filed bug 638664):

https://hg.mozilla.org/projects/birch/rev/63aee8dc13c6
Whiteboard: fixed-in-birch
https://hg.mozilla.org/mozilla-central/rev/ab94c1fd24cd
https://hg.mozilla.org/mozilla-central/rev/63aee8dc13c6
https://hg.mozilla.org/mozilla-central/rev/87cc48f6190a
Assignee: nobody → dbaron
Status: NEW → RESOLVED
Closed: 9 years ago
Priority: -- → P3
Resolution: --- → FIXED
Whiteboard: fixed-in-birch
Target Milestone: --- → mozilla2.2
There appears to still be a similar bug, at least in Nightly, so either this wasn't fixed or I just found another bug: http://jsfiddle.net/leaverou/n7Kd2/
Btw, the bug I mentioned in my last comment makes a few of the patterns displayed here: http://leaverou.me/css3patterns/ not work in Gecko, so it's not a rare case, given that technique's increasing popularity.
Please can you file a new bug in Core::Style System (CSS) with that testcase - thanks!
Sure, in a minute!
You need to log in before you can comment on or make changes to this bug.