Closed Bug 798843 Opened 7 years ago Closed 6 years ago

change value names for -moz-objectFill to context-fill etc., and put them behind opentype SVG pref

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: heycam, Assigned: jfkthame)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(5 files, 8 obsolete files)

1.51 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
106.04 KB, patch
Details | Diff | Splinter Review
156.81 KB, patch
Details | Diff | Splinter Review
2.55 KB, patch
Details | Diff | Splinter Review
1.19 KB, patch
roc
: review+
Details | Diff | Splinter Review
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.)
Assignee: nobody → eflores
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?
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(dbaron)
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.)
Attached patch Rename non-style system bits (obsolete) — Splinter Review
Rename things not in style system to be consistent with change in name.
Attachment #718616 - Flags: review?(roc)
Attached patch Rename style system bits (obsolete) — Splinter Review
needinfo: Is this okay unprefixed?
Flags: needinfo?(cam)
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?
Flags: needinfo?(cam)
Attached patch Rename style system bits (obsolete) — Splinter Review
Attachment #718620 - Attachment is obsolete: true
Attachment #738304 - Flags: review?(dbaron)
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.
Attachment #738304 - Flags: review?(dbaron) → review-
Summary: change value names for -moz-objectFill to -moz-context-fill etc. → change value names for -moz-objectFill to context-fill etc., and put them behind opentype SVG pref
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
Attachment #738305 - Flags: review?(dbaron) → review+
And if you do (2), then you can use SpecialPowers.pushPrefEnv() to set and restore the pref automatically.
Attached patch Rename style system bits v2 (obsolete) — Splinter Review
Attachment #738304 - Attachment is obsolete: true
Attachment #742925 - Flags: review?(dbaron)
Carry over r=dbaron from comment 10
Attachment #738305 - Attachment is obsolete: true
Attachment #742927 - Flags: review+
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
Attachment #742925 - Flags: review?(dbaron) → review+
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
Attachment #742926 - Flags: review?(dbaron) → review+
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.
Edwin, could we get this fixed up and re-landed?
Flags: needinfo?(edwin)
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.
Attachment #803634 - Attachment is obsolete: true
Assignee: edwin → jfkthame
Depends on: 915715
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.
Attachment #803636 - Attachment is obsolete: true
Attachment #718616 - Attachment is obsolete: true
Attachment #742925 - Attachment is obsolete: true
Attachment #742927 - Attachment is obsolete: true
Target Milestone: --- → mozilla26
Oops, we missed this when updating the names.
Attachment #804475 - Flags: review?(roc)
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla26 → ---
Blocks: svg2
You need to log in before you can comment on or make changes to this bug.