Repository navigation
SVG improvements - #2926
SVG improvements#2926TimothyGu wants to merge 8 commits into
Conversation
domenic
left a comment
domenic
left a comment
There was a problem hiding this comment.
Wow, again, great work tackling a user pain point! It's awesome to see the reflection support is flexible enough to handle this sort of thing.
It looks like my biggest concern is basically a spec issue, about DOMRect/DOMRectReadonly not being specced in a way that actually makes sense in an SVG context. The rest are mostly nits.
And, I guess, tests will be annoying :(.
|
I was looking for DOMRect support in jsdom and found this PR. Maybe it's ready for another review ? |
| optional unrestricted double width = 0, optional unrestricted double height = 0); | ||
|
|
||
| // https://github.com/jsdom/webidl2js/issues/186 | ||
| // [NewObject] static DOMRect fromRect(optional DOMRectInit other = {}); |
There was a problem hiding this comment.
| // [NewObject] static DOMRect fromRect(optional DOMRectInit other = {}); | |
| [NewObject, WebIDL2JSCallWithGlobal] static DOMRect fromRect(optional DOMRectInit other = {}); |
|
Hey folks. Any progress? We have a huge svg based app and cannot work fine. I think this PR may resolve the problem. |
|
Same as @julienw for me; was looking for I second @Kingwl’s curiosity, too. (Great work on JSDom in general, and @TimothyGu for this awesome work!) |
|
#2956 seems to be fixed now :-) |
Namely, <defs>, <desc>, <g>, <metadata>, and <switch>. Fixes jsdom#2734.
New SVG elements: - SVGSymbolElement - SVGUseElement New SVG interface mixins: - SVGFitToViewBox - SVGURIReference New auxiliary classes: - DOMRect - DOMRectReadOnly - SVGAnimatedPreserveAspectRatio - SVGAnimatedRect - SVGPreserveAspectRatio Implemented reflection for: - SVGAnimatedPreserveAspectRatio - SVGAnimatedRect - SVGAnimatedString
|
Any updates on this? Looks like this PR has been sitting around for a while and my team could really use the |
| const reflectDeprecatedAttr = idl.extAttrs.find(extAttr => extAttr.name === extAttrName); | ||
| if (reflectDeprecatedAttr) { | ||
| return JSON.parse(reflectDeprecatedAttr.rhs.value); |
There was a problem hiding this comment.
This function supports any extended attribute, so the variable name should probably be more generic:
| const reflectDeprecatedAttr = idl.extAttrs.find(extAttr => extAttr.name === extAttrName); | |
| if (reflectDeprecatedAttr) { | |
| return JSON.parse(reflectDeprecatedAttr.rhs.value); | |
| const foundAttr = idl.extAttrs.find(extAttr => extAttr.name === extAttrName); | |
| if (foundAttr) { | |
| return JSON.parse(foundAttr.rhs.value); |
| // Use a do-while "loop" as JavaScript `goto end`. | ||
| // eslint-disable-next-line no-unreachable-loop | ||
| do { |
There was a problem hiding this comment.
You can use a labelled block to achieve this:
| // Use a do-while "loop" as JavaScript `goto end`. | |
| // eslint-disable-next-line no-unreachable-loop | |
| do { | |
| // Use a labelled block as JavaScript `goto end`. | |
| block: { |
| do { | ||
| // When the attribute does not exist. | ||
| if (typeof str !== "string") { | ||
| break; |
There was a problem hiding this comment.
| break; | |
| break block; |
| skipSpace(); | ||
| const xStr = matchNumber(); | ||
| if (!xStr) { | ||
| break; |
There was a problem hiding this comment.
| break; | |
| break block; |
| break; | ||
| } | ||
| if (!skipDelimiter()) { | ||
| break; |
There was a problem hiding this comment.
| break; | |
| break block; |
| } | ||
| const heightStr = matchNumber(); | ||
| if (!heightStr) { | ||
| break; |
There was a problem hiding this comment.
| break; | |
| break block; |
| // Test for trailing junk. | ||
| skipSpace(); | ||
| if (i < str.length) { | ||
| break; |
There was a problem hiding this comment.
| break; | |
| break block; |
| // A negative value for <width> or <height> is an error and invalidates the 'viewBox' attribute. | ||
| const width = Number(widthStr); | ||
| if (width < 0) { | ||
| break; |
There was a problem hiding this comment.
| break; | |
| break block; |
| } | ||
| const height = Number(heightStr); | ||
| if (height < 0) { | ||
| break; |
There was a problem hiding this comment.
| break; | |
| break block; |
| skipSpace(); | ||
| return i > start; | ||
| } | ||
| } while (false); |
There was a problem hiding this comment.
| } while (false); | |
| } |
|
Almost Can I request a re-review? Would love to use DOMRect and friends in my JSDOM tests… † EDIT: (Still waiting for my coffee to reach my brain.) It’s almost two years since my last visit to this issue. |
|
@domenic Thank you for #3555! 🙏 @TimothyGu Are you still working on this? If not, can someone mark this “HELP WANTED” (or similar), and find a steward to take over to completion? |
|
Found this from here. Would be awesome to see this merged! What is the status here. I see its been a few months. |
|
Rebased and merged in #3843. |
8 participants
New elements:
<defs>,<desc>,<g>,<metadata>,<switch>,<symbol>,<use>.New method:
SVGSVGElement.createSVGRectAuxiliary classes:
Tests are being written:
<defs><desc><g><metadata><switch><symbol><use>Fixes #2734.
Some roadmap for future work:
SVGAnimatedLength+ reflection (complicated, as it requires us to implement a CSS dimension parser and unit converter – with support for relative units likeem, ideally)