Bug 26908 - <canvas> without CSS width/height is unaffected by full page zoom
Summary: <canvas> without CSS width/height is unaffected by full page zoom
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://cufon.shoqolate.com/tests/webk...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-01 15:38 PDT by Simo Kinnunen
Modified: 2009-08-21 09:07 PDT (History)
4 users (show)

See Also:


Attachments
Fix <canvas> zooming (2.06 KB, patch)
2009-08-11 09:37 PDT, George Wright
hyatt: review-
Details | Formatted Diff | Diff
Canvas zooming fix using intrinsic coordinates (2.97 KB, patch)
2009-08-11 16:02 PDT, George Wright
hyatt: review-
Details | Formatted Diff | Diff
Removed unnecessary null pointer check (3.32 KB, patch)
2009-08-11 17:20 PDT, George Wright
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simo Kinnunen 2009-07-01 15:38:56 PDT
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>).
Comment 1 George Wright 2009-08-11 09:37:31 PDT
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.
Comment 2 Dave Hyatt 2009-08-11 09:52:59 PDT
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.
Comment 3 George Wright 2009-08-11 16:02:00 PDT
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.
Comment 4 Dave Hyatt 2009-08-11 16:05:42 PDT
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?
Comment 5 George Wright 2009-08-11 17:20:28 PDT
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.
Comment 6 Dave Hyatt 2009-08-12 08:46:37 PDT
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.
Comment 7 George Wright 2009-08-12 08:57:24 PDT
(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.
Comment 8 Dave Hyatt 2009-08-12 14:23:05 PDT
Comment on attachment 34618 [details]
Removed unnecessary null pointer check

r=me
Comment 9 Eric Seidel (no email) 2009-08-12 18:07:54 PDT
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
Comment 10 Eric Seidel (no email) 2009-08-12 18:07:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Mark Rowe (bdash) 2009-08-13 06:06:27 PDT
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.
Comment 12 Eric Seidel (no email) 2009-08-13 08:57:42 PDT
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.)
Comment 13 George Wright 2009-08-13 09:09:26 PDT
(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.
Comment 14 Eric Seidel (no email) 2009-08-13 13:49:34 PDT
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.
Comment 15 Eric Seidel (no email) 2009-08-13 13:50:10 PDT
Reopening since there was a regression on the bots:
http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/3425
Comment 16 George Wright 2009-08-14 08:16:54 PDT
(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.
Comment 17 Eric Seidel (no email) 2009-08-14 08:58:40 PDT
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.
Comment 18 Dirk Schulze 2009-08-21 01:33:50 PDT
The build-bots are green. If no one rolled out the patch, can we close the bug now?