At the recent SVG WG F2F, I showed a demo of the SVG-in-OpenType work and we discussed the syntax. Most people present thought the "object" in the new value names was not explanatory enough, and the consensus was the "context" would be a better word to use there. Also, there was no desire to continue along with the mixed-case value names following currentColor. Instead, names like "-moz-context-fill" were preferred. (These values still haven't been added to the spec yet.)

I've added context-fill and context-stroke to the SVG 2 spec (the other values e.g. for stroke-width not yet): https://svgwg.org/svg2-draft/painting.html#SpecifyingPaint I'd like get these values working inside <marker>s for cases like the example I've got in this section: https://svgwg.org/svg2-draft/painting.html#VertexMarkerProperties So I think we should unprefix these two while renaming them. David, given the SVG WG resolved on these new names, do you think it is acceptable to have them unprefixed and not behind a pref?

I'm ok with unprefixing if (1) our implementation matches what the spec says (e.g., what to do if there's no context element), (2) you and the WG are comfortable saying those spec details (in addition to just the name) are solid and (3) the WG (probably both SVG and CSS, actually) is aware you're doing this.

I will go back to the WG and see how comfortable they are with this. (I'll bring it up at the F2F which is in a couple of weeks.)

Rename things not in style system to be consistent with change in name.

needinfo: Is this okay unprefixed?

Finally talked about this in the telcon today. People weren't happy with having it unprefixed and generally available. Can we put these names unprefixed but behind the gfx.font_rendering.opentype_svg.enabled pref for now?

Comment on attachment 738304 [details] [diff] [review] Rename style system bits please fix the indentation in nsStyleConsts.h Is that .woff file generated in some sensible way? It would be great to document or script it if possible. In CSSParserImpl, please cache the preferences somewhere rather than calling Preferences::GetBool every time. The performance of GetBool is unacceptable in this context, but Preferences offers some nice APIs for caching. And instead of scattering additional code all over the place in the CSS parser, I think it would be much simpler to add an additional VARIANT_SVG_OPENTYPE_KEYWORD that concentrates the check in a single place and doesn't require the new code in so many places (or so many changes from CSS_PROPERTY_PARSE_VALUE to PARSE_FUNCTION). This will also allow removing all occurrences of the non-word "Contextable" (I think; if not, it needs a better name). review- since I'd like to look at the parser/proplist changes again, but otherwise this looks fine.

Comment on attachment 738305 [details] [diff] [review] Test that context-* values can be pref'ed off >+<script> >+ >+var attrs = { >+ "fill" : "context-stroke none", >+ "stroke" : "context-fill none", >+ "fillOpacity" : "context-stroke-opacity", >+ "strokeOpacity" : "context-fill-opacity", >+ "strokeDasharray" : "context-value", >+ "strokeDashoffset" : "context-value", >+ "strokeWidth" : "context-value" >+}; Please call this props instead of attrs. >+SpecialPowers.setBoolPref("gfx.font_rendering.opentype_svg.enabled", true); You should either: (1) assert before you do this that the current value of the preference is false, because otherwise the test would leave the pref in a different state from what it started in, or (2) get the value of the pref before this, and restore it at the end of the test (probably preferable) r=dbaron with that

And if you do (2), then you can use SpecialPowers.pushPrefEnv() to set and restore the pref automatically.

Carry over r=dbaron from comment 10

Comment on attachment 742925 [details] [diff] [review] Rename style system bits v2 Please fix the indentation in nsStyleConts.h. (I said that last time.) The numbers should line up with the numbers surrounding them. >+ static bool sOpentypeSVGEnabledCached = false; >+ if (!sOpentypeSVGEnabledCached) { >+ Preferences::AddBoolVarCache(&sOpentypeSVGEnabled, >+ "gfx.font_rendering.opentype_svg.enabled"); >+ } You need to set sOpentypeSVGEnabledCached to true in here. >@@ -5020,17 +5023,19 @@ CSSParserImpl::ParseVariant(nsCSSValue&· > tk->mIdent.LowerCaseEqualsLiteral("rgba") || > tk->mIdent.LowerCaseEqualsLiteral("hsla")))) > { > // Put token back so that parse color can get it > UngetToken(); > if (ParseColor(aValue)) { > return true; > } >- return false; >+ // ParseColor ungets token if it fails. Undo that here to get back to our >+ // previous state. >+ GetToken(true); > } > } This isn't always true; ParseColor might consume an entire rgb() function with a syntax error inside, and return true. Instead, you should: (1) undo this change (2) not make a recursive call, but instead just detect VARIANT_OPENTYPE_SVG_KEYWORD prior to any VARIANT_KEYWORD handling, check the pref, and if the pref is set, aVariantMask |= VARIANT_KEYWORD. (that is, move your current VARIANT_OPENTYPE_SVG_KEYWORD code earlier and change it) r=dbaron with those changes

Comment on attachment 742926 [details] [diff] [review] README for SVG in OpenType fonts used in reftests >+nosvg.woff >+---------- >+This font is Liberation Serif with the addition of a glyph with a UVS selector >+(Liberation fonts do not come with UVS glyphs; one had to be added to test that >+SVG glyphs work with UVS selectors). It contains no 'SVG ' table. >+ >+rubbish.woff >+------------ >+This font is the same as nosvg.woff above, but with the addition of an 'SVG ' >+table with the contents of rubbish.txt. Its purpose is to test that SVG tables >+without valid XML are ignored. If you can, maybe say how you made these? >+svg.woff >+-------- >+This font is the same as nosvg.woff above, but with the glyphs-*.svg SVG >+documents from this directory embedded in it using the tools described below. >+ >+Creating the Fonts >+------------------ >+The tools used here are insertsvg.py from [1] and sfnt2woff from [2]. >+ >+svg.woff can be recreated with: >+ >+woff2sfnt nosvg.woff > nosvg.ttf >+insertsvg.py nosvg.ttf svg.ttf glyphs-*.svg >+sfnt2woff svg.ttf >+ >+[1] https://github.com/edf825/SVG-OpenType-Utils >+[2] http://people.mozilla.com/~jkew/woff/ Seems like you can drop the "Creating the Fonts" heading since this is really all a description of svg.woff. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/81d152795bea https://hg.mozilla.org/integration/mozilla-inbound/rev/4028158373fe https://hg.mozilla.org/integration/mozilla-inbound/rev/b4017fe1e7f5 https://hg.mozilla.org/integration/mozilla-inbound/rev/4d755ea39a56

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/747173e387c9 for https://tbpl.mozilla.org/php/getParsedLog.php?id=23011391&tree=Mozilla-Inbound&full=1 - test_value_cloning and test_value_computation and test_value_storage would like some attention from you, too.

Also https://tbpl.mozilla.org/php/getParsedLog.php?id=23013745&tree=Mozilla-Inbound - your test was busted, in b2g mochitest-9.

Edwin, could we get this fixed up and re-landed?

This is Edwin's patch from above, just rebased to apply on current trunk. Carrying forward r=roc.

Second patch, rebased to apply on current trunk and adding the missing changes to property_database.js so as to keep mochitest happy. I believe this will resolve the test failures that happened when this was landed previously. Carrying forward r=dbaron.

And finally the patch that adds the new test here, rebased to apply on current trunk. Carrying forward r=dbaron.

I've taken a stab at rebasing/updating the patches here. AFAICS, the main reason they bounced last time was that property_database.js was not updated for the changed property names; this accounts for the various mochitest-5 failures. So I've added this into the second patch. Tryserver run with the updated patches, to see if we're clear to land: https://tbpl.mozilla.org/?tree=Try&rev=6e5b40ef4bc9

Close, but not quite: 85105 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_value_computation.html | should get initial value for 'stroke-dashoffset:context-value' - got 0, expected 0px 85106 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_value_computation.html | should get initial value for 'stroke-dashoffset:context-value' - got 0, expected 0px I believe this is a minor bug in the update to nsRuleNode::ComputeSVGData(); fix coming.

Updated patch 2: mStrokeDashoffset is supposed to be a Coord, not an Int. Carrying forward r=dbaron.

Once more, fingers crossed: https://tbpl.mozilla.org/?tree=Try&rev=8f12d9d7d299

That's made test_value_computation.html happy, but there's also a problem on B2G: 6126 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug798843_pref.html | fill not settable to context-stroke none - got context-stroke none, expected 6127 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug798843_pref.html | stroke not settable to context-fill none - got context-fill none, expected 6128 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug798843_pref.html | fillOpacity not settable to context-stroke-opacity - got context-stroke-opacity, expected 6129 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug798843_pref.html | strokeOpacity not settable to context-fill-opacity - got context-fill-opacity, expected 6130 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug798843_pref.html | strokeDasharray not settable to context-value - got context-value, expected 6131 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug798843_pref.html | strokeDashoffset not settable to context-value - got context-value, expected 6132 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug798843_pref.html | strokeWidth not settable to context-value - got context-value, expected It appears that the use of SpecialPowers to dynamically toggle the preference does not work there, and so the tests for the non-default setting (disabled) all fail. I've filed bug 915715 on this, and propose to simply skip this test on B2G for now. FxOS users aren't going to be dynamically toggling this anyway; AFAIK there isn't any UI provided for them to do so.

How about using SpecialPowers.pushPrefEnv instead? Does that fix it? It should be simpler in any case as you wouldn't need to remember the original value.

That sounds like the right solution. Tryserver run using SpecialPowers: https://tbpl.mozilla.org/?tree=Try&rev=60c0ba5c3de1 If this passes on b2g, I'll post an updated version of the test patch and we should be good to go.

Tryserver confirms the SpecialPowers.pushPrefEnv version works correctly on b2g (as well as on desktop), so let's use that. Carrying forward r=dbaron, as the actual test here remains the same as the previously-reviewed version, it's just being run in a b2g-friendlier way.

And re-landed the rebased/fixed-up patches, hoping it all sticks this time: https://hg.mozilla.org/integration/mozilla-inbound/rev/6c097bdfc079 https://hg.mozilla.org/integration/mozilla-inbound/rev/51f5d900cd27 https://hg.mozilla.org/integration/mozilla-inbound/rev/9fdf99c07466 https://hg.mozilla.org/integration/mozilla-inbound/rev/b322938b37ef

https://hg.mozilla.org/mozilla-central/rev/6c097bdfc079 https://hg.mozilla.org/mozilla-central/rev/51f5d900cd27 https://hg.mozilla.org/mozilla-central/rev/9fdf99c07466 https://hg.mozilla.org/mozilla-central/rev/b322938b37ef

Oops, we missed this when updating the names.

Backed out for frequent Windows test failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/c88b666cff54 https://tbpl.mozilla.org/php/getParsedLog.php?id=27814809&tree=Mozilla-Inbound

And relanded, with SimpleTest.waitForExplicitFinish()/finish() added to ensure the tests actually get a chance to run. https://hg.mozilla.org/integration/mozilla-inbound/rev/13743a1f2db3 https://hg.mozilla.org/integration/mozilla-inbound/rev/fa8322c03573 https://hg.mozilla.org/integration/mozilla-inbound/rev/4f410931c167 https://hg.mozilla.org/integration/mozilla-inbound/rev/41c637af7de9

https://hg.mozilla.org/mozilla-central/rev/13743a1f2db3 https://hg.mozilla.org/mozilla-central/rev/fa8322c03573 https://hg.mozilla.org/mozilla-central/rev/4f410931c167 https://hg.mozilla.org/mozilla-central/rev/41c637af7de9

Pushed the followup CSS fix (comment 34): https://hg.mozilla.org/integration/mozilla-inbound/rev/0040a5dd7c62