Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CanvasFilter objects as input to CanvasRenderingContext2D.filter attribute #6763

Merged
merged 3 commits into from
Dec 14, 2021

Conversation

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic domenic added addition/proposal New features or enhancements topic: canvas labels Jun 14, 2021
@annevk
Copy link
Member

annevk commented Jun 17, 2021

@whatwg/canvas please review as well.

@mysteryDate

This comment has been minimized.

@domenic
Copy link
Member

domenic commented Jul 28, 2021

Pulling out a separate thread from the data model to discuss some input questions:

Currently the input is defined as CanvasFilterDictionary which has four properties. I'm a bit unclear: is the intent to support only those four types of filters? Do blur, colorMatrix, convolveMatrix, and componentTransfer corresponding directly to specific filters from https://drafts.fxtf.org/filter-effects-1/#FilterPrimitivesOverview ? If so we need a translation table since you seem to not be using the same names, e.g. I think blur corresponds to feGaussianBlur?

If you want to support arbitrary filters using the same names as the SVG spec, then record<> is probably better than CanvasFilterDictionary, so that you can operate generically. But if you want to specifically opt in only some filters, with new names, then yeah, you need a custom dictionary.


What happens if I fill in more than one member of the dictionary? All the examples seem to only fill in a single one.


I assume you want to allow a pipeline to contain multiple of the same type of filter, right? E.g.

new CanvasFilter([
  { blur: ... },
  { colorMatrix: ... },
  { blur: ... }
]);

Otherwise we could get rid of the array, i.e. transform

const f = new CanvasFilter([
  { blur: ... },
  { colorMatrix: ... }
]);

into

const f = new CanvasFilter({
  blur: ...,
  colorMatrix: ...
});

(taking advantage of the fact that JavaScript objects are ordered.)


You can probably get more type-safety, and get the bindings layer to do more work for you, by using record<> instead of object for the value. E.g. instead of object blur you could use record<DOMString, DOMString> blur or maybe record<DOMString, (DOMString or long long or sequence<long long> or boolean)>.

Of course, you'd get maximal type-safety---and I think it would also benefit ecosystems like TypeScript, which generate IDE autocomplete from the IDL---if you created separate dictionaries for each filter type, which contained exactly the right properties with exactly the right types. But that would require serious maintenance work to keep in sync with the SVG filters spec. (Probably we'd want to define those dictionaries in the SVG filter spec and just reference them from HTML...)

source Outdated Show resolved Hide resolved
source Outdated
Comment on lines 65949 to 65950
<p class="note">Currently, <code>CanvasFilter</code>s can only be linked lists. Full filter
graphs are a planned expansion of this feature.</p>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is agreed upon

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was basing this off of the conclusions of the TAG review (w3ctag/design-reviews#627) I'm okay with leaving this note out for the time being if it makes other implementers uncomfortable and TAG is okay with it.

@mysteryDate
Copy link
Contributor Author

is the intent to support only those four types of filters?

No, these four were the top requested by our users/partners. The plan is to eventually support all, though some make little sense in canvas world. For example, feFlood is just fillRect, but if we have a simple and extensible way of supporting ALL filters I don't see why not have things like feFlood, for the weirdos who hate fillRect for some reason. (Insert something about Hyrum's law here).

Also, there was a preference expressed by Apple to keep things modest initially in this thread: "In practice, most content uses only a fairly small set of filter types. If we're going to add filter support, we should start with a small set of popular filters, and only expand the set if compelling needs arise."

If so we need a translation table since you seem to not be using the same names, e.g. I think blur corresponds to feGaussianBlur?

To be honest that was an arbitrary decision made by me when writing the initial proof-of-concept. I'd still love to drop the "fe" prefix though.

If you want to support arbitrary filters using the same names as the SVG spec, then record<> is probably better than CanvasFilterDictionary, so that you can operate generically.

Spec-wise that sounds good, but given Apple's reluctance to bite this all off at once that feels unwise. Each one of these does require some work on the user-agent side.

What happens if I fill in more than one member of the dictionary? All the examples seem to only fill in a single one.

Good question. In my head these dictionaries were really designed as individual stages, but having multiple members filled out will simply apply those filter stages in the order they appear. Perhaps this is another point in favor of record.

pipeline to contain multiple of the same type of filter,

Yeah, totally necessary.

You can probably get more type-safety, and get the bindings layer to do more work for you, by using record<> instead of object for the value. E.g. instead of object blur you could use record<DOMString, DOMString> blur or maybe record<DOMString, (DOMString or long long or sequence or boolean)>.

I like this, though the idea with using plain-old dictionaries was one put for by @fserb to reduce the amount of changes to IDL files that would be needed. Thoughts @fserb?

@domenic
Copy link
Member

domenic commented Jul 29, 2021

Based on what you've said so far, my recommendation is that you do something like the following. It takes into account the following desires:

  • Easy to extend to other filter types in the future; just expand the "supported filter names" list.
  • Leverage the existing filter spec as much as possible, instead of having to create per-filter spec or IDL.
  • Make these APIs behave like "strongly typed" parts of the platform for the attribute values, instead of just saying the values match XML and are always strings.
typedef record<DOMString, record<DOMString, any>> CanvasFilterInput;

[Exposed=(Window,Worker)]
interface CanvasFilter {
  constructor(CanvasFilterInput or sequence<CanvasFilterInput>) filters);
};

Define the supported filter names to be « "gaussianBlur", "colorMatrix", "convolveMatrix", "componentTransfer" ».

Define an XML filter as a struct whose items are a name, a string, and attributes, an ordered map of strings to strings.

Define the IDL type for the canvas filter attribute attrName using an algorithm that looks at the grammar definition and returns one of boolean, long long, sequence<long long>, or DOMString.

Then your code to assemble the normalized form as part of the constructor looks something like:

  1. Let xmlFilters be an empty list.
  2. If filters is a CanvasFilterInput, then set filters to « filters ».
  3. For each filterDict of filters:
    1. For each nameattributes of filterDict:
      1. If name is not one of the [supported filter names], then continue. (Or throw?)
      2. Let xmlName be the concatenation of "fe", the first code unit of name converted to ASCII uppercase, and all code units of name after the first one.
      3. Let xmlFilter be a new [XML filter] whose [name] is xmlName and whose [attributes] is an empty ordered map.
      4. Append xmlFilter to xmlFilters.
      5. For each attrNameattrValue of attributes:
        1. If any of the following are true:
          • attrName is not listed as an attribute for the filter primitive given by xmlName
          • attrName is a core attribute
          • attrName is a presentation attribute
          • attrName is a filter primitive attribute
          • attrName is "class", "style", or "in"
            then continue. (Or throw?)
        2. Let type be the [IDL type for the canvas filter attribute] attrName.
        3. Let attrIDLValue be the result of converting attrValue to type. (This step does things like convert "false" to true for boolean-types, or converting document.documentElement into "[object HTMLHtmlElement]" for string types, just like other places on the platform that accept booleans or strings.)
        4. Let attrXMLValue be the result of converting attrIDLValue to an ECMAScript value, and then converting that result to a DOMString. (This step just gets us back into strings since that's what XML operates on.)
        5. Set xmlFilter's attributes[attrName] to attrXMLValue.

Some notes:

  • I went with record<> for the top-level type instead of dictionary because dictionary keys are always lexicographically ordered, which might be confusing. That is, if we used dictionary, then new CanvasFilter({ gaussianBlur: ..., convolveMatrix: ... }) would get normalized by the bindings layer (before your code/spec steps ever saw it) into new CanvasFilter({ convolveMatrix: ..., gaussianBlur: ... }). Using record<> preserves the ordering.

  • I went with any instead of the union type we discussed above because I realized that you need to do a conversion that depends on the specific attribute in question. So doing a generic bindings-layer conversion into the union type (boolean or long long or sequence<long long> or DOMString) doesn't buy us anything; someone could still put in a string (which is allowed by the union) for a sequence-of-numers attribute. We need to do the conversion in prose.

@Kaiido
Copy link
Member

Kaiido commented Jul 30, 2021

For each name → attributes of filterDict:

Is there a precedent where the order of the keys is preserved in such a meaningful manner?
As a web author I find this very surprising, and I really wouldn't expect anyone to understand why they have completely different graphic results depending on when in their code they did insert a property to a JS object.

I feel it would be a lot more simpler to not wrap the attributes inside a filterDict, but instead have its name be one of the members of this attribute.

So something like

const f = new CanvasFilter([
   { name: "convolveMatrix", kernelMatrix: [ ... ] },
   { name: "gaussianBlur", stdDeviation: 2 }
 ]);

This way no ambiguity about the order (though if graph is coming, I guess ins and outs attribute's members would be able to change that order).

Note: name was an attribute used on some SVG elements (<font-face-name> and <color-profile> both deprecated in SVG2), I doubt it would ever become one of the official attributes on any future SVG filter, but of course there may be a better name for this.

@mysteryDate
Copy link
Contributor Author

I feel it would be a lot more simpler to not wrap the attributes inside a filterDict, but instead have its name be one of the members of this attribute.

@Kaiido I like this. Unless @fserb objects I vote we go with that. I also think leveraging the ordering of javascript objects is surprising.

Note: name was an attribute used on some SVG elements ( and both deprecated in SVG2), I doubt it would ever become one of the official attributes on any future SVG filter, but of course there may be a better name for this.

Unless I'm missing something, it looks like SVG itself uses "name": https://drafts.fxtf.org/filter-effects/#elementdef-fecolormatrix

@Kaiido
Copy link
Member

Kaiido commented Aug 4, 2021

Unless I'm missing something, it looks like SVG itself uses "name": https://drafts.fxtf.org/filter-effects/#elementdef-fecolormatrix

Yes, they use the word "name" to describe the filters in the specs, my concern was about the fact that there could theoretically have been an attribute named name on one of these filters.

<feFooBar name="meaningful-value"/>

In this case that would have conflicted with this model, since we could not set both the CanvasFilter's name and the SVGFilter's attribute.

So I checked for such attributes, and the two only I found were on unrelated elements, both deprecated now.
Still I think it's a point to keep in mind. Maybe there is a way to let SVG folks know we have this case that forbids the addition of this attribute on future filters, but I'm not aware of the procedure.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a description of how "XML filter" is mapped to a filter to be used for rendering. I guess it pretty straightforwardly maps to a filter primitive (why is that not its name?), but that needs to be spelled out somewhere.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@mysteryDate
Copy link
Contributor Author

@annevk

This needs a description of how "XML filter" is mapped to a filter to be used for rendering. I guess it pretty straightforwardly maps to a filter primitive (why is that not its name?), but that needs to be spelled out somewhere.

I added some minimal language to 4.12.5.1.21 Drawing model. Thoughts?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with only a couple conceptual issues remaining before we can move on to editorial tweaks:

  • Make sure each CanvasFilter has a defined XML filter list associated with it.
  • Do you want to change to the structure @Kaiido mentioned where instead of records we use { name: "x", attr1: "y", attr2: "z" } structure? In that case I think CanvasFilterInput becomes just record<DOMString, any>, and then the constructor code needs to be updated to special-case name (before the generic attributes loop).

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, great! The overall model looks good so we're at the point of lots of editorial tweaks :)

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@mysteryDate
Copy link
Contributor Author

@Kaiido

I feel it would be a lot more simpler to not wrap the attributes inside a filterDict, but instead have its name be one of the members of this attribute.

I spoke with @fserb about this and he suggested the key "filter" in order to be even less ambiguous. With this kind of use "name" is a little funny:

const f1 = {
  name: "gaussianBlur", 
  stdDeviation: 2 
 };
const f2 = {
  name: "convolveMatrix", 
  kernelMatrix: [ ... ]
 };
const f = new CanvasFilter([f1, f2]);

As opposed to:

const f1 = {
 filter: "gaussianBlur", 
 stdDeviation: 2 
};
const f2 = {
 filter: "convolveMatrix", 
 kernelMatrix: [ ... ]
};
const f = new CanvasFilter([f1, f2]);

Thoughts?

@Kaiido
Copy link
Member

Kaiido commented Aug 17, 2021

I spoke with @fserb about this and he suggested the key "filter" in order to be even less ambiguous. With this kind of use "name" is a little funny

I personally do like filter a bit more too, there is probably even less risk of collision with an hypothetic future attribute1, and indeed, name made sense because we were talking about CanvasFilters, but in user code these are just objects, so it looses its meaning.
That being said, this becomes more of an esthetic question than a real practical one as it was before, so if anyone has any objection or an even better proposition, I wouldn't mind either way.

1. Technically, the global filter attribute is applicable even on <filter> elements, but I don't think this would ever have any visible effect.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 18, 2021
As initially specced, CanvasFilter inputs had a single key that was the
name of the filter:

ctx.filter = new CanvasFilter({gaussianBlur: {stdDeviation: 5}});

As the result of the debate here
(whatwg/html#6763) the interface is changed to
have a "filter" key that names the filter type:

ctx.filter = new CanvasFilter(
  {filter: "gaussianBlur", stdDeviation: 5}});

Bug: 1169216
Change-Id: If3fdd9185c1f02dd2dece7db0c92aba76f2d97e1
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 30, 2021
As initially specced, CanvasFilter inputs had a single key that was the
name of the filter:

ctx.filter = new CanvasFilter({gaussianBlur: {stdDeviation: 5}});

As the result of the debate here
(whatwg/html#6763) the interface is changed to
have a "filter" key that names the filter type:

ctx.filter = new CanvasFilter(
  {filter: "gaussianBlur", stdDeviation: 5});

Bug: 1169216
Change-Id: If3fdd9185c1f02dd2dece7db0c92aba76f2d97e1
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 30, 2021
As initially specced, CanvasFilter inputs had a single key that was the
name of the filter:

ctx.filter = new CanvasFilter({gaussianBlur: {stdDeviation: 5}});

As the result of the debate here
(whatwg/html#6763) the interface is changed to
have a "filter" key that names the filter type:

ctx.filter = new CanvasFilter(
  {filter: "gaussianBlur", stdDeviation: 5});

Bug: 1169216
Change-Id: If3fdd9185c1f02dd2dece7db0c92aba76f2d97e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3098009
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#916494}
@annevk
Copy link
Member

annevk commented Dec 8, 2021

Reference tests are fine for successful conversions (although you could use getImageData() presumably?). However, to ensure he conversions are implemented correctly we should also test the cases where they end up throwing (as it seems these exceptions will end up rethrowing and abort the constructor steps).

And yeah, merging this PR makes this feature part of the standard and the standard ought to have good test coverage, so we should have those tests. The process is a bit quicker than in other standards organizations, but we still have some prerequisites.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 9, 2021
From the discussion here (whatwg/html#6763) we
need to explicitly test all possible types of inputs for filter
attributes.

Bug: 1201359
Change-Id: I5ef5905298a17310e4eb54c3c01ab8d6b7973737
@mysteryDate
Copy link
Contributor Author

mysteryDate commented Dec 10, 2021

@annevk Here's the PR with the tests crrev.com/c/3320960

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @annevk?

aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 13, 2021
From the discussion here (whatwg/html#6763) we
need to explicitly test all possible types of inputs for filter
attributes.

Bug: 1201359
Change-Id: I5ef5905298a17310e4eb54c3c01ab8d6b7973737
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3320960
Reviewed-by: Yi Xu <yiyix@chromium.org>
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#951163}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 13, 2021
From the discussion here (whatwg/html#6763) we
need to explicitly test all possible types of inputs for filter
attributes.

Bug: 1201359
Change-Id: I5ef5905298a17310e4eb54c3c01ab8d6b7973737
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3320960
Reviewed-by: Yi Xu <yiyix@chromium.org>
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#951163}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 13, 2021
From the discussion here (whatwg/html#6763) we
need to explicitly test all possible types of inputs for filter
attributes.

Bug: 1201359
Change-Id: I5ef5905298a17310e4eb54c3c01ab8d6b7973737
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3320960
Reviewed-by: Yi Xu <yiyix@chromium.org>
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#951163}
@annevk
Copy link
Member

annevk commented Dec 14, 2021

Thanks, looks good to me as well. Will you file the remaining implementation bugs @mysteryDate? See instructions at https://github.com/whatwg/meta/blob/main/MAINTAINERS.md.

I might push another fixup for some indentation issue I just noticed.

@mysteryDate
Copy link
Contributor Author

Yes, I'll file the remaining implementation bugs this week. Glad to be moving on! 🙌

@domenic domenic merged commit 5db6a65 into whatwg:main Dec 14, 2021
@domenic
Copy link
Member

domenic commented Dec 14, 2021

Woohoo, thanks for sticking with us through this very long (323 comment!) review. I think the resulting spec is really nice.

@mysteryDate
Copy link
Contributor Author

Yay! Happy holidays everyone. So glad to get this in.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 24, 2021
…asFilter, a=testonly

Automatic update from web-platform-tests
Test outside of IDL conversions for CanvasFilter

From the discussion here (whatwg/html#6763) we
need to explicitly test all possible types of inputs for filter
attributes.

Bug: 1201359
Change-Id: I5ef5905298a17310e4eb54c3c01ab8d6b7973737
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3320960
Reviewed-by: Yi Xu <yiyix@chromium.org>
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#951163}

--

wpt-commits: fee11623a87d803799f1ec469edcbbefa7d72383
wpt-pr: 31973
aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 21, 2022
If the "filter" key to a new CanvasFilter is unsupported or empty it
will now warn the user in the console.
See discussion here: whatwg/html#6763

I'm out of ideas for how to test this as there appears to be no way to
catch the warning in javascript.

Bug: 1272819
Change-Id: I2847b5efd126e29bd3228a4de540671c21087539
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3296334
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#961977}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
As initially specced, CanvasFilter inputs had a single key that was the
name of the filter:

ctx.filter = new CanvasFilter({gaussianBlur: {stdDeviation: 5}});

As the result of the debate here
(whatwg/html#6763) the interface is changed to
have a "filter" key that names the filter type:

ctx.filter = new CanvasFilter(
  {filter: "gaussianBlur", stdDeviation: 5});

Bug: 1169216
Change-Id: If3fdd9185c1f02dd2dece7db0c92aba76f2d97e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3098009
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#916494}
NOKEYCHECK=True
GitOrigin-RevId: 833d2100d38c8656a28bb72109f07aa5158e9f9c
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
From the discussion here (whatwg/html#6763) we
need to explicitly test all possible types of inputs for filter
attributes.

Bug: 1201359
Change-Id: I5ef5905298a17310e4eb54c3c01ab8d6b7973737
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3320960
Reviewed-by: Yi Xu <yiyix@chromium.org>
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#951163}
NOKEYCHECK=True
GitOrigin-RevId: ba6cbce4987586600aa98a2b2289c55faa2cdfc3
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
If the "filter" key to a new CanvasFilter is unsupported or empty it
will now warn the user in the console.
See discussion here: whatwg/html#6763

I'm out of ideas for how to test this as there appears to be no way to
catch the warning in javascript.

Bug: 1272819
Change-Id: I2847b5efd126e29bd3228a4de540671c21087539
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3296334
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#961977}
NOKEYCHECK=True
GitOrigin-RevId: 3c24d94ae972621f005e42d6f74e0b6588fb44d5
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 30, 2023
Only the height was checked (rejecting `[]` kernels) but kernels with
zero width like `[[], []]` were incorrectly accepted.

The current CanvasFilter specification is defined by reusing the
SVG specification for how the filters attributes work
(whatwg/html#6763). For the
"feConvolveMatrix" filter, the SVG specification says
(https://www.w3.org/TR/SVG11/filters.html#feConvolveMatrixElement):

"Attribute definitions:
- order = "<number-optional-number>"
  Indicates the number of cells in each dimension for ‘kernelMatrix’.
  The values provided must be <integer>s greater than zero. The first
  number, <orderX>, indicates the number of columns in the matrix.
  The second number, <orderY>, indicates the number of rows in the
  matrix. If <orderY> is not provided, it defaults to <orderX>."

Change-Id: I23baf05db73e679a52cd9d553d7c6308d91b56f9
Bug: 1428652
aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 30, 2023
Only the height was checked (rejecting `[]` kernels) but kernels with
zero width like `[[], []]` were incorrectly accepted.

The current CanvasFilter specification is defined by reusing the
SVG specification for how the filters attributes work
(whatwg/html#6763). For the
"feConvolveMatrix" filter, the SVG specification says
(https://www.w3.org/TR/SVG11/filters.html#feConvolveMatrixElement):

"Attribute definitions:
- order = "<number-optional-number>"
  Indicates the number of cells in each dimension for ‘kernelMatrix’.
  The values provided must be <integer>s greater than zero. The first
  number, <orderX>, indicates the number of columns in the matrix.
  The second number, <orderY>, indicates the number of rows in the
  matrix. If <orderY> is not provided, it defaults to <orderX>."

Change-Id: I23baf05db73e679a52cd9d553d7c6308d91b56f9
Bug: 1428652
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4377066
Reviewed-by: Aaron Krajeski <aaronhk@chromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravel@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124423}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 30, 2023
Only the height was checked (rejecting `[]` kernels) but kernels with
zero width like `[[], []]` were incorrectly accepted.

The current CanvasFilter specification is defined by reusing the
SVG specification for how the filters attributes work
(whatwg/html#6763). For the
"feConvolveMatrix" filter, the SVG specification says
(https://www.w3.org/TR/SVG11/filters.html#feConvolveMatrixElement):

"Attribute definitions:
- order = "<number-optional-number>"
  Indicates the number of cells in each dimension for ‘kernelMatrix’.
  The values provided must be <integer>s greater than zero. The first
  number, <orderX>, indicates the number of columns in the matrix.
  The second number, <orderY>, indicates the number of rows in the
  matrix. If <orderY> is not provided, it defaults to <orderX>."

Change-Id: I23baf05db73e679a52cd9d553d7c6308d91b56f9
Bug: 1428652
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4377066
Reviewed-by: Aaron Krajeski <aaronhk@chromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravel@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124423}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 30, 2023
Only the height was checked (rejecting `[]` kernels) but kernels with
zero width like `[[], []]` were incorrectly accepted.

The current CanvasFilter specification is defined by reusing the
SVG specification for how the filters attributes work
(whatwg/html#6763). For the
"feConvolveMatrix" filter, the SVG specification says
(https://www.w3.org/TR/SVG11/filters.html#feConvolveMatrixElement):

"Attribute definitions:
- order = "<number-optional-number>"
  Indicates the number of cells in each dimension for ‘kernelMatrix’.
  The values provided must be <integer>s greater than zero. The first
  number, <orderX>, indicates the number of columns in the matrix.
  The second number, <orderY>, indicates the number of rows in the
  matrix. If <orderY> is not provided, it defaults to <orderX>."

Change-Id: I23baf05db73e679a52cd9d553d7c6308d91b56f9
Bug: 1428652
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4377066
Reviewed-by: Aaron Krajeski <aaronhk@chromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravel@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124423}
cookiecrook pushed a commit to cookiecrook/wpt that referenced this pull request Apr 8, 2023
Only the height was checked (rejecting `[]` kernels) but kernels with
zero width like `[[], []]` were incorrectly accepted.

The current CanvasFilter specification is defined by reusing the
SVG specification for how the filters attributes work
(whatwg/html#6763). For the
"feConvolveMatrix" filter, the SVG specification says
(https://www.w3.org/TR/SVG11/filters.html#feConvolveMatrixElement):

"Attribute definitions:
- order = "<number-optional-number>"
  Indicates the number of cells in each dimension for ‘kernelMatrix’.
  The values provided must be <integer>s greater than zero. The first
  number, <orderX>, indicates the number of columns in the matrix.
  The second number, <orderY>, indicates the number of rows in the
  matrix. If <orderY> is not provided, it defaults to <orderX>."

Change-Id: I23baf05db73e679a52cd9d553d7c6308d91b56f9
Bug: 1428652
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4377066
Reviewed-by: Aaron Krajeski <aaronhk@chromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravel@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124423}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 15, 2023
…olveMatrix CanvasFilters., a=testonly

Automatic update from web-platform-tests
Reject kernelMatrix with 0 width in convolveMatrix CanvasFilters.

Only the height was checked (rejecting `[]` kernels) but kernels with
zero width like `[[], []]` were incorrectly accepted.

The current CanvasFilter specification is defined by reusing the
SVG specification for how the filters attributes work
(whatwg/html#6763). For the
"feConvolveMatrix" filter, the SVG specification says
(https://www.w3.org/TR/SVG11/filters.html#feConvolveMatrixElement):

"Attribute definitions:
- order = "<number-optional-number>"
  Indicates the number of cells in each dimension for ‘kernelMatrix’.
  The values provided must be <integer>s greater than zero. The first
  number, <orderX>, indicates the number of columns in the matrix.
  The second number, <orderY>, indicates the number of rows in the
  matrix. If <orderY> is not provided, it defaults to <orderX>."

Change-Id: I23baf05db73e679a52cd9d553d7c6308d91b56f9
Bug: 1428652
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4377066
Reviewed-by: Aaron Krajeski <aaronhk@chromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravel@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124423}

--

wpt-commits: 8b9f4e275162d151dc2a0c847601542d75051cbf
wpt-pr: 39255
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: canvas
Development

Successfully merging this pull request may close these issues.

8 participants