Last Comment Bug 686514 - Ctrl-plus and Ctrl-minus should scale directly-viewed images even when otherwise using "zoom text only"
: Ctrl-plus and Ctrl-minus should scale directly-viewed images even when otherw...
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
-- enhancement (vote)
: mozilla12
Assigned To: Timothy Zhu
Depends on:
  Show dependency treegraph
Reported: 2011-09-13 12:36 PDT by Josh Triplett
Modified: 2012-04-02 11:51 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (925 bytes, patch)
2011-11-10 22:34 PST, Timothy Zhu
mats: feedback-
Details | Diff | Splinter Review
Patch (1.96 KB, patch)
2011-11-16 23:25 PST, Timothy Zhu review+
jonas: review+
Details | Diff | Splinter Review

Description User image Josh Triplett 2011-09-13 12:36:04 PDT
(Not sure if I've found the right component for this bug.)

I have whole-page zooming / image scaling disabled, so that ctrl-plus and ctrl-minus just change the text size.  (I prefer to see larger text in the same layout, and I don't like the appearance of scaled images.)  However, this means that when I view an image directly (not as part of an HTML page), I can't scale that image.  When viewing an image directly, I'd like the usual zoom keystrokes to actually zoom the image, even though I normally use "zoom text only".
Comment 1 User image mjh563 2011-09-13 13:04:14 PDT
But surely the "zoom text only" option by definition only zooms text?

You can resize images using the Image Zoom add-on, if that's any help:
Comment 2 User image Josh Triplett 2011-09-13 13:53:15 PDT
"Zoom Text Only" only makes sense in the context of a page which can potentially contain both text and images.  When viewing an image directly, which interpretation makes more sense and provides more useful functionality: "do absolutely nothing" or "zoom the image"?
Comment 3 User image Mats Palmgren (:mats) 2011-09-15 12:58:19 PDT
I think the suggested change makes sense for image documents.
Should be an easy fix.
Comment 4 User image Timothy Zhu 2011-11-06 16:05:24 PST

I'm interested in trying to fix this bug. Would you mind telling me which files or classes should I start looking in?
Comment 6 User image Timothy Zhu 2011-11-10 22:34:02 PST
Created attachment 573752 [details] [diff] [review]
Comment 7 User image Josh Matthews [:jdm] 2011-11-10 23:53:00 PST
Comment on attachment 573752 [details] [diff] [review]

Nice work! I think Gavin is a reasonable review for this.
Comment 8 User image Timothy Zhu 2011-11-15 13:27:29 PST
review ping?
Comment 9 User image Mats Palmgren (:mats) 2011-11-15 14:21:16 PST
Comment on attachment 573752 [details] [diff] [review]

I tested the patch on OSX, it works when zooming with keyboard commands
(CMD++/-) but not when using scroll zooming (CTRL+scroll).
Comment 10 User image Josh Matthews [:jdm] 2011-11-15 14:34:48 PST
Timothy, the second link I provided in comment 5 is where mouse zooming is handled.
Comment 11 User image Timothy Zhu 2011-11-16 23:25:46 PST
Created attachment 575104 [details] [diff] [review]

In order to find whether the document is synthetic, I added a method to nsIDocument to get mIsSyntheticDocument. I know nsDocument::GetMozSyntheticDocument exists, but it makes sense for a getter to be in nsIDocument (, so I'm not too sure why that's implemented in nsDocument and not nsIDocument...
Comment 12 User image Timothy Zhu 2011-11-20 15:42:21 PST
review ping?
Comment 13 User image :Gavin Sharp [email:] 2011-11-23 16:13:59 PST
Thanks for the patch, Timothy, and sorry for the delay in responding. One thing that would help in the future is making sure that your .hgrc has the "[diff]" options from , just so that the generated diffs have a bit more context.

I noticed one issue with the patch as-is: it makes the View->Zoom->"Zoom Text Only" menu option not function properly on synthetic documents (you can toggle it but it doesn't have any effect on zooming behavior). I think ideally we would disable the menu option on such pages, by having FullZoom.updateMenu (defined in browser-fullZoom.js) check gBrowser.contentDocument.mozSyntheticDocument and set disabled accordingly.
Comment 14 User image :Gavin Sharp [email:] 2011-11-23 16:20:32 PST
Hmm, though actually, I guess it might be weird for a global toggle to be enabled/disabled based on the the current page. Maybe it's best to just leave the patch as-is.
Comment 15 User image :Gavin Sharp [email:] 2011-12-02 11:59:30 PST
Comment on attachment 575104 [details] [diff] [review]

Please also get review from a DOM peer on the content/ changes.
Comment 16 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-01-23 23:01:26 PST
Comment on attachment 575104 [details] [diff] [review]

Review of attachment 575104 [details] [diff] [review]:

r=me on the content/ changes.
Comment 17 User image Josh Matthews [:jdm] 2012-01-24 00:10:02 PST

Thanks for the patch, Timothy! Sorry it took so long from start to finish.
Comment 18 User image Matt Brubeck (:mbrubeck) 2012-01-24 10:41:30 PST

Note You need to log in before you can comment on or make changes to this bug.