Filing this bug on adding native support for -webkit-gradient() expressions (not using the CSS Unprefixing Service). (Note that Bug 1210575 covers -webkit-linear-gradient() & -webkit-radial-gradient() expressions. This bug here is about "-webkit-gradient" which is an older syntax & doesn't really overlap with our existing gradient parsing code; hence, I'm giving it its own dedicated bug, here.) I think the only documentation on "-webkit-gradient()" syntax is https://www.webkit.org/blog/175/introducing-css-gradients/ Quoting that post: > The syntax is as follows: > -webkit-gradient(<type>, <point> [, <radius>]?, <point> [, <radius>]? [, <stop>]*)

FWIW, here's the code that Blink (and WebKit based on the file-path) uses to parse these expressions: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp&l=5076 The function is called "CSSPropertyParser::parseDeprecatedGradient". (side note: there's also a code-comment elsewhere in the file saying "This should go away once we drop support for -webkit-gradient". Heh...)

Apple also has some documentation about -webkit-gradient on developer.apple.com [0], under "Prior Syntax (-webkit-gradient)", with a bit of prose & some examples. However, right now, half of the examples (the radial ones) don't actually render anything in Safari or Chrome, because they have px/% units on their <radius> values, and that makes them invalid because (as noted in the webkit.org blog post from comment 0), "a radius is a number". (i.e., no units) Those examples work if I remove the units, though, as shown in this jsfiddle[1] (view in Safari or Chrome). I reported this doc-bug via the "feedback" link on that page, and also to a WebKit developer [2], so hopefully the docs will be corrected at some point. [0] https://developer.apple.com/library/safari/documentation/InternetWeb/Conceptual/SafariVisualEffectsProgGuide/Gradients/Gradient.html#//apple_ref/doc/uid/TP40008032-CH10-SW34 [1] https://jsfiddle.net/nj41f3wx/ [2] https://twitter.com/CodingExon/status/662043748927320064

I've got a patch with complete (I think?) parser code for this, but *representing* the parsed value in computed style turns out to be a bit tricky, because -webkit-gradient(...) can actually express a different (albeit overlapping) set of things, as compared to standard unprefixed gradients. Hence, not all -webkit-gradient expressions can be mapped onto our internal representation. Here's a list of the particular things I've run up against, where -webkit-gradient can express something that doesn't map to an equivalent standards-based gradient expression: (1) Standardized gradients are restricted in their choice of starting & ending points for their gradient line (the line where we actually do the fade from one color to another color). In contrast, -webkit-gradient(linear, ...) expressions have complete control over those points, and they can be expressed as absolute px values (with no unit), or percentages, or a mix. Example that can't be represented w/ standard syntax: > background: -webkit-gradient(linear, 20 30, 70% 50, > color-stop(0%, blue), color-stop(100%, purple)); (2) A -webkit-gradient(radial, ...) expression can have the color-fade happen *between two different nonzero-side circles*, whereas a standardized radial-gradient has the color-fade happen from a point (a 0-sized circle) out to the bounds of a circle. Example that can't be represented w/ standard syntax: > background: -webkit-gradient(radial, 50% 50%, 30, 40% 40%, 60, from(white), to(black)); The structures that we use to represent gradients aren't really suited to holding these more-expressive expression right now. For now, I'm thinking of shoehorning these sorts of things into something approximately-similar, and filing a lower-priority followup on removing the shoehorning. (Depending on how much people use this expressiveness & depend on the resulting gradient being *exactly* right, it may not be worth caring about these special cases.)

> *between two different nonzero-side circles* er, "nonzero-sized"

(In reply to Daniel Holbert [:dholbert] from comment #3) > For now, I'm thinking of shoehorning these sorts of things into something > approximately-similar, and filing a lower-priority followup on removing the > shoehorning. (Depending on how much people use this expressiveness & depend > on the resulting gradient being *exactly* right, it may not be worth caring > about these special cases.) This sounds like the right approach. I would guess the special cases are very low priority, but we can track incoming bugs to help determine this.

Here's the parsing patch that I hinted at at the beginning of comment 3. This patch only focuses on *parsing* "-webkit-gradient", and rendering something, but not actually producing the right rendering. (It does actually use the color-stops, and it uses the -webkit-gradient expression's first <point> as the gradient location [not actually what it means, but no big deal]. But we don't do anything with the rest of the positioning values. This will be fixed in the next patch.) I've added a bunch of valid/invalid "-webkit-gradient" expressions to our mochitests, which are useful to exercise this parsing code and to verify which values are accepted/rejected. I verified that Chrome accepts/rejects all of these values as-expected, too.

(reposting patch to fix a typo and add a few more comments) Note on parsing strategy (nothing too surprising): - ParseWebkitGradient() is the main parsing function here. - On parse failure, it calls SkipUntil(')') to jump to the end of the "-webkit-gradient(...invalid stuff...)" expression. - In contrast, when its helper functions fail, they simply put back whatever they failed to parse & return false (and then ParseWebkitGradient() catches the false return-value & seeks to the end of the expression). - The one exception to this is "ParseWebkitGradientColorStops()", which does the SkipUntil(')') on its own. This allows for ParseWebkitGradient() to have a more concise final return statement, and it also matches what we do in the normal gradient-parsing code (ParseLinearGradient() & ParseRadialGradient() & ParseGradientColorStops())

Comment on attachment 8683881 [details] [diff] [review] part 1, v2 Add support for parsing CSS -webkit-gradient expressions, into local variables at least (and use some of them to render something) Review of attachment 8683881 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, awaiting an answer on duplicate stops. ::: layout/style/nsCSSParser.cpp @@ +10044,5 @@ > + return false; > + } > + // Choose our keyword table: > + const nsCSSKeyword* kwTable = aIsHorizontal ? kHorizKeywords : kVertKeywords; > + // Convert keyword to percent value (0%, 50%, or 100% Missing ")". @@ +10143,5 @@ > + nsCSSValueGradientStop* stop = aGradient->mStops.AppendElement(); > + > + // Parse color-stop location (or infer it, for shorthands "from"/"to"): > + if (mToken.mIdent.LowerCaseEqualsLiteral("color-stop")) { > + CSSParseResult result = ParseVariant(stop->mLocation, Since you don't have a VARIANT_CALC or other type that can consume multiple tokens, you can use ParseSingleTokenVariant() here (which returns a boolean), and if you wish, embed that in the if statement below. @@ +10251,5 @@ > + nsCSSValueGradientStop* stop = aGradient->mStops.AppendElement(); > + *stop = aGradient->mStops[0]; > + } else { > + // We have >2 stops. Sort them in order of increasing location. > + aGradient->mStops.Sort(GradientStopComparator()); How are duplicate stops handled in WebKit? Not using a stable sort here means that we don't know which colour will win when there are duplicates. ::: layout/style/test/property_database.js @@ +794,5 @@ > + // linear w/ trailing comma (which implies missing color-stops): > + "-webkit-gradient(linear, 1 2, 3 4,)", > + > + // linear w/ invalid color values: > + "-webkit-gradient(linear, 1 2, 3 4, from(invalidcolorname)))", Are the extra ")"s at the end of these intentional? @@ +800,5 @@ > + "-webkit-gradient(linear, 1 2, 3 4, from(initial)))", > + "-webkit-gradient(linear, 1 2, 3 4, from(currentColor)))", > + "-webkit-gradient(linear, 1 2, 3 4, from(00ff00)))", > + "-webkit-gradient(linear, 1 2, 3 4, from(##00ff00)))", > + "-webkit-gradient(linear, 1 2, 3 4, from(#00ff)))", // (need 3 or 6 digits) Note that four-hex-digit colours might become valid soon: https://twitter.com/grorgwork/status/661964480041975808

(In reply to Cameron McCormack (:heycam) from comment #8) > Missing ")". Fixed. > @@ +10143,5 @@ > Since you don't have a VARIANT_CALC or other type that can consume multiple > tokens, you can use ParseSingleTokenVariant() here (which returns a > boolean), and if you wish, embed that in the if statement below. Nice, good catch! Fixed. > > + // We have >2 stops. Sort them in order of increasing location. > > + aGradient->mStops.Sort(GradientStopComparator()); > > How are duplicate stops handled in WebKit? Not using a stable sort here > means that we don't know which colour will win when there are duplicates. Ooh, good question. Looks like Blink (and presumably WebKit) uses a stable sort... void CSSGradientValue::addDeprecatedStops(Gradient* gradient, const LayoutObject& object) { [...] if (!m_stopsSorted) { if (m_stops.size()) std::stable_sort(m_stops.begin(), m_stops.end(), compareStops); m_stopsSorted = true; } https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/css/CSSGradientValue.cpp&cl=GROK&l=200 ...so, we should too. Unfortunately nsTArray doesn't have one (bug 1147091). I'll just use std::stable_sort(), since it works well enough, and we've fallen back to that for nsTArray elsewhere, e.g.: https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGridContainerFrame.cpp?rev=5ae04c1c341c#78 > ::: layout/style/test/property_database.js > > + // linear w/ invalid color values: > > + "-webkit-gradient(linear, 1 2, 3 4, from(invalidcolorname)))", > > Are the extra ")"s at the end of these intentional? Oops! Not intentional, good catch. I'll remove those, so we're only testing one invalid thing at a time. (And I'm pretty sure we already have generalized mochitests that test extra ")" at the end of otherwise-valid properties, and things like that.) > > + "-webkit-gradient(linear, 1 2, 3 4, from(#00ff)))", // (need 3 or 6 digits) > > Note that four-hex-digit colours might become valid soon: > https://twitter.com/grorgwork/status/661964480041975808 Good point. We could just update this test if/when we support those, but I'll make things simpler by just using a five-digit hex value as my "invalid" example here.

Comment on attachment 8683881 [details] [diff] [review] part 1, v2 Add support for parsing CSS -webkit-gradient expressions, into local variables at least (and use some of them to render something) Thanks, r=me with std::stable_sort.

Thanks! Here's the updated part 1, with review comments addressed.

Here are my reftests for this bug. Note that there are two reftests that cover cases where we know we're not quite getting things right (these have "approx" in the name) -- see comment 3 / comment 5. The "references" for these testcases don't intend to be true references for correctness - they just represent how-we-render-these-tests-right-now, so that we notice if we accidentally change our behavior on any of these cases [and don't crash, etc]. I'll use dbaron's strategy of putting reftests (annotated as failing) as the first thing in the patch-stack, and then marking tests as passing as-appropriate on intermediate patches.

Here's an updated version of "part 1", refactored slightly in two ways: - I've added stub functions (populated in later patches here) to do linear/radial-specific fixup. These are called FinalizeLinearWebkitGradient and FinalizeRadialWebkitGradient. - I've changed ParseWebkitGradientColorStops() to no longer be responsible for doing the ultimate outparam-setting-and-returning (and seeking to the end of the gradient expression if something goes wrong). I changed this because I want to invoke my "Finalize" functions to run *after* we've parsed color stops, so that they can reverse the order of color-stops in some radial cases. I'll post an interdiff for these changes and request review on that, so you don't have to look over this whole patch again.

(Have to run now, but I'll post the remaining patches -- which populate FinalizeLinearWebkitGradient & FinalizeRadialWebkitGradient -- later tonight.)

This patch populates the stub FinalizeLinearWebkitGradient() that I added in the updated version of "part 1", to create a linear-flavored nsCSSValueGradient based on the parsed expression.

This patch populates the stub FinalizeRadialWebkitGradient() that I added in the updated version of "part 1", to create a radial-flavored nsCSSValueGradient based on the parsed expression.

Comment on attachment 8688197 [details] [diff] [review] part 0: reftests Review of attachment 8688197 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these addressed (or an explanation of the transparent div handling, if I'm missing something). ::: layout/reftests/webkit-gradient/webkit-gradient-approx-radial-1-ref.html @@ +17,5 @@ > + br { clear: both; } > + </style> > +</head> > +<body> > + <!-- Inner circle has nonzero radius, in testcase: --> As you've done in webkit-gradient-approx-linear-1-ref.html, can you just briefly mention how we drop information when converting to a gradient we can handle? ::: layout/reftests/webkit-gradient/webkit-gradient-linear-2.html @@ +27,5 @@ > +<body> > + <!-- No color stops (transparent): --> > + <div style="background: -webkit-gradient(linear, left center, right center > + )"></div> > + <!-- Add another background to be sure we're transparent, not white: --> I don't understand how this is working. Don't we need to render the should-be-transparent div on top of the pink/purple one?

Comment on attachment 8688199 [details] [diff] [review] interdiff between part 1 v3 & v4 Review of attachment 8688199 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for supplying an interdiff.

Comment on attachment 8688284 [details] [diff] [review] part 2: handle linear gradients Review of attachment 8688284 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsCSSParser.cpp @@ +10272,5 @@ > +// across the painted area's center. (The legacy -moz-linear-gradient syntax > +// also lets us store an angle.) > +// > +// In this function, we try to go from the two-point representation to an > +// equivalent or approximately-equivaelnt one-point representation. equivalent @@ +10322,5 @@ > + // progress from this first point, towards the center of the covered element, > + // to a reflected end point on the far side. (Note that we have to use > + // mIsLegacySyntax=true for this to work, because standardized (non-legacy) > + // gradients place some restrictions on the reference point [namely, that it > + // use percent units & be on the border of the element.) missing "]"

Comment on attachment 8688285 [details] [diff] [review] part 3: handle radial gradients Review of attachment 8688285 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsCSSParser.cpp @@ +10369,5 @@ > + // Swap 0th color stop with the final one, 1st with next-to-final-one, etc. > + size_t swapIdx = aGradient->mStops.Length() - 1 - i; > + MOZ_ASSERT(swapIdx > i, "Should only go up to halfway through array"); > + Swap(aGradient->mStops[i], aGradient->mStops[swapIdx]); > + } Can we use std::reverse(aGradient->mStops.begin(), aGradient->mStops.end()) instead? (It would be nice if nsTArray had a Reverse method on it that did this...)

(In reply to Cameron McCormack (:heycam) (away Nov 23 – Dec 4) from comment #18) > I don't understand how this is working. Don't we need to render the > should-be-transparent div on top of the pink/purple one? We use the transparent gradient *twice* -- once on its own (where it lets the white page background shine through), and then a second time, as part of a longer / more complex gradient expression, right after that "Add another" comment, here: <div style="background: linear-gradient(to right, pink, purple), -webkit-gradient(linear, left center, right center)"> (Note the list of 2 backgrounds there -- this is one div, with two stacked backgrounds. The transparent one lets the purple one shine through. We could also use two nested divs, as you suggest, but we don't need to; a comma-separated list of backgrounds works just as well here.) (In reply to Cameron McCormack (:heycam) (away Nov 23 – Dec 4) from comment #21) > Can we use std::reverse(aGradient->mStops.begin(), aGradient->mStops.end()) > instead? Likely yes - I'll give that a shot. Thanks for the suggestion! (I'll address that & the other review comments tomorrow.)

Addressed all review feedback. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b07333c6279 I've added a mac-only fuzzy-if annotation on one test, for some surprising & visually-imperceptible differences in gradient renderings on mac (for which I filed bug 1225372). (I tried to work around the fuzziness by changing my gradient expression in the reference case, but it defied my attempts to make it pass; I think something a bit more complex than my summary in bug 1225372 is going on here.) So, I ended up just using fuzzy-if, since it's only a maximum difference of 3 in any color channel (and only on TreeHerder-mac [not my own Mac], and only in a subset of 1 test.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3800368143 https://hg.mozilla.org/integration/mozilla-inbound/rev/d9418704c3c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/de257a913ac5 https://hg.mozilla.org/integration/mozilla-inbound/rev/631ec6c50d24

https://hg.mozilla.org/mozilla-central/rev/3c3800368143 https://hg.mozilla.org/mozilla-central/rev/d9418704c3c6 https://hg.mozilla.org/mozilla-central/rev/de257a913ac5 https://hg.mozilla.org/mozilla-central/rev/631ec6c50d24