Fix #1365: Support image loading through node-canvas module. #1366

lehni · 2016-01-25T22:11:11Z

Also add support for:

HTMLImageElement.naturalWidth, naturalHeight, complet, currentSrc
Image drawing through HTMLCanvasElement
HTMLCanvasElement.toBlob()

lehni · 2016-01-26T14:07:35Z

The change in de89791 has fixed all failing tests as far as I can see. You may want to look into handling statusCodes properly elsewhere in the library, as it doesn't look like there is any handling of it right now.

I think whit this commit, my work is so far completed. i have done extensive tests with the paper.js library, and haven't managed to find any issues. What's missing for this still is unit tests. How are you testing the other parts of the library?

domenic · 2016-01-27T22:36:06Z

To test things that only work with the canvas package installed, we probably need to follow the pattern in test/living-html/htmlcanvaselement.js. The new web platform tests framework won't work as well for that.

domenic · 2016-01-30T01:23:37Z

lib/jsdom/living/nodes/HTMLCanvasElement-impl.js

  get width() {
-    const parsed = parseInt(this.getAttribute("width"));
+    const parsed = parseInt(this.getAttribute("width"), 10);


No need for the 10s; that is the default.

Oh, ok! But it used to be that parseInt("0...")returned an octal value on some browsers? I'll take it out again : )

ES5 removed octal interpretation. older browsers will indeed interpret it as octal, however those older browsers are not supported anyway, since we require ES6 features. Do note that: parseInt('0x123', 10) === 0 and parseInt('0x123') === 291

Yeah I read up on it in the meantime. There are definitely too many things to keep track of in the ES* universe, but it's an exciting time!

But looking through the jsdom source, I can spot other places where this is still in use, e.g. three locations in xmlhttprequest alone.

And lastly, I am wondering why HTMLImageElement-Impl.jsuses this:

conversions["unsigned long"](this.getAttribute("width"))

While HTMLCanvasElement-Impl.js uses this:

parseInt(this.getAttribute("width"))

lehni · 2016-02-08T10:29:35Z

Do you need more work from me for this PR, or is it just awaiting review now?

domenic · 2016-02-08T15:42:09Z

It needs tests!

lehni · 2016-02-08T17:15:26Z

I've been a bit frightened of touching that part, as it seems massive... Where do I find the canvas tests?

domenic · 2016-02-08T19:41:58Z

#1366 (comment)

lehni · 2016-03-14T16:40:03Z

Which command runs the sub-set of tests that test/living-html/htmlcanvaselement.js is part of?

domenic · 2016-03-14T16:42:35Z

npm run test-old -- -s htmlcanvaselement

lehni · 2016-03-14T16:44:43Z

Great, thanks! Is there a way to run both htmlcanvaselement and htmlimageelement?

lehni · 2016-03-14T16:45:11Z

npm run test-old -- -s htmlcanvaselement,htmlimageelement seems to do that. No need to respond.

lehni · 2016-03-27T22:40:44Z

I have finally started to implement unit tests for this in 3d9060d, but some are currently failing: When setting image.src to a data URL, the image does not get loaded immediately, due to the use of process.nextTick() in resourceLoader.load(): https://github.com/tmpvar/jsdom/blob/master/lib/jsdom/browser/resource-loader.js#L81

Could you tell me why this is used there? If I replace the call with the content of its callback, then it all works as expected. But I cannot judge if this would break things elsewhere.

domenic · 2016-03-27T23:06:17Z

I am not sure exactly why that is there; maybe there was just a desire that fetch() always be async. You can remove it, as long as it doesn't break other tests.

It would be great if you could rebase, instead of merge. This branch is not really readable or reviewable as-is.

lehni · 2016-03-27T23:12:15Z

Ok, I will give that a go. And I'm sorry about the merges. How can I fix this now?

domenic · 2016-03-27T23:14:30Z

If you rebase on master (and preferably squash your commits), you can then force-push once the local history looks reasonable.

lehni · 2016-03-27T23:15:15Z

Oh dear, that's all news to me...

lehni · 2016-03-27T23:15:56Z

I might just create a patch from all my changes and reapply it to master in one commit.

domenic · 2016-03-27T23:16:45Z

Yeah, that will work great!

lehni · 2016-03-27T23:17:40Z

Cool, I keep working with the current branch, and once I'm done I do that as the last move, if you're OK with that. I'm nearly done with finishing the tests.

lehni · 2016-03-27T23:23:34Z

Hmm I can't seem to be able to run all tests locally either way. It stalls at this line:

living-html/cookie.js: .FFF.

Do you have any ideas why?

domenic · 2016-03-27T23:47:53Z

I'm not really sure. Maybe try doing npm install in case the cookie dependency we use has not been updated on your system recently?

Also adds support for: - HTMLImageElement.naturalWidth, naturalHeight, complete, currentSrc - HTMLCanvasElement.drawImage() - HTMLCanvasElement.toBlob()

lehni · 2016-03-28T04:43:55Z

So here we go: Commit 86d9470 contains all my changes along with unit tests for them, and my modifications now pass all tests. This is good to go from my end, please review.

lehni · 2016-03-31T14:36:28Z

Is there anything else that I can do to help out and expedite the merging in of this? The upcoming v0.10.0 of paper.js depends on it and we are otherwise good to go, so hoping it can find its way into a release soonish...

domenic · 2016-03-31T17:38:51Z

Well, if you want to do my day job then that would certainly expedite things :). I generally work on jsdom on weekends when I have free time.

lehni · 2016-03-31T17:40:55Z

Haha, gotcha :) I didn't mean to put pressure on, just wanted to make sure I do what I can to get the ball rolling.

domenic · 2016-04-03T22:21:49Z

test/living-html/htmlimageelement.js

+  t.strictEqual(image.naturalWidth, 0, "before loading, naturalWidth should be 0");
+  t.strictEqual(image.naturalHeight, 0, "before loading, naturalHeight should be 0");
+  t.strictEqual(image.complete, false, "before loading, complete should be false");
+  t.strictEqual(image.src, "", "before loading, src should be an empty string");


This is incorrect. src should, per spec, reflect the src="" content attribute. It should not change in the course of loading.

Oh, I see, you simply haven't set src yet. OK, no problem.

domenic · 2016-04-03T22:47:50Z

Merged as 59f277b; thank you! Very cool stuff.

lehni · 2016-04-04T17:47:13Z

Sweet, thanks! I'm happy to help out where I can. As soon as I find the time, I will tackle #1368 next.

See jsdom/jsdom#1366

This was referenced Jan 25, 2016

Add optional support for image loading through Canvas.Image #1365

Closed

NPM Install for hybrid node browser environments paperjs/paper.js#739

Closed

lehni added a commit to paperjs/paper.js that referenced this pull request Jan 26, 2016

Update to jsdom v7.2.2 with jsdom/jsdom#1366 back-portedd.

1579790

lehni added a commit to lehni/jsdom that referenced this pull request Jan 26, 2016

Address all linting errors of jsdom#1366.

796379b

domenic reviewed Jan 30, 2016
View reviewed changes

lehni mentioned this pull request Feb 12, 2016

npm hangs at custom jsdom install paperjs/paper.js#957

Closed

domenic added the needs tests label Feb 17, 2016

lehni force-pushed the 1365-image-loading branch 2 times, most recently from 370e8f0 to 9326a2c Compare March 28, 2016 03:36

Fix jsdom#1365: Support image loading through node-canvas module.

86d9470

Also adds support for: - HTMLImageElement.naturalWidth, naturalHeight, complete, currentSrc - HTMLCanvasElement.drawImage() - HTMLCanvasElement.toBlob()

lehni force-pushed the 1365-image-loading branch from 9326a2c to 86d9470 Compare March 28, 2016 04:23

domenic removed the needs tests label Apr 3, 2016

domenic reviewed Apr 3, 2016
View reviewed changes

domenic closed this Apr 3, 2016

lehni mentioned this pull request Apr 5, 2016

Setting src on image should kick off request #189

Closed

lehni added a commit to paperjs/paper.js that referenced this pull request Apr 5, 2016

Switch to jsdom v8.3.0, now that it includes our node-canvas PR.

f4e4e7a

See jsdom/jsdom#1366

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1365: Support image loading through node-canvas module. #1366

Fix #1365: Support image loading through node-canvas module. #1366

lehni commented Jan 25, 2016

lehni commented Jan 26, 2016

domenic commented Jan 27, 2016

domenic Jan 30, 2016

lehni Jan 30, 2016

Joris-van-der-Wel Jan 30, 2016

lehni Jan 30, 2016

lehni Jan 30, 2016

lehni Jan 30, 2016

lehni commented Feb 8, 2016

domenic commented Feb 8, 2016

lehni commented Feb 8, 2016

domenic commented Feb 8, 2016

lehni commented Mar 14, 2016

domenic commented Mar 14, 2016

lehni commented Mar 14, 2016

lehni commented Mar 14, 2016

lehni commented Mar 27, 2016

domenic commented Mar 27, 2016

lehni commented Mar 27, 2016

domenic commented Mar 27, 2016

lehni commented Mar 27, 2016

lehni commented Mar 27, 2016

domenic commented Mar 27, 2016

lehni commented Mar 27, 2016

lehni commented Mar 27, 2016

domenic commented Mar 27, 2016

lehni commented Mar 28, 2016

lehni commented Mar 31, 2016

domenic commented Mar 31, 2016

lehni commented Mar 31, 2016

domenic Apr 3, 2016

domenic Apr 3, 2016

domenic commented Apr 3, 2016

lehni commented Apr 4, 2016

Fix #1365: Support image loading through node-canvas module. #1366

Fix #1365: Support image loading through node-canvas module. #1366

Conversation

lehni commented Jan 25, 2016

lehni commented Jan 26, 2016

domenic commented Jan 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lehni commented Feb 8, 2016

domenic commented Feb 8, 2016

lehni commented Feb 8, 2016

domenic commented Feb 8, 2016

lehni commented Mar 14, 2016

domenic commented Mar 14, 2016

lehni commented Mar 14, 2016

lehni commented Mar 14, 2016

lehni commented Mar 27, 2016

domenic commented Mar 27, 2016

lehni commented Mar 27, 2016

domenic commented Mar 27, 2016

lehni commented Mar 27, 2016

lehni commented Mar 27, 2016

domenic commented Mar 27, 2016

lehni commented Mar 27, 2016

lehni commented Mar 27, 2016

domenic commented Mar 27, 2016

lehni commented Mar 28, 2016

lehni commented Mar 31, 2016

domenic commented Mar 31, 2016

lehni commented Mar 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Apr 3, 2016

lehni commented Apr 4, 2016