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>).
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 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.
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 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?
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 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.
(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 on attachment 34618 [details] Removed unnecessary null pointer check r=me
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
All reviewed patches have been landed. Closing bug.
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.
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.)
(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.
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.
Reopening since there was a regression on the bots: http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/3425
(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.
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.
The build-bots are green. If no one rolled out the patch, can we close the bug now?