Filing this bug on adding native support for -webkit-linear-gradient & -webkit-radial-gradient (not using the CSS Unprefixing Service). (This bug does not cover the older "-webkit-gradient(radial|linear, ...)" syntax -- I'll file a separate bug for that, since that's considerably more different from the standardized gradient syntax, and so that'll likely require its own separate codepath.)

First things first: So, we currently have a function called "ParseWebkitPrefixedGradient" (which calls out to the JS-implemented CSSUnprefixingService). This function will become confusing/ambiguous once we've added a new (native, no-JS) way of doing this parsing. So, before I add the new way, I'm renaming this function to make it more specific -- adding "WithService" to the end of its name. (Eventually, this function will probably die, since the native support will obsolete it. But in the interim, it's useful to keep the CSSUnprefixingService stuff around until we've got native support that we're ready to ship.)

This is also just a refactoring patch -- just shifting out a large "is this css function-token valid for background-image?" if-check into a helper function. Note that many of these lines are currently well over 80 chars long, and later patches here will add some even longer lines (with e.g. "-webkit-repeating-linear-gradient") which would make them even longer. This helper-function makes that less egregious, and also makes the logic a bit easier to follow I think. (This patch also annotates CSSParserImpl::ShouldUseUnprefixingService as "const" -- which it technically could & should've previously been annotated as -- so that the new helper-function can also be 'const'.)

Comment on attachment 8668684 [details] [diff] [review] part 1: Add "WithService" suffix to existing function ParseWebkitPrefixedGradient, for clarity Review of attachment 8668684 [details] [diff] [review]: ----------------------------------------------------------------- In the commit message: > (This patch rename the function to "ParseWebkitPrefixedGradientWithService", s/rename/renames/

(In reply to Cameron McCormack (:heycam) from comment #3) > s/rename/renames/ Thanks! Fixed. Remaining patches to do the real work coming soon, hopefully tomorrow.

This patch refactors ParseRadialGradient / ParseLinearGradient to use a "flags" bitfield instead of multiple boolean args to customize the parsing behavior. (And then, my next patch will add a flag that we can use here, for webkit gradient parsing.)

This patch: - Hooks up the pref "layout.css.prefixes.webkit" (being added in bug 837211) to a static bool in nsCSSParser (so that we'll be able check this bool before handling webkit-*-gradient support in subsequent patches). - Prevents us from calling out to the CSSUnprefixingService if that ^ bool is set. (We shouldn't be applying both types of webkit-prefix-support in parallel, for sanity's sake.)

If webkit prefix support is enabled, this patch lets webkit-prefixed gradient expressions reach our gradient-parsing code (getting through the "if" block that I refactored out in part 2), with a new gradient-parsing flag that tells us to activate webkit-specific gradient parsing quirks. (These quirks are still to-be-added in a later patch here -- at this point in the queue, -webkit-*gradient is treated just like the standard versions.) Notes: - We already let -webkit prefixed gradients reach the gradient-parsing code if the CSSUnprefixingService is active. This patch just checks the new pref as well, as one other way of letting them through. (And then *either* our native gradient-parsing codepath *or* the unprefixing service will handle them.) - We previously didn't support -webkit-repeating-{linear|radial}-gradient with the JS-implemented "CSS Unprefixing Service". But this is valid syntax (which works in Chrome) that we *should* support, so this patch lets that through for our new native webkit prefix support to handle.

(In reply to Daniel Holbert [:dholbert] from comment #7) > - We previously didn't support -webkit-repeating-{linear|radial}-gradient > with the JS-implemented "CSS Unprefixing Service". But this is valid syntax > (which works in Chrome) that we *should* support, so this patch lets that > through for our new native webkit prefix support to handle. Is it OK to let these two properties through to the CSS unprefixing service too, or should we be splitting out the sWebkitPrefixedAliasesEnabled and ShouldUseUnprefixingService() checks so that the latter doesn't allow them?

Yup -- it should be OK. We'll pass them to the unprefixing service "generateUnprefixedGradientValue()" method, and it'll reject them because it won't recognize the repeating gradient expression, by returning false here: http://mxr.mozilla.org/mozilla-central/source/layout/style/CSSUnprefixingService.js?rev=39c35b0c2e04&mark=174-174#171 > 162 if (aPrefixedFuncName == "-webkit-gradient") { [...] > 168 }else{ // we're dealing with more modern syntax - should be somewhat easier, at least for linear gradients. > 169 // Fix three things: remove -webkit-, add 'to ' before reversed top/bottom keywords (linear) or 'at ' before position keywords (radial), recalculate deg-values > 170 // -webkit-linear-gradient( [ [ <angle> | [top | bottom] || [left | right] ],]? <color-stop>[, <color-stop>]+); > 171 if (aPrefixedFuncName != "-webkit-linear-gradient" && > 172 aPrefixedFuncName != "-webkit-radial-gradient") { > 173 // Unrecognized prefixed gradient type > 174 return false; > 175 } And in fact, this is already what happens, if a site specifies e.g. "background-image: -webkit-repeating-radial-gradient". The huge "if" gatekeeper-code that I refactored in part 2 is only ever checked when we're parsing the "background" shorthand. (not e.g. "background-image") (This ^ quoted code may make you ask the question "OK, what about plain '-webkit-gradient'? That doesn't seem to be handled here, on the non-unprefixing-service codepath." That's true; see second half of comment 0.)

(Note that CSSUnprefixingService is only enabled for a whitelist of sites, and we can fairly-safely assume that these sites don't actually depend on repeating-gradient expressions [or else we would have noticed them when adding those sites to the whitelist & getting them working]. But hypothetically, if they *do* rely on any such expressions (and we're failing to unprefix them), then "part 5" will represent a slight increase in parser-work, because we'll be making an unnecessary round-trip through the CSSUnprefixingService (to parse a doomed-to-fail expression) which we weren't making before. I'm not really worried about that though, because: (a) The whitelist is short enough that one extra roundtrip on a given site that already uses the service won't have a meaningful impact in the real world. (b) As noted above, this will already happen for repeating-gradient expressions in "background-image" [just not for "background"] (c) The CSSUnprefixingService is getting replaced anyway [by this bug here, in part]

Landed (reviewed) parts 1 through 4: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a17eed123926 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b2cc761bf9f7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c82707060b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/73d17d930b75

Updating part 5 to add some tests (just appending to property_database.js's list of valid & invalid gradient expressions properties). As of this patch, I'm only adding simple gradient expressions which should be handled the same [accepted/rejected] regardless of whether they're prefixed. I tested these in Chrome to be sure it accepts them. (As hinted by XXXdholbert comments in these tests, later patches will add more tests where -webkit prefixed expression is treated differently than a -moz and/or unprefixed expression. Those wouldn't pass at this point, so I'm not adding them yet.)

https://hg.mozilla.org/mozilla-central/rev/a17eed123926 https://hg.mozilla.org/mozilla-central/rev/b2cc761bf9f7 https://hg.mozilla.org/mozilla-central/rev/d7c82707060b https://hg.mozilla.org/mozilla-central/rev/73d17d930b75

Note: -webkit-linear-gradient & -webkit-radial-gradient were introduced in this blog post: https://www.webkit.org/blog/1424/css3-gradients/ ...and I'm pretty sure they correspond closely to this spec revision (based on the datestamp on that blog post & a bit of testing): http://www.w3.org/TR/2011/WD-css3-images-20110217/#linear-gradients http://www.w3.org/TR/2011/WD-css3-images-20110217/#radial-gradients

Comment on attachment 8671069 [details] [diff] [review] part 5 v2: allow -webkit-*-gradient expressions (& "repeating" variants) to reach our gradient parsing code, if webkit prefix support is preffed on Landed part 5: https://hg.mozilla.org/integration/mozilla-inbound/rev/a792738b876e

So as of part 5, we treat "-webkit-radial-gradient" expression just like an unprefixed expression. This patch implements & tests the main parsing quirks that make it differ from unprefixed expressions -- in particular: - We use the same "legacy" keyword database that -moz-radial-gradient uses (to get "contain"/"cover" sizing keywords) - We skip the code for parsing <lengths> to size a shape, because that's not valid for -webkit-radial-gradient expressions. (Again, this is the same as -moz-radial-gradient). That syntax was added later in the spec history. - We skip the code for parsing the "at" keyword for positioning (again matching "-moz") - We prevent ourselves from parsing an angle as part of the gradient's position. This angle-parsing is a quirk that was added to the spec at some point and is supported in -moz-radial-gradient, but was later removed when the syntax was standardized. Tests included, for valid & invalid cases. If you'd like to take a look at spec text for this syntax, see links in comment 14. I verified that the included valid/invalid testcases are either rendered or not-rendered in Chrome, as-appropriate for the list that I'm putting them in.

Comment on attachment 8674101 [details] [diff] [review] part 6: implement -webkit-radial-gradient parse quirks with contain/cover keywords, sized shapes, "at" keyword, & angles Review of attachment 8674101 [details] [diff] [review]: ----------------------------------------------------------------- You'll need to rebase over bug 1213076. ::: layout/style/test/property_database.js @@ +682,4 @@ > // * initial length > + "-webkit-linear-gradient(10px, red, blue)", > + > + // *<shape> followed by angle: Nit: space after "*".

Comment on attachment 8674101 [details] [diff] [review] part 6: implement -webkit-radial-gradient parse quirks with contain/cover keywords, sized shapes, "at" keyword, & angles (In reply to Cameron McCormack (:heycam) from comment #18) > You'll need to rebase over bug 1213076. Thanks, done. > ::: layout/style/test/property_database.js > > + // *<shape> followed by angle: > > Nit: space after "*". Fixed. Landed part 6: https://hg.mozilla.org/integration/mozilla-inbound/rev/d25420b73478

This patch refactors existing code in ParseLinearGradient() -- no behavior changes. (in preparation for the next patch, which will implement -webkit-linear-gradient's parsing quirks) In particular: - I'm refactoring out some existing logic for "Did ParseBoxPositionValues parse something that only uses top/left/right/bottom/center keywords?" into a helper-function. (I'm going to add a second caller for this new helper-function in the next patch.) - I'm removing some only-used-once alias-variables ("ty" and "id"), and I'm shifting their one usage to be *before* we call UngetToken(), which feels slightly more kosher. (Since the UngetToken call is to allow ourselves to re-parse the token in other parsing functions lower down.) - I'm inverting some "haveAngle + expect-comma" parsing logic, in a way that shouldn't change behavior, to prepare for adding some -webkit-linear-gradient nuance to that spot in the next patch. - And I'm tweaking/adding some comments for clarity.

This implements -webkit-linear-gradient parse quirks. As noted above, the syntax is described here: http://www.w3.org/TR/2011/WD-css3-images-20110217/#linear-gradients Quoting: > <linear-gradient> = linear-gradient( > [ > [ [top | bottom] || [left | right] ] > | > <angle> > ,]? > <color-stop>[, <color-stop>]+ > ); This patch makes us: (a) skip the "to [edge-position]" expression-parsing code (valid for unprefixed & -moz prefixed, invalid for -webkit) (b) broadens a "not MozLegacy" check to use "not AnyLegacy" (since it's really checking if we're unprefixed) (c) broadens the -moz-linear-gradient "<legacy-gradient-line>" section to handle webkit-linear-gradient's simpler syntax as well. (The differences are: -webkit can only have an angle *OR* a position, not both; and -webkit only supports positions that are specified as "[top | bottom] || [left | right]", not 'center' & not arbitrary pixel values.) I added a bunch of valid & invalid testcases, too, which should hopefully serve as examples of the syntax we're trying to accept & reject here.

(Oops, I accidentally left in a stale XXXdholbert comment that was reminding me to do some refactoring that I've already done. I removed that stale comment in this updated version.)

Comment on attachment 8677257 [details] [diff] [review] part 7: refactor ParseLinearGradient a bit (no behavior change) Thanks for the reviews! Landed... ...part 7: https://hg.mozilla.org/integration/mozilla-inbound/rev/6008b75fe289 ...part 8: https://hg.mozilla.org/integration/mozilla-inbound/rev/88e2038fc5f1 Try run with those patches (a few reds which look infra/unrelated): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5221aa0136e

Pushing one final patch to remove some placeholder XXXdholbert comments in property_database.js file, which were for categories of syntax that I populated in other patches here: "part 9": https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc357be10ec With that, I think I'm good to call this bug FIXED. Hence, dropping "leave-open". (We'll likely run across some additional -webkit-linear-gradient/-webkit-radial-gradient syntax quirks that we'll need to implement -- e.g. I know of a few "advanced" radial syntax variants that I didn't cover here -- but we can do that in followups.)

https://hg.mozilla.org/mozilla-central/rev/6008b75fe289 https://hg.mozilla.org/mozilla-central/rev/88e2038fc5f1 https://hg.mozilla.org/mozilla-central/rev/2dc357be10ec

(In reply to Daniel Holbert [:dholbert] from comment #14) > Note: -webkit-linear-gradient & -webkit-radial-gradient were introduced in > this blog post: > https://www.webkit.org/blog/1424/css3-gradients/ > ...and I'm pretty sure they correspond closely to this spec revision (based > on the datestamp on that blog post & a bit of testing): > http://www.w3.org/TR/2011/WD-css3-images-20110217/#linear-gradients > http://www.w3.org/TR/2011/WD-css3-images-20110217/#radial-gradients Thanks, I'll likely use these as the basis for the relevant parts in the compat spec.

Documented these changes: https://developer.mozilla.org/en-US/docs/Web/CSS/linear-gradient https://developer.mozilla.org/en-US/docs/Web/CSS/radial-gradient https://developer.mozilla.org/en-US/docs/Web/CSS/repeating-linear-gradient https://developer.mozilla.org/en-US/docs/Web/CSS/repeating-radial-gradient https://developer.mozilla.org/en-US/Firefox/Releases/44 Daniel, can you please verify the doc changes? Sebastian

Bug 1132748 says, it was already implemented in Firefox 40, but I couldn't it to work there. Therefore I assume the real implementation was done here. Daniel, could you also please clarify that? Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #31) > Bug 1132748 says, it was already implemented in Firefox 40, but I couldn't > it to work there. Therefore I assume the real implementation was done here. > Daniel, could you also please clarify that? For documentation purposes, we should ignore the implementation in bug 1132748 -- that was done using the "CSS Unprefixing Service", which was a JS-implemented CSS-rewriting shim that known-to-be-hacky & was only enabled for a whitelist of sites. In contrast, this bug here actually gave us "native" support (implemented in C++), without any restriction to a whitelist of sites. (but preffed off until bug 1259345)

(In reply to Sebastian Zartner [:sebo] from comment #30) > https://developer.mozilla.org/en-US/docs/Web/CSS/repeating-linear-gradient It looks like you added an explicit {{CompatGeckoDesktop("44")}}{{property_prefix("-webkit")}} entry to the compat table under Firefox for this one, but not for the other ones. Maybe best to make that consistent, one way or another? (IIRC, for other CSS features, we've been mentioning -webkit support in a footnote but not explicitly listing it in the table, so maybe best to stick with that here.) > https://developer.mozilla.org/en-US/Firefox/Releases/44 I don't think we normally include preffed-off-by-default features in developer release notes, do we? Seems like that'd give the wrong impression that the feature is supported for users with Firefox 44, but really it's not. We already have this documented for Firefox 49 here: https://developer.mozilla.org/en-US/Firefox/Releases/49 so I think we should probably remove the Firefox 44 release note.

(In reply to Daniel Holbert [:dholbert] from comment #32) > (In reply to Sebastian Zartner [:sebo] from comment #31) > > Bug 1132748 says, it was already implemented in Firefox 40, but I couldn't > > it to work there. Therefore I assume the real implementation was done here. > > Daniel, could you also please clarify that? > > For documentation purposes, we should ignore the implementation in bug > 1132748 -- that was done using the "CSS Unprefixing Service", which was a > JS-implemented CSS-rewriting shim that known-to-be-hacky & was only enabled > for a whitelist of sites. > > In contrast, this bug here actually gave us "native" support (implemented in > C++), without any restriction to a whitelist of sites. (but preffed off > until bug 1259345) I see. Thank you for the explanation! (In reply to Daniel Holbert [:dholbert] from comment #33) > (In reply to Sebastian Zartner [:sebo] from comment #30) > > https://developer.mozilla.org/en-US/docs/Web/CSS/repeating-linear-gradient > > It looks like you added an explicit > {{CompatGeckoDesktop("44")}}{{property_prefix("-webkit")}} entry to the > compat table under Firefox for this one, but not for the other ones. Maybe > best to make that consistent, one way or another? (IIRC, for other CSS > features, we've been mentioning -webkit support in a footnote but not > explicitly listing it in the table, so maybe best to stick with that here.) You're right, better to keep things consistent. I've removed the entry again and added the note to the "Removed in 16 (16)" entry. I hope that won't confuse readers, but I'm asking the MDN team for opinions[1]. > > https://developer.mozilla.org/en-US/Firefox/Releases/44 > > I don't think we normally include preffed-off-by-default features in > developer release notes, do we? Seems like that'd give the wrong impression > that the feature is supported for users with Firefox 44, but really it's not. > > We already have this documented for Firefox 49 here: > https://developer.mozilla.org/en-US/Firefox/Releases/49 > so I think we should probably remove the Firefox 44 release note. As far as I can see, preffed-off-by-default features are often mentioned in the release notes, so I've also added them, but I've asked about that in the MDN mailing list[2], so we can get an official conclusion. In my opinion, as long as it is obviously stated that a feature is hidden behind a preference, I believe it's ok to mention it, so people can start playing with it, especially in regard of those release notes targeting web developers. But I agree that such features should not be mentioned in the normal Firefox release notes. Sebastian [1] https://groups.google.com/forum/#!topic/mozilla.dev.mdc/8pi123_SIMs [2] https://groups.google.com/forum/#!topic/mozilla.dev.mdc/ct5L63DV0mY

(In reply to Sebastian Zartner [:sebo] from comment #34) > (In reply to Daniel Holbert [:dholbert] from comment #33) > > I don't think we normally include preffed-off-by-default features in > > developer release notes, do we? ... > > ... I've asked about that in the MDN mailing list ... Ok, according to Eric "Sheppy" Sheppard[1] we did so historically, though it is planned to created "Experimental features in Firefox X" pages in the future. So, until the structure for those pages is clarified, I'll keep describing the features in the existing pages. Sebastian [1] https://groups.google.com/d/msg/mozilla.dev.mdc/ct5L63DV0mY/K7zEo2V-BAAJ