Closed
Bug 798843
Opened 12 years ago
Closed 11 years ago
change value names for -moz-objectFill to context-fill etc., and put them behind opentype SVG pref
Categories
(Core :: SVG, defect)
Core
SVG
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
Reporter | ||
Comment 1•12 years ago
|
||
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)
Reporter | ||
Comment 3•12 years ago
|
||
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.
Attachment #718616 -
Flags: review?(roc)
needinfo: Is this okay unprefixed?
Flags: needinfo?(cam)
Attachment #718616 -
Flags: review?(roc) → review+
Blocks: 832483
Reporter | ||
Comment 6•12 years ago
|
||
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)
Attachment #718620 -
Attachment is obsolete: true
Attachment #738304 -
Flags: review?(dbaron)
Attachment #738305 -
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+
Reporter | ||
Comment 11•12 years ago
|
||
And if you do (2), then you can use SpecialPowers.pushPrefEnv() to set and restore the pref automatically.
Attachment #738304 -
Attachment is obsolete: true
Attachment #742925 -
Flags: review?(dbaron)
Attachment #742926 -
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+
Comment 18•12 years ago
|
||
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.
Comment 19•12 years ago
|
||
Also https://tbpl.mozilla.org/php/getParsedLog.php?id=23013745&tree=Mozilla-Inbound - your test was busted, in b2g mochitest-9.
Reporter | ||
Comment 20•11 years ago
|
||
Edwin, could we get this fixed up and re-landed?
Flags: needinfo?(edwin)
Assignee | ||
Comment 21•11 years ago
|
||
This is Edwin's patch from above, just rebased to apply on current trunk. Carrying forward r=roc.
Assignee | ||
Comment 22•11 years ago
|
||
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.
Assignee | ||
Comment 23•11 years ago
|
||
And finally the patch that adds the new test here, rebased to apply on current trunk. Carrying forward r=dbaron.
Assignee | ||
Comment 24•11 years ago
|
||
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
Assignee | ||
Comment 25•11 years ago
|
||
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.
Assignee | ||
Comment 26•11 years ago
|
||
Updated patch 2: mStrokeDashoffset is supposed to be a Coord, not an Int. Carrying forward r=dbaron.
Assignee | ||
Updated•11 years ago
|
Attachment #803634 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Assignee: edwin → jfkthame
Assignee | ||
Comment 27•11 years ago
|
||
Once more, fingers crossed: https://tbpl.mozilla.org/?tree=Try&rev=8f12d9d7d299
Assignee | ||
Comment 28•11 years ago
|
||
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.
Comment 29•11 years ago
|
||
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.
Assignee | ||
Comment 30•11 years ago
|
||
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.
Assignee | ||
Comment 31•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Attachment #803636 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #718616 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #742925 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #742927 -
Attachment is obsolete: true
Assignee | ||
Comment 32•11 years ago
|
||
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
Flags: needinfo?(edwin)
Assignee | ||
Updated•11 years ago
|
Target Milestone: --- → mozilla26
Comment 33•11 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 34•11 years ago
|
||
Oops, we missed this when updating the names.
Attachment #804475 -
Flags: review?(roc)
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 35•11 years ago
|
||
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 → ---
Assignee | ||
Comment 36•11 years ago
|
||
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
Attachment #804475 -
Flags: review?(roc) → review+
Comment 37•11 years ago
|
||
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
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Assignee | ||
Comment 38•11 years ago
|
||
Pushed the followup CSS fix (comment 34):
https://hg.mozilla.org/integration/mozilla-inbound/rev/0040a5dd7c62
Comment 39•11 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•