1318208 - Implement SVG vector-effect: non-scaling-size & non-rotation & fixed-position

Status: NEW

(Core :: SVG, enhancement, P3)

Comment 1

[Teiichiro Fukuda]

SVG2 has a vector-effect property which allows a non-scaling-size.
I will add a vector effect property(non-scaling-size).

https://svgwg.org/svg2-draft/coords.html#VectorEffects

Comment 2

[Teiichiro Fukuda]

Attachment: svg_nonScalingSize.patch

I added the 'non-scaling-size' property to the CSS parser.

Comment 3

[Teiichiro Fukuda]

Attachment: svg_nonScalingSize_r1.patch

Hi Robert,
Please tell me whether the way of patch implementation is correct.

Comment 4

[Teiichiro Fukuda]

Attachment: svg_circle.html

Comment 5

[Robert Longson [:longsonr]]

Note that this feature is labelled as at-risk. https://svgwg.org/svg2-draft/Overview.html

You should probably break this up into a CSS part and a layout part and get a CSS peer to review the CSS part (dbaron or cam perhaps).

I could review the SVG layout code.

You need tests, tests of the CSS parsing (there's a property_database.js) for layout you need reftests.

Since we're using the transform to outer svg for something other than non-scaling-stroke you should rename it i.e. aToNonScalingStrokeSpace should become aToOuterSVGSpace

The patch does not seem to allow for non-scaling-stroke and non-scaling-size to be set together.

Comment 6

[Teiichiro Fukuda]

hi Robert
first we wanted to make sure if we are modifying the right place.
if it is the case, we can go on with further coding. if not, please let us know. 

> Note that this feature is labelled as at-risk.
> https://svgwg.org/svg2-draft/Overview.html
yes we are aware of that.

> You should probably break this up into a CSS part and a layout part and get
> a CSS peer to review the CSS part (dbaron or cam perhaps).
We will separate this into two patches. 
please kindly carry on with reviewing SVG part.

> You need tests, tests of the CSS parsing (there's a property_database.js)
> for layout you need reftests.
who is the right person in case we need some help in CSS parsing tests?

> Since we're using the transform to outer svg for something other than
> non-scaling-stroke you should rename it i.e. aToNonScalingStrokeSpace should
> become aToOuterSVGSpace
as for variable namings we will correct it i our next patch.

> The patch does not seem to allow for non-scaling-stroke and non-scaling-size
> to be set together.
our upcoming code will include combination of different vector effects.

Comment 7

[Robert Longson [:longsonr]]

If you want help with the CSS tests I'd probably try Cameron McCormack.

I suggest you put this feature behind a pref too so we can decide when and if to enable it.

Comment 8

[Teiichiro Fukuda]

Attachment: nonScalingSize_css.patch

Comment 9

[Teiichiro Fukuda]

Attachment: nonScalingSize_css.patch

Comment 10

[Teiichiro Fukuda]

hi David
If you want to specify more than one property for the property of vector-effect as below, what should be changed?

An example：
vector-effect="non-scaling-size non-rotation"

Comment 11

[Teiichiro Fukuda]

Attachment: nonScalingSize_svg.patch

Comment 12

[Robert Longson [:longsonr]]

Comment on attachment 8820140 [details] [diff] [review]
nonScalingSize_svg.patch

please add reftests into the patch, test patterns and having both fill/stroke effects together. Adjust code to ensure tests pass.

Also ignore the superreview box, just ask for reviews.

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8818158 [details] [diff] [review]
nonScalingSize_css.patch

Please use the review flag rather than the superreview flag in the future.

When posting patches for review, please include the correct hg metadata (including author and commit message) in the patch, so they can be reviewed.

Also, please send an intent to ship as described in https://wiki.mozilla.org/WebAPI/ExposureGuidelines


As you noted in your needinfo? request, you also need to fix the parsing of the 'vector-effect' property.  This requires roughly (modeling what needs doing after the text-decoration-line or font-variant-numeric properties, which are similar):
 * in nsCSSPropList.h, add the flag CSS_PROPERTY_VALUE_PARSER_FUNCTION
 * adding a parsing function in nsCSSParser.cpp (see ParseTextDecorationLine for  an example)
 * adjusting the code in nsCSSValue::AppendToString to call AppendBitmaskCSSValue for this property (like for the ones I mentioned)
 * adjusting nsComputedDOMStyle::DoGetVectorEffect to use AppendBitmaskCSSValue as appropriate (see DoGetTextDecorationLine)
 * adding a test of the combination of the two values in property_database.js

I'd also suggest writing the constants in nsStyleConsts.h as (1<<0), (1<<1), etc., and the mask as ((1<<2)-1)

Comment 14

[Teiichiro Fukuda]

Attachment: WIP:bug-1318208-part1

Comment 15

[Teiichiro Fukuda]

Attachment: WIP:bug-1318208-part2

Comment 16

[Teiichiro Fukuda]

Attachment: VectorEffect_so_non-scaling-size.html

Comment 17

[Teiichiro Fukuda]

[Tracking Request]: Afterimages appear

I tried to implement Vector-Effect:non-scaling-size.
Now, I had modified TransformMatrix to cancel sacale factor.
For checking this modification, I made it possible to change the scale according to keyboard input with JavaScript.
When I push key repeatedly, an unintended afterimage will appear.

Steps To Reproduce:
1. Open attached html file. (vector-effect-non-scaling-size.html)
2. Select the Zoom slider and change scale size.
3. Please press direction key(left or right) on the keyboard.
  -> zoom size will be changed.
4. Press key repeatedly.

Actual Results:
  key hit repeatedly
  slow : no afterimage
  fast : afterimage will appear.

Expected Results:
  slow or fast : no afterimage appear.

Comment 18

[Teiichiro Fukuda]

Attachment: VectorEffectRenderingIssue.mp4

The reproduced environment is as follows.

・Windows10(64bit)
・Nightly 53.0a1
・CPU:Core i7 4770
・RAM 16GB

Comment 19

[Robert Longson [:longsonr]]

(In reply to Teiichiro Fukuda from comment #17)
> [Tracking Request]: Afterimages appear
> 
> I tried to implement Vector-Effect:non-scaling-size.
> Now, I had modified TransformMatrix to cancel sacale factor.
> For checking this modification, I made it possible to change the scale
> according to keyboard input with JavaScript.
> When I push key repeatedly, an unintended afterimage will appear.
> 
> Steps To Reproduce:
> 1. Open attached html file. (vector-effect-non-scaling-size.html)
> 2. Select the Zoom slider and change scale size.
> 3. Please press direction key(left or right) on the keyboard.
>   -> zoom size will be changed.
> 4. Press key repeatedly.
> 
> Actual Results:
>   key hit repeatedly
>   slow : no afterimage
>   fast : afterimage will appear.
> 
> Expected Results:
>   slow or fast : no afterimage appear.

Presumably you've got something wrong with the calculation of bounding boxes. If the region occupied by a shape is incorrect then when it moves gecko will not redraw the place it used to occupy correctly.

Comment 20

[Teiichiro Fukuda]

Attachment: WIP:bug-1318208-part2

Comment 21

[Teiichiro Fukuda]

(In reply to Robert Longson from comment #19)
> (In reply to Teiichiro Fukuda from comment #17)
> > [Tracking Request]: Afterimages appear
> > 
> > I tried to implement Vector-Effect:non-scaling-size.
> > Now, I had modified TransformMatrix to cancel sacale factor.
> > For checking this modification, I made it possible to change the scale
> > according to keyboard input with JavaScript.
> > When I push key repeatedly, an unintended afterimage will appear.
> > 
> > Steps To Reproduce:
> > 1. Open attached html file. (vector-effect-non-scaling-size.html)
> > 2. Select the Zoom slider and change scale size.
> > 3. Please press direction key(left or right) on the keyboard.
> >   -> zoom size will be changed.
> > 4. Press key repeatedly.
> > 
> > Actual Results:
> >   key hit repeatedly
> >   slow : no afterimage
> >   fast : afterimage will appear.
> > 
> > Expected Results:
> >   slow or fast : no afterimage appear.
> 
> Presumably you've got something wrong with the calculation of bounding
> boxes. If the region occupied by a shape is incorrect then when it moves
> gecko will not redraw the place it used to occupy correctly.

I have modified calculation of bounding boxes.
The image is no longer chipped, but afterimage is still displayed.

Comment 22

[Sanggyu SON]

Attachment: layout-reftests-svg.zip

Comment 23

[Sanggyu SON]

Attachment: WIP:bug-1318208-part3

I made Reftest for VectorEffect:non-sacling-size.
layout-reftests-svg.zip includes six svg files.

Is this the contents of the reftest file OK?

Comment 24

[ramin]

Hi Robort
It's been a while since your last comment and I'm writing this because our implementor team (SON-san, Fukuda-san) are desperately waiting for some info regarding "refresh" issues after adding vector effects. 
So far we're looking through the SVG/dom code but we've no clue why this issue 
is happening, and if it has really anything to do with coordinate miscalculations.
Would you kindly take a look into attached code and give us a hint which potentially may cause
this issue? I appreciate your time and cooperation in advance. 
Thank you

Comment 25

[Sanggyu SON]

Attachment: WIP:bug-1318208-part2

Fixed a problem of afterimages appearing when Vector effect:non-scaling-size was applied.
In the previous patch, I modified the context transformation matrix directly.
The correct way is to rewrite path information before drawing.
This patch will not display afterimages.

Comment 26

[Robert Longson [:longsonr]]

I'm a volunteer and I don't always have a lot of time. If you're looking for faster feedback jwatt or dholbert might be better.

Comment 27

[Robert Longson [:longsonr]]

I've looked at the reftests now:

a) you need to include them as part of the patch
b) you need to create additional reftests that have both non-scaling-size and non-scaling-stroke set
c) you need to create additional reftests that have items with non-scaling-size/stroke in a pattern (see the reftests in bug 528332).
d) you need to create additional reftests that have items with non-scaling-size/stroke that have gradients to ensure that the gradients display as expected.
e) you need to create additional reftests using use to point at a non-scaling-size/stroke object.

bug 528332 has reftests that do c, d and e for non-scaling-stroke only. Multiple things are done in the same reftest there. Copying and then adapting those reftests would be a good place to start.

Comment 28

[Sanggyu SON]

(In reply to Robert Longson from comment #27)
> I've looked at the reftests now:
> 
> a) you need to include them as part of the patch
> b) you need to create additional reftests that have both non-scaling-size
> and non-scaling-stroke set
> c) you need to create additional reftests that have items with
> non-scaling-size/stroke in a pattern (see the reftests in bug 528332).
> d) you need to create additional reftests that have items with
> non-scaling-size/stroke that have gradients to ensure that the gradients
> display as expected.
> e) you need to create additional reftests using use to point at a
> non-scaling-size/stroke object.
> 
> bug 528332 has reftests that do c, d and e for non-scaling-stroke only.
> Multiple things are done in the same reftest there. Copying and then
> adapting those reftests would be a good place to start.

Thank you for your advice.
I will try to create reftests by referring to them.

Comment 29

[Sanggyu SON]

Attachment: bug-1318208-part1

I had modified Vectoreffect:non-scaling-size CSS parser.

Comment 30

[Sanggyu SON]

Attachment: bug-1318208-part2

I had modified transform calculation for non-scaling-size and added to apply [screen|viewport] property.

Comment 31

[Sanggyu SON]

Attachment: bug-1318208-part3

I had modified reftests and created patch.

Comment 32

[Sanggyu SON]

I have modified VectorEffect:non-scaling-size calculation, and added property [screen|viewport].
These reftest are executed under Windows10x64 and Ubuntu14.04x64 environment.

Comment 33

[Sanggyu SON]

Now,other vector-effects were also implemented. 
So, I want to change the current bug title.
I'd like to resolve vector-effect that includes non-scaling-sized, non-rotation and fixed-position with this same bug number.

I will upload some patches for non-rotation and fixed-position soon.

Comment 34

[Sanggyu SON]

Attachment: bug-1318208-part4

Comment 35

[Sanggyu SON]

Attachment: bug-1318208-part5

This patch added Preference about:config svg2.vector-effect.enabled keyword.
On release or beta configuration this key is set to false.

Added CSS entry for Vector-effect: non-rotation and fixed-position.

Comment 36

[Sanggyu SON]

Attachment: bug-1318208-part6

Vector Effect: non-rotation and fixed-position effect code is added.

Comment 37

[Sanggyu SON]

Attachment: bug-1318208-part7

Reftest for non-rotation and fixed-position is added.
Non-scaling-size, non-rotation, fixed-position and it's combination test is also added.

Comment 38

[Sanggyu SON]

patch information
1.bug-1318208-part1 : non-scaling-size CSS added.
2.bug-1318208-part2 : non-scaling-size effect code added.
3.bug-1318208-part3 : non-scaling-size Reftests.
4.bug-1318208-part4 : non-scaling-size 
  Preference about:config svg.vector-effect-non-scaling-size.enabled added.
5.bug-1318208-part5 : Preference added.
  Preference about:config svg2.vector-effect.enabled added for all vector-effect.
  CSS for non-rotation and fixed-position added.
6.bug-1318208-part6 : 
  Both vector-effect:non-rotation and fixed position effects are added.
7.bug-1318208-part1 : non-scaling-size CSS added.
  Reftests for non-rotation, fixed-position and non-scaling-size and it's combinations are added.

I had tested these code and reftests on Windows10 x64 environment.

Comment 39

[Daniel Holbert [:dholbert]]

Comment on attachment 8830590 [details] [diff] [review]
bug-1318208-part4

Review of attachment 8830590 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +15846,5 @@
>          int32_t index;
> +        int32_t prefCount=2;        
> +        bool svg2ve = false;
> +        if (Preferences::GetBool("svg.vector-effect.non-scaling-size.enabled", false)) {
> +          prefCount = 4;        

A few things to fix here:
 (1) You've got trailing space at the end of some lines here (after "2" and "4"). And you're missing spaces around "=2".  (But don't bother fixing that, since I think these lines should really get removed, per my note on prefCount below.)

 (2) Avoid unexplained magic numbers like "2" and "4".  If you really need to use these raw numbers, please add a code-comment explaining what they represent and why 2 or 4 is the correct value.  (But in this case, I think you should probably remove them, per my next comment about prefCount).

 (3) The variable "prefCount" seems to be misnamed. It's not actually a count of preferences. It really seems to control how many consecutive keywords we'll parse for this property, I think?  And even if it were correctly named (e.g. "numKeywordsToAccept"), it's not clear to me that it's a good strategy for how to enable/disable this *particular* keyword, since generally, CSS lets you provide keywords in any order.

 (4) Preferences::GetBool is not super-cheap, so we'd prefer not to invoke it every time we parse this CSS property (which might be manymany times per pageload).  Please use Preferences::AddBoolVarCache -- that'll let you register a static bool variable which gets updated any time the corresponding pref is modified (but otherwise just acts as a read-only "proxy" for the pref's value).  You should follow the strategy in this changeset:
 https://hg.mozilla.org/mozilla-central/rev/464952b30298
In particular, look at (and match) the usage of AddBoolVarCache and the variable "sControlCharVisibility" there.  (You'll want to add a new static variable, like sControlCharVisibility, maybe called something like "sSVGNonScalingSizeEnabled". And you'll want to directly check that bool here or wherever your code needs to test for this feature being enabled/disabled.)

 (5) This patch might just want to be folded into your "part 1" patch here (where you're adding the code that you're modifying here).  When you're adding a pref-controlled feature, it's kind of nice (for review purposes and for sanity-checking purposes) to have the pref-checking logic baked in from the start, rather than being added as an afterthought. That makes it clearer *how* the preference actually controls the feature.

@@ +15865,5 @@
>              break;
>            }
>          }
> +        if (!svg2ve) {
> +          intValue &= 0x00000001;

As above:
 (1) As above, please avoid raw magic numbers, or please explain what they mean.

 (2) It looks like the intent here is to disable non-scaling-size support by simply suppressing its bit in the parsed value, or something like that...  But that's not really an effective way to disable the feature.  This will mean we don't *render* non-scaling-size (I assume), but we'll still accept it in the CSS parser -- we'll behave as if we support it, from the web developers's perspective.  When the pref is disabled, we really need to act as if the keyword isn't supported at all.  I'm not immediately sure what the most effective way is to do that -- perhaps we should add a special case inside of the ParseEnum block above this (kind of like the existing check for newValue == NS_STYLE_VECTOR_EFFECT_NONE), and return false [reject the CSS] if we've just parsed non-scaling-size and the pref is disabled?

::: modules/libpref/init/all.js
@@ +3548,5 @@
>  
>  // We cannot retrieve active IME name with IMM32 API if a TIP of TSF is active.
>  // This pref can specify active IME name when Japanese TIP is active.
>  // For example:
> +//   Google Japanese Input: "Google åõ¥æ×æëåØå$¥å³û IMM32 ã¢ã£ìáã¥ã¥îáã«¢

This fixup seems unrelated to this patch, and I'm not sure if a useful change or not.  Based on bugzilla's diff view, it seems to be removing some unprintable-in-my-browser UTF boxes, and replacing them with some special (but non-Japanese) characters like "õ".

This might've just been your text editor being "helpful" without your knowledge. :) Sometimes text editors do weird things behind our back.

But regardless, this doesn't belong in this patch -- please revert it, and if it's a change that you actually intended to make, then please split it out into its own patch/bug with a clearer explanation of what the change is.

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 8830591 [details] [diff] [review]
bug-1318208-part5

Review of attachment 8830591 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +15852,3 @@
>          bool svg2ve = false;
> +        if (Preferences::GetBool("svg2.vector-effect.enabled", false)) {
> +          prefCount = 8;

Wait, so your previous patch made us use one new pref here, and then this patch makes us use a different new pref instead...

We should probably just use a single consistent new pref here.  (Or if we need per-keyword prefs, let's consistently use per-keyword prefs. One way or the other, but let's not add code just to remove it in a later patch if we can avoid it.)  So, please sort this about:config pref stuff out (probably with AddBoolVarCache as noted above) and get it the way you want it, up-front, in whichever patch adds it.)

Also, I think the code that you're replacing here was the only usage of your new pref "svg.vector-effect.non-scaling-size.enabled".  So as of this patch, that pref is left orphaned in all.js with no usages in the codebase.

Also, my above notes about magic numbers and prefCount still apply at this point.

::: layout/style/nsStyleConsts.h
@@ +1165,5 @@
> +#define NS_STYLE_VECTOR_EFFECT_NON_ROTATION         (1 << 2)
> +#define NS_STYLE_VECTOR_EFFECT_FIXED_POSITION       (1 << 3)
> +#define NS_STYLE_VECTOR_EFFECT_VIEW_PORT            (1 << 4)
> +#define NS_STYLE_VECTOR_EFFECT_SCREEN               (1 << 5)
> +#define NS_STYLE_VECTOR_EFFECT_MASK                 ((1 << 6) -1)

Add a space after "-" here.

(Gecko coding style uses spaces around operations like addition/subtraction/etc.  If this were representing the quantity "negative 1", then -1 would be the correct spelling (with no space).  But that's not what this is -- this is the operation "subtract 1".)

::: modules/libpref/init/all.js
@@ +2923,5 @@
>  
> +#ifdef RELEASE_OR_BETA
> +pref("svg2.vector-effect.enabled", false);
> +#else
> +pref("svg2.vector-effect.enabled", true);

A few nits on this pref name:
 (1) Let's drop the "2" from this pref name.  Just call it "svg.[whatever]", not "svg2".  Our existing SVG prefs don't have a version number, so this one shouldn't either.

 (2) Based on the pref name, it sounds like this controls whether the vector-effect CSS property is enabled or not. BUT, really, it only controls certain values of that property (I think?).  So, perhaps it should be named something like "svg.vector-effect.extra-values.enabled", and you should add a comment alongside it listing the values that it controls.

@@ +3554,5 @@
>  
>  // We cannot retrieve active IME name with IMM32 API if a TIP of TSF is active.
>  // This pref can specify active IME name when Japanese TIP is active.
>  // For example:
> +//   Google Japanese Input: "Google æ¥æ¬èªå¥å IMM32 ã¢ã¸ã¥ã¼ã«"

Here, it looks like you're reverting the change from the previous patch. Please just revert this in both patches.

(Though really, as noted above, maybe this patch and the previous patch just want to be folded together, since this patch is already replacing most of the code added in the previous patch.)

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 8827795 [details] [diff] [review]
bug-1318208-part2

Review of attachment 8827795 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/SVGGeometryFrame.cpp
@@ +897,5 @@
> +        (nVectorEffectCombination == eNonScalingStrokeAndSize)) {
> +
> +      //  screen or viewport properties.
> +      //  Default property is viewport. If Both property is setted, it
> +      //  will set to viewport property.

Drive-by review notes on this patch, since I encountered this chunk when looking at a later patch (part6, which reindents this whole section -- I started writing review feedback for that patch and then realized it was really for stuff that was added in this patch & simply reintented in that later patch.)

This comment and the logic that it's describing have some issues:

 (1) The terminology isn't quite right here -- "screen" and "viewport" are not "properties".  There is only one *property* here, and that's the "vector-effect" property.  screen/viewport are "keywords" (or "values") that the vector-effect property can accept.
So, I think the first line means to say something like:
  // Check for the vector-effect keywords "screen" vs. "viewport".
This actually applies to every usage of the word "property" that this patch is adding in this file -- all of those usages really mean to say "value" or "keyword", *not* "property".

 (2) Regarding the phrase "If Both property is setted" -- even with the word "property" corrected to "value", this line makes no sense, because it should be *impossible* for both of these values to be set at the same time.  The CSS parser should reject any input that attempts to set both of them.  The spec says that these are alternatives, and exactly one of them must be provided -- it has the following markup:
   [ viewport | screen ]?       (source: https://www.w3.org/TR/SVG2/coords.html#VectorEffects )
The vertical-bar there means "exactly one of these alternatives".

Since there are only two possibilities for this part (with "viewport" being the default behavior when neither is provided), we probably want to encode these values as a single bit in our vector-effect bitmask representation.  So e.g. the bit would be *set* if "screen" was provided, and otherwise it would be unset.  That will simplify the logic a good deal, and it'll mean you don't need comments like this about edge cases that should really be impossible.

 (3) This is a higher-level comment: if at all possible, please structure your changes in this bug such that you don't have later patches (like part6) reindenting+rewriting huge amounts of code that you added in earlier patches (like this part).  This will make your life much easier when you make changes to earlier patches.  Right now, if you change a line here, your later patches will fail to apply and will need to be carefully fixed by hand so that they continue to apply (and so that they carry the corrected change forward, to the extent that the fixed code still exists after the later patch).

I suspect some of part 6's logic might really want to be folded into this patch...  Or if you like, maybe all of this rendering code can just be added in one patch towards the end of the series, rather than being added and then rewritten in later patches in the series.

@@ +920,5 @@
> +      //  Calculate parent CTM scale
> +      double determinant = parentCTM.Determinant();
> +      if (determinant < 0)
> +        determinant = -1.0 * determinant;     
> +      double scale = sqrt(determinant);       

You've got lots of trailing whitespace in this patch (which bugzilla's "review" view helpfully highlights in red for me :))

Please be sure that your added code isn't adding trailing whitespace.  If you're using mercurial, you can make "hg diff" show you trailing whitespace (highlighted in red) by adding "color=" or "hgext.color =" to your [extensions] section in your ~/.hgrc file:
https://www.mercurial-scm.org/wiki/ColorExtension

Comment 42

[Daniel Holbert [:dholbert]]

Comment on attachment 8830593 [details] [diff] [review]
bug-1318208-part6

Review of attachment 8830593 [details] [diff] [review]:
-----------------------------------------------------------------

A few nits on part6, though I haven't looked at it super-thoroughly since (as noted above) a lot of it seems to just be a rewrite of part2.

r- for now, but feel free to re-request review after you've addressed these changes (and ideally sorted this out so that it's not rewriting code from an earlier patch)

::: layout/svg/SVGGeometryFrame.cpp
@@ +922,5 @@
> +      frmOriginY = 0;
> +    }
> +    //  User to Outer Matrix : ScreenCTM or ViewportCTM
> +    nsSVGUtils::GetVectorEffectTransform(this, nSelectedCTM, &userToOuterSVG);
> +    //  Transform Matrix for Each Element 

Fix trailing whitespace on this line.

@@ +939,2 @@
>      //  non-scaling-size includes non-scaling-stroke property.
> +    //  2,3

In my first skim of this patch, it's unclear to me what this "2,3" comment means (and later comments "4,5" etc.)

Please include a bit more context, explaining what those are referring to.

::: layout/svg/SVGGeometryFrame.h
@@ +128,5 @@
> +    eNonScalingStrokeAndRotation = (eNonScalingStroke + eNonRotation),
> +    eFixedPosition    = (1 << 3),
> +    eNonScalingStrokeAndFixedPosition = (eNonScalingStroke + eFixedPosition),
> +    eNonRotateAndFixedPosition = (eNonRotation + eFixedPosition),
> +    eNonRotateAndFixedPositionAndNonStroke = (eNonRotation + eFixedPosition),

(1) Why are you creating all of these combination values here? (eFooAndBar) It doesn't look like you use them anywhere.

(2) The last two combination values seem to have identical values... (they're both eNonRotation + eFixedPosition)  Anyway, that's not an issue anymore if you just get rid of all the combinations.

(3) We had one preexisting combination value (eNonScalingStrokeAndSize) which we *did* use previous to this patch, but it looks like you're removing its usages in this patch's changes to the .cpp file -- so we might as well remove that enum-value entirely (from this enum definition).  (Actually, it looks like that combination value was created in part 2 -- so if it's doomed to be unused by the end of this series, it's probably cleaner to avoid adding it in the first place, if you can.)

(4) If you *do* happen to need these combination values, then please define them with boolean operator "|" instead of "+".  That's the more appropriate way to mask together individual bits.

Comment 43

[Daniel Holbert [:dholbert]]

Comment on attachment 8830594 [details] [diff] [review]
bug-1318208-part7

Review of attachment 8830594 [details] [diff] [review]:
-----------------------------------------------------------------

First-pass review for these reftests:

::: layout/reftests/svg/reftest.list
@@ -229,5 @@
>  fuzzy-if(d2d&&/^Windows\x20NT\x20(6\.1|10\.0)/.test(http.oscpu),63,168) fuzzy-if(cocoaWidget,1,122) fuzzy-if(skiaContent,2,1000) == non-scaling-stroke-01.svg non-scaling-stroke-01-ref.svg # bug 1074161 for Win7 and OSX 10.8
>  fuzzy-if(gtkWidget,1,99) fuzzy-if(!contentSameGfxBackendAsCanvas,9,99) fuzzy-if(Android,9,586) == non-scaling-stroke-02.svg non-scaling-stroke-02-ref.svg
>  == non-scaling-stroke-03.svg non-scaling-stroke-03-ref.svg
>  # bug1318208 non-scaling-size
> -fuzzy-if(d2d||skiaContent,63,720) == non-scaling-size-01.svg non-scaling-size-01-ref.svg

It looks like you're adding these test files with one name in an earlier patch (in part 3), and then modifying & renaming them in this later patch (part 7).

Let's just add them with their final name up-front (whether that's adding them in part 3), and avoid needlessly rewriting them in separate patches unless there's a meaningful reason for the separation.

SO: you probably just want to fold this patch and part 3 together (for example, with the "hg qfold" command) to create a single patch with the final version of these tests.

@@ +231,5 @@
>  == non-scaling-stroke-03.svg non-scaling-stroke-03-ref.svg
>  # bug1318208 non-scaling-size
> +== svg2-vector-effect-01.svg svg2-vector-effect-01-ref.svg
> +== svg2-vector-effect-02.svg svg2-vector-effect-02-ref.svg
> +== svg2-vector-effect-03.svg svg2-vector-effect-03-ref.svg

Two nits:
(1) I'd rather we drop the "svg2-" prefix entirely from the filenames here. "vector-effect" is a specific enough name for these tests, I think -- we don't get any added clarity by prefixing it with "svg2".  (Most of the other files in this directory don't bother using "svg" as a prefix, because this is all inside of layout/reftests/svg/)

You should be able to fix this quickly & easily by opening this patch file in a text editor, and doing a search-and-replace to replace all instances of "svg2-" with the empty string.

(2) I assume these tests *only pass* when the new pref is enabled, right?  If so, you need to explicitly turn on the pref at the beginning of each reftest.list line here.  Otherwise, these tests will all fail (and trigger bustage on TreeHerder) when they eventually ride the trains to a release where the pref is disabled by default (Beta/Release).

So, your reftest.list lines should end up looking a bit like this:
test-pref(svg.whatever.enabled,true) fuzzy-if(...) == testcase.svg ref.svg
                              ^^
The lack of a space here -----'' is important for it to work correctly, as I recall. You can search for reftest.list files for "pref" or "test-pref" for examples with other prefs & tests.

With that, the harness will set the pref before loading the testcase. If you need the pref to be set for the reference case as well, then use "pref(...)" instead of "test-pref(...)".  (But ideally your reference cases shouldn't make use of the feature that they're providing a reference for -- so you should hopefully only need test-pref(...).)

::: layout/reftests/svg/svg2-vector-effect-05.svg
@@ +1,2 @@
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
> +<title>VectorEffect:fixed position</title>

This *looks* like CSS, but it's not valid, which is kind of problematic.

Let's just use the correct spelling of this CSS property name & value name in the title here, to be clearer & more precise. So:
 s/VectorEffect/vector-effect/
 s/fixed position/fixed-position/

There are a number of instances of this in this patch.  You might want to just open the patch file in a text-editor and do search-and-replace to apply these changes.

@@ +11,5 @@
> +    <stop offset="0" stop-color="blue"/>
> +    <stop offset="1" stop-color="lime"/>
> +  </linearGradient>
> +
> +  <linearGradient id="grad2" x1="0" y1="0" x2="100" y2="0" gradientUnits="userSpaceOnUse" gradientTransform="scale(1.0,1.0)">

Does the scale(1.0,1.0) actually do anything useful here? That looks like a no-op transform to me. Is there a reason you've got it here?

(If you do keep it for some reason, you might use "scale(1,1)" for brevity & readability)

@@ +35,5 @@
> +      <circle cx="0" cy="20" r="4"/>
> +    </g>
> +  </pattern>
> +  <rect id="rect1" width="100" height="50" transform="matrix(1.0,0,0,1.0,10,130)"/>
> +  <rect id="rect2" width="100" height="50" transform="matrix(1.0,0,0,1.0,10,190)"/>

Here as well, it seems odd that you've got "1.0" as the only value with a decimal point in this matrix. (It makes this a bit harder to read -- "1.0" alongside "10" etc)

You probably want to just replace "1.0" with "1", throughout this patch, for better readability & consistency & brevity.

::: layout/reftests/svg/svg2-vector-effect-09-ref.svg
@@ +18,5 @@
> +  <pattern id="pattern" x="0%" y="0%" width="15%" height="30%"  patternTransform="scale(1.0,1.0) rotate(45)">
> +    <circle cx="0"  cy="0"  r="5" fill="red"  fill-opacity="0.5"/>
> +    <circle cx="5" cy="5" r="5" fill="green" fill-opacity="0.5"/>
> +    <circle cx="10" cy="10" r="5" fill="blue" fill-opacity="0.5"/>
> +  </pattern> 

trailing space here -- please remove

::: layout/reftests/svg/svg2-vector-effect-10.svg
@@ +39,5 @@
> +    <rect x="0" y="0" width="50" height="25" fill="url(#grad1)" stroke="red" transform="matrix(1.0,0,0,1.0,6.82,1.83)"/>
> +</g>
> +<rect width="100" height="50" fill="url(#grad2)" stroke="blue" transform="matrix(1.0,0,0,1.0,10,70)"/>
> +
> +<use xlink:href="#rect1" transform="scale(0.25) rotate(45) " stroke="black" fill="url(#ptn)"/>

There's a trailing space at the end of the transform="..." attribute here, between close-paren & close-quote. Please get rid of that space.

Comment 44

[Daniel Holbert [:dholbert]]

Comment on attachment 8827795 [details] [diff] [review]
bug-1318208-part2

Review of attachment 8827795 [details] [diff] [review]:
-----------------------------------------------------------------

A few drive-by comments here which should be addressed before jwatt steps in to review this:

::: dom/svg/SVGCircleElement.cpp
@@ +104,5 @@
>      if (aStrokeOptions.mLineWidth > 0.f) {
> +      if (aToOuterSVGSpace) {
> +        Rect userBounds(x - r, y - r, 2 * r, 2 * r);
> +        // non-scaling-stroke
> +        if (aFrame->StyleSVGReset()->HasNonScalingStroke()) {

Observation:
 - You're using "aFrame" without null-checking it (which is reasonable assuming the caller is guaranteed to pass in a non-null aFrame).
 - BUT in the header file, you have aFrame set to null by default.

That means that if any caller uses the default arg, they'd trigger a crash.... This needs resolving one way or another. (Either it's expected to be always non-null & we can assert/assume that it's non-null, or we need to gracefully handle nullptr.)

::: dom/svg/SVGCircleElement.h
@@ +8,5 @@
>  #define mozilla_dom_SVGCircleElement_h
>  
>  #include "SVGGeometryElement.h"
>  #include "nsSVGLength2.h"
> +#include "nsIFrame.h"

You should just forward-declare nsIFrame here -- there's no need to add an #include.  (It looks like this is just for "const nsIFrame *aFrame" which you're adding in the function signature, and you're not actually using that variable in this file, so you don't need the full nsIFrame definition.)

So, just add this after the #include list (separated by a blank line):
class nsIFrame;

This applies to all of the .h files in this patch, I think (unless any of them actually use aFrame).

@@ +34,5 @@
>    // SVGGeometryElement methods:
>    virtual bool GetGeometryBounds(Rect* aBounds, const StrokeOptions& aStrokeOptions,
>                                   const Matrix& aToBoundsSpace,
> +								 const Matrix* aToOuterSVGSpace = nullptr,
> +								 const nsIFrame *aFrame = nullptr) override;

The indentation is way off here, because you're using tab characters instead of spaces.  Please use spaces instead of tabs, and make sure these new lines line up.

::: dom/svg/SVGContentUtils.cpp
@@ +490,5 @@
> +  return gfx::ToMatrix(matrix);
> +  }
> +  ancestor = ancestor->GetFlattenedTreeParent();
> +  }
> +  */

This file has several blocks of commented-out code that clearly need cleaning up.

@@ +491,5 @@
> +  }
> +  ancestor = ancestor->GetFlattenedTreeParent();
> +  }
> +  */
> +  return ToMatrix(matrix);

This function has another ~55 lines of code after this "return" statement, all of which is clearly unreachable...

::: dom/svg/SVGGeometryElement.h
@@ +98,5 @@
>     */
>    virtual bool GetGeometryBounds(Rect* aBounds, const StrokeOptions& aStrokeOptions,
>                                   const Matrix& aToBoundsSpace,
> +                                 const Matrix* aToOuterSVGSpace = nullptr,
> +                                 const nsIFrame *aFrame = nullptr) {

What is this new "aFrame" arg that you're adding here?  (That's what this patch is all about adding, it seems)  What frame is it expected to be?  If it's just the frame associated with this SVGGeometryElement object, then you should be able to just call GetPrimaryFrame() internally, and you don't need to bother bother touching the function signature at all...

Or if it's a different frame, you probably want to give it a clearer name to make it more obvious what it represents.

(This patch doesn't seem to update any of the callsites, so it's not clear to me what aFrame actually represents here.)

If you *do* need this aFrame parameter (i.e. if it's not just GetPrimaryFrame()), then please extend the documentation for this method in this header here, to do the following:
(a) explain what aFrame represents.
(b) explain whether aFrame is allowed to be null, & (if so) what the behavior will be if it is.
(NOTE, some of your implementations of this method, e.g. the one in SVGCircleElement.cpp, seem to assume aFrame is non-null by simply dereferencing it without null-checking it.)

And...
(c) If it's not allowed to be null, then get rid of the "= nullptr" default value.

Comment 45

[Daniel Holbert [:dholbert]]

(Also: before you spend time addressing the review feedback & working on improving the patches here, it's perhaps worth trying to get a better consensus on the intent-to-implement thread:
  https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.platform/T_7HTTdt-Es

The "have other browsers expressed interest in supporting this" question [raised by Boris & Jeff in that thread] is the most important outstanding question, I think. And if the answer there is "no", then it's not clear that a cost/benefit analysis would support adding the feature to Firefox/Gecko.)

Comment 46

[Sanggyu SON]

(In reply to Daniel Holbert [:dholbert] from comment #45)
> (Also: before you spend time addressing the review feedback & working on
> improving the patches here, it's perhaps worth trying to get a better
> consensus on the intent-to-implement thread:
>  
> https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.platform/
> T_7HTTdt-Es
> 
> The "have other browsers expressed interest in supporting this" question
> [raised by Boris & Jeff in that thread] is the most important outstanding
> question, I think. And if the answer there is "no", then it's not clear that
> a cost/benefit analysis would support adding the feature to Firefox/Gecko.)

Thank you for review and all the advice.
I will check about correspondence of other browsers.
We will fix the patch as soon as possible.

Comment 47

[Sanggyu SON]

Attachment: bug-1318208-part1

Comment 48

[Sanggyu SON]

Attachment: bug-1318208-part2

Comment 49

[Sanggyu SON]

I modified reviewed patches.

bug-1318208-part1 : 
 Add CSS for VectorEffect: non-scaling-size, non-rotation, fixed-position And Pref Entry "svg.vector-effect.extra-values.enabled" for these effect enable/disable switching.

bug-1318208-part2 : 
 Add VectorEffect execution code.
non-scaling-size, non-rotation, fixed-position and these combinations.

Now, I try to combine all of reftests for vector-effect.

Comment 50

[Sanggyu SON]

Attachment: bug-1318208-part3

Comment 51

[Sanggyu SON]

bug-1318208-part3:
I have modified patches that includes reftest files.
I setted test-perf() for every entry lines.

Comment 52

[Daniel Holbert [:dholbert]]

Thanks for fixing that up! I'm going to post a few review notes for things that I noticed in a quick skim.  But I'm not going to dive too far into code review here quite yet, since we really need to figure out whether this feature is something we should actually incorporate or not, per comment 45.

Comment 53

[Daniel Holbert [:dholbert]]

Comment on attachment 8831599 [details] [diff] [review]
bug-1318208-part2

Review of attachment 8831599 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/svg/SVGCircleElement.h
@@ +8,5 @@
>  #define mozilla_dom_SVGCircleElement_h
>  
>  #include "SVGGeometryElement.h"
>  #include "nsSVGLength2.h"
> +#include "nsIFrame.h"

This patch is still adding these #includes for nsIFrame.h, from an earlier patch-version where you were adding nsIFrame usages in these headers.

You should revert these #include additions (throughout this patch).

@@ +33,5 @@
>  
>    // SVGGeometryElement methods:
>    virtual bool GetGeometryBounds(Rect* aBounds, const StrokeOptions& aStrokeOptions,
>                                   const Matrix& aToBoundsSpace,
> +                                 const Matrix* aToOuterSVGSpace) override;

Observation: so, a lot of the changes in this patch are really just this ^^ rename. (throughout various methods)  Could you split that rename into its own patch, separate from the functional changes here?

(Where possible, it's best to perform non-functional rewrites (like this renaming) in a separate patch from functional changes. That way, it's easier to sanity-check the non-functional changes don't impact anything, and it's easier to analyze/review the functional changes if they're in their own more-targeted atomic commit.

Also: it looks like this patch sometimes removes the "= nullptr" default-arg from these lines.  I'm fine with removing it (in the subclasses of SVGGeometryElement) since it's probably not directly exercised there -- but if we do that, we should do it consistently.  So, please either: (1) consistently preserve this "= nullptr", or (2) consistently remove it [in subclasses].

::: dom/svg/SVGContentUtils.cpp
@@ +472,5 @@
> +GetOriginalCTMInternal(nsSVGElement *aElement, bool aScreenCTM, bool aHaveRecursed)
> +{
> +  gfxMatrix matrix = aElement->PrependLocalTransformsTo(gfxMatrix(), eUserSpaceToParent);
> +  nsSVGElement *element = aElement;
> +  nsIContent *ancestor = aElement->GetFlattenedTreeParent();

Minor Gecko Coding Style nit:
"In general, snuggle pointer stars with the type, not the variable name"
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations

(The existing code here isn't very consistent about this, unfortunately. Don't worry about this too much; mostly mentioning it for future reference)

@@ +533,5 @@
> +                               Rect* aBounds)
> +{
> +  double determinant =  aToBoundsSpace.Determinant();
> +  if (determinant < 0)
> +    determinant = -1.0 * determinant;

Gecko coding style requires "if" bodies to always be surrounded by braces (except rare cases like trivial early-returns).
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

But in this case, you should probably just collapse the if-check to:
 determinant = std::abs(determinant);

Also: you should include some explanatory comments here to explain what you're doing.

@@ +535,5 @@
> +  double determinant =  aToBoundsSpace.Determinant();
> +  if (determinant < 0)
> +    determinant = -1.0 * determinant;
> +  double scale = sqrt(determinant);
> +  Float a = 1.0/scale;

Is it possible that |scale| is 0 here?  (i.e. can aToBoundsSpace.Determinant() return 0?)

If so, you need to handle that, to avoid dividing by 0.

If not, you should probably assert that "determinant" is not equal to 0.

::: dom/svg/SVGContentUtils.h
@@ +215,5 @@
>    static nsresult ReportToConsole(nsIDocument* doc,
>                                    const char* aWarning,
>                                    const char16_t **aParams,
>                                    uint32_t aParamsLength);
> +  static Matrix GetOriginalCTM(nsSVGElement *aElement, bool aScreenCTM);

Please add documentation for this new function, in particular explaining how it differs from "GetCTM()" just below it.

@@ +235,5 @@
>                               const Matrix& aToNonScalingStrokeSpace,
>                               float aStrokeWidth,
>                               Rect* aBounds);
> +  static void
> +  GetBoundsSize(const Matrix& aToBoundsSpace, Rect* aBounds);

SVGContentUtils functions are pretty well documented, generally. Please add documentation for this new function.

::: layout/svg/nsSVGUtils.h
@@ +467,5 @@
>                                             gfxMatrix* aUserToOuterSVG);
> +  enum VectorEffectMatrixType {
> +           eScreenCTM          = 0,     // Flag Value for getting ScreenCTM 
> +           eViewportCTM        = 1,     // Flag Value for getting ViewportCTM 
> +           eElementMatrix      = 2,     // Flag Value for getting ElementMatrix 

There's a trailing space character at the end of each line here.

@@ +473,2 @@
>  
> +  static bool GetVectorEffectTransform(nsIFrame *aFrame,

(similar to my comment for SVGContentUtils.h): This file (nsSVGUtils.h) tries to have pretty good documentation about what its methods do. Please include documentation for this new method.

Comment 54

[Sanggyu SON]

(In reply to Daniel Holbert [:dholbert] from comment #53)

Thank you for review. 
I am sorry that the part pointed out last review was not corrected well.
I will carefully check these comments and modify the patch.

Thanks for the quick review.　(^^♪

Comment 55

[Daniel Holbert [:dholbert]]

(In reply to Sanggyu SON from comment #54)
> I am sorry that the part pointed out last review was not corrected well.

(No worries - your corrections were nice, it's just that complex patch series often go through multiple rounds of review because it's hard to catch everything at once. Don't take that as a knock against you. :))

Comment 56

[Sanggyu SON]

Attachment: bug-1318208-part2

Comment 57

[Sanggyu SON]

Attachment: bug-1318208-part3

Comment 58

[Sanggyu SON]

Attachment: bug-1318208-part4

Comment 59

[Sanggyu SON]

Although I was a little late, we updated patches according to review.
Older patch 2 splitted to two patchs. 
bug-1318208-part2 : variable name change.
bug-1318208-part3 : function change for vector-effect.
bug-1318208-part4 : reftest modified parameter description.
part1 is not modified in this time.

Comment 60

[Sanggyu SON]

Attachment: bug-1318208-part3

I'm sorry, I made a mistake in writing one place.(bug-1318208-part3)

Comment 61

[Sanggyu SON]

dom/svg/SVGCircleElement.h
dom/svg/SVGRectElement.h
> +#include "nsIFrame.h"

I tried to erase the Include of this part, but when recalculating the boundary after transformation it is necessary to access the element of the IFrame to determine the type of the Vector effect being used. For this reason, it could not be erased.
In other way, if increase the arguments of the calling function and pass the determined information in the preceding stage, this part is unnecessary, but please give me a better manner.

Comment 62

[Daniel Holbert [:dholbert]]

(In reply to Sanggyu SON from comment #61)
> dom/svg/SVGCircleElement.h
> dom/svg/SVGRectElement.h
> > +#include "nsIFrame.h"
> 
> I tried to erase the Include of this part, but when recalculating the
> boundary after transformation it is necessary to access the element of the
> IFrame to determine the type of the Vector effect being used. For this
> reason, it could not be erased.

That access ("frame->...") happens in the .cpp file, though, right?  So, this new #include should also be added in the .cpp file (where the access happens), rather than being added to the .h file.

Having said that: I'm going to cancel the review requests on the patches here, for now, since it doesn't make sense to invest time in pushing this forward at this point unless/until it's clear that Mozilla should add this feature, per comment 45 & the posts in the dev-platform thread.

Comment 63

[ramin]

Is it the policy of Mozilla to address volunteers to Chrome or IE (who have their own priorities like Mozilla) rather than encouraging them by giving some technical advise?

Comment 64

[Robert Longson [:longsonr]]

There isn't much point in giving you technical advice as to how to complete this feature if IE and Chrome have no intention of achieving feature-parity. A feature that's only supported by Firefox (or IE or Chrome for that matter) has little value because it can't really achieve widespread use. 

We don't need IE or Chrome to complete their implementation of this feature before we go forward with it, we just need an indication that it's on their radar and they plan to do so within a reasonable timescale.

Comment 65

[ramin]

and in which format must this "indication" be presented? Do we expect Chrome or IE make public signals of what exactly they are doing and what they are going to do? as for SVG2 

Status in Chromium : under development
https://www.chromestatus.com/feature/5760616295825408

Status in Edge: under evaluation
https://developer.microsoft.com/en-us/microsoft-edge/platform/status/svg2/
"We are still evaluating this technology. There may be significant spec stabilization, foundational work, or additional community input required before we can begin development."

Comment 66

[Robert Longson [:longsonr]]

yes we do expect public signals. And they need to be more granular than "we're considering SVG 2", i.e. we need to know about this specific feature from SVG 2.

Comment 67

[Brian Birtles (:birtles)]

Unfortunately both Edge and Chrome have so far indicated they do not support the feature here:
https://docs.google.com/spreadsheets/d/1kkqzcxY53h7liRYppLSSFG2sjaJ8V8TCP5rWLZK0AxA/edit#gid=0

I'm sorry but we can't ship a platform feature unless we expect other browsers to follow, so we really do need some indication other browsers will likely implement this in future. Even something like comments from Chrome team members on the chromium bug tracker would be a useful public signal.

Comment 68

[ramin]

Hi Brian, good to hear from you. yes I've seen that document before, but I didn't find
anywhere in that document that it indicates the official status of each browser.
( and I couldn't get what that "+" mark (medium priority) in "non-scaling-size" row "Gecko" column means... are you considering "non-scaling-size"? and if the answer is yes, are you going to ship 
a platform with this feature? )

Comment 69

[Brian Birtles (:birtles)]

Yes, that document is completed by the browser vendors (except for already-shipped features where I believe someone else provided the data). The '+' in non-scaling-size is because of this bug. That is, we intend to ship this feature but that is contingent on other browsers also intending to do the same.

Comment 70

[Amelia Bellamy-Royds]

Given Brian's reasonable argument that Firefox should not ship until there is some sort of commitment to implement from other browsers, the next step is to solicit interest from SVG creators, so that they can petition other browser teams to support the feature.

The SVG WG spreadsheet linked above was a one-time effort to identify browser teams' priorities.  It is not likely to be updated and does not provide a way for authors to weigh in.  So further discussion should happen in the places where other browser teams discuss feature requests:

There's an outstanding MS Edge user voice request for vector-effect (they do not support even non-scaling-stroke) here: https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/suggestions/15078327-vector-effect

I have created a Chromium feature request here: https://bugs.chromium.org/p/chromium/issues/detail?id=691398

I don't have a Webkit bugzilla login, but if anyone wants to create a matching feature request there, please feel free to copy & paste my text from the Chrome bug.

Comment 72

[Alexander V. Ulyanov]

Hi. Why it does't work to <rect>?

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 312 48" preserveAspectRatio="none" onclick="(document.fullscreenElement)?document.exitFullscreen():this.requestFullscreen()"><rect width="8" height="8" fill="red" vector-effect="non-scaling-size"/></svg>

Comment 73

[Sebastian Zartner [:sebo]]

Because vector-effect="non-scaling-size" is not implemented yet. It's the purpose of this bug to implement it.

Sebastian

Comment 74

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:jwatt, could you have a look please?
For more information, please visit auto_nag documentation.