Bug 26908 – <canvas> without CSS width/height is unaffected by full page zoom

• Component: New Bugs
• Status: RESOLVED FIXED

[Simo Kinnunen]

Canvas, if defined as follows, does not respect full page zoom:

<canvas width="100" height="100"></canvas>

But the following does:

<canvas width="100" height="100" style="width: 100px; height: 100px"></canvas>

You'd expect both cases to behave the same. Gecko and Presto respect the zoom level (i.e. it's pretty much identical to <img>).

[George Wright]

Created attachment 34567 [details]
Fix <canvas> zooming

This patch makes the canvas element parser add CSS properties for the width and height in the same manner as the image element parser does, thus allowing the canvas to scale properly. It also removes a superfluous zoomedSize declaration which is never used.

[Dave Hyatt]

Comment on attachment 34567 [details]
Fix <canvas> zooming

I think it's better to patch the initial intrinsic size on the rendering side.  I don't think you have to touch the DOM element.  This patch is flawed since it doesn't zoom omitted attributes (that should default to 300 and 150 but then be zoomed).

Need a test case also obviously.

[George Wright]

Created attachment 34609 [details]
Canvas zooming fix using intrinsic coordinates

New patch which updates the intrinsic coordinates instead of adding to the CSS properties. This also fixes the intrinsic coordinates not being updated for an initial style setting to a renderer.

[Dave Hyatt]

Comment on attachment 34609 [details]
Canvas zooming fix using intrinsic coordinates

Everything looks fine to me except for the if (!parent()) check in RenderReplaced::layout().  It's not clear to me why you would need that...are you sure you didn't just need to patch the intrinsicSizeChanged method for other replaced elements in a fashion similar to what you did in RenderHTMLCanvas?

[George Wright]

Created attachment 34618 [details]
Removed unnecessary null pointer check

You're right - that null pointer check was an artifact left over from testing. It works fine without it. I have also added the new test which I forgot to put in my last patch.

[Dave Hyatt]

Comment on attachment 34618 [details]
Removed unnecessary null pointer check

You are sure that other types of replaced elements aren't going to crash without a parent null check somewhere?  I'd recommend browsing in a zoomed window for a bit to make sure.

[George Wright]

(In reply to comment #6)
> (From update of attachment 34618 [details])
> You are sure that other types of replaced elements aren't going to crash
> without a parent null check somewhere?  I'd recommend browsing in a zoomed
> window for a bit to make sure.

Seems fine here. I tested it with images and media (from http://webkit.org/blog/140/html5-media-support/) and it didn't crash when zoomed.

[Dave Hyatt]

Comment on attachment 34618 [details]
Removed unnecessary null pointer check

r=me

[Eric Seidel (no email)]

Comment on attachment 34618 [details]
Removed unnecessary null pointer check

Clearing flags on attachment: 34618

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/canvas/canvas-zoom.html
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderHTMLCanvas.cpp
	M	WebCore/rendering/RenderReplaced.cpp
Committed r47172
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderHTMLCanvas.cpp
	M	WebCore/rendering/RenderReplaced.cpp
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/canvas/canvas-zoom.html
r47172 = 02e5f59486ffcc081f9d82d6bc99a675c2fbe394 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47172

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Mark Rowe (bdash)]

This change caused tables/mozilla/bugs/bug2479-4.html to start failing on the Tiger buildbot.  Is that expected?  See <http://build.webkit.org/results/Tiger%20Intel%20Release/r47194%20(3443)/tables/mozilla/bugs/bug2479-4-pretty-diff.html> for the diff.

[Eric Seidel (no email)]

Given the number of troubles we've had with this bug, I must have run "bugzilla-tool land-diff --no-build" manually for some reason.  I would not have expected the commit-queue to land a patch with missing results or failing tests.  I'm happy to roll out this patch given the troubles we've seen with it.  (Email from Darin Adler to authors, missing test results which I've checked in, and now a failing test.)

[George Wright]

(In reply to comment #12)
> Given the number of troubles we've had with this bug, I must have run
> "bugzilla-tool land-diff --no-build" manually for some reason.  I would not
> have expected the commit-queue to land a patch with missing results or failing
> tests.  I'm happy to roll out this patch given the troubles we've seen with it.
>  (Email from Darin Adler to authors, missing test results which I've checked
> in, and now a failing test.)

I think the email from Darin was a misunderstanding (see my email). The patch that was landed was the one that David Hyatt r+d.

We're still looking into the problem with the layout test on Intel. I'm not convinced this patch is the outright cause as it has not broken that test on every platform, which suggests it may have uncovered another bug somewhere.

[Eric Seidel (no email)]

We still have one failing layout test from this change on Tiger.  Unless we have a fix in the pipeline, I recommend we roll out this change to make the Tiger bot green.

[Eric Seidel (no email)]

Reopening since there was a regression on the bots:
http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/3425

[George Wright]

(In reply to comment #15)
> Reopening since there was a regression on the bots:
> http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/3425

It seems to be working again; did you roll out the patch or not? I can't see in the logs that you did.

[Eric Seidel (no email)]

http://trac.webkit.org/changeset/47191 was added to fix the qt bot.

I did not roll your change out.  I'm not sure why the bot results changed. I have not investigated.

[Dirk Schulze]

The build-bots are green. If no one rolled out the patch, can we close the bug now?