Closed
Bug 686514
Opened 13 years ago
Closed 12 years ago
Ctrl-plus and Ctrl-minus should scale directly-viewed images even when otherwise using "zoom text only"
Categories
(Core :: General, enhancement)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: josh, Assigned: nattofriends)
Details
(Whiteboard: [goodfirstbug])
Attachments
(1 file, 1 obsolete file)
1.96 KB,
patch
|
Gavin
:
review+
sicking
:
review+
|
Details | Diff | Splinter Review |
(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".
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: https://addons.mozilla.org/en-US/firefox/addon/image-zoom/
Reporter | ||
Comment 2•13 years ago
|
||
"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•13 years ago
|
||
I think the suggested change makes sense for image documents. Should be an easy fix.
Severity: normal → enhancement
Component: Layout: Images → General
Keywords: helpwanted
OS: Linux → All
QA Contact: layout.images → general
Hardware: x86_64 → All
Whiteboard: [goodfirstbug]
Version: Other Branch → unspecified
Assignee | ||
Comment 4•13 years ago
|
||
Hi, I'm interested in trying to fix this bug. Would you mind telling me which files or classes should I start looking in?
Comment 5•13 years ago
|
||
I believe the relevant file is http://mxr.mozilla.org/mozilla-central/source/toolkit/content/viewZoomOverlay.js#91, and perhaps http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#2517 as well.
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #573752 -
Flags: review?
Comment 7•13 years ago
|
||
Comment on attachment 573752 [details] [diff] [review] Patch Nice work! I think Gavin is a reasonable review for this.
Attachment #573752 -
Flags: review? → review?(gavin.sharp)
Assignee | ||
Comment 8•13 years ago
|
||
review ping?
Comment 9•13 years ago
|
||
Comment on attachment 573752 [details] [diff] [review] Patch I tested the patch on OSX, it works when zooming with keyboard commands (CMD++/-) but not when using scroll zooming (CTRL+scroll).
Attachment #573752 -
Flags: feedback-
Comment 10•13 years ago
|
||
Timothy, the second link I provided in comment 5 is where mouse zooming is handled.
Assignee | ||
Comment 11•13 years ago
|
||
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 (https://bugzilla.mozilla.org/show_bug.cgi?id=462892#c16), so I'm not too sure why that's implemented in nsDocument and not nsIDocument...
Attachment #573752 -
Attachment is obsolete: true
Attachment #575104 -
Flags: review?(gavin.sharp)
Attachment #573752 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 12•13 years ago
|
||
review ping?
Comment 13•13 years ago
|
||
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 https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F , 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•13 years ago
|
||
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•13 years ago
|
||
Comment on attachment 575104 [details] [diff] [review] Patch Please also get review from a DOM peer on the content/ changes.
Attachment #575104 -
Flags: review?(gavin.sharp) → review+
Updated•13 years ago
|
Attachment #575104 -
Flags: review?(jonas)
Comment on attachment 575104 [details] [diff] [review] Patch Review of attachment 575104 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the content/ changes.
Attachment #575104 -
Flags: review?(jonas) → review+
Comment 17•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/043c16767e33 Thanks for the patch, Timothy! Sorry it took so long from start to finish.
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/043c16767e33
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Updated•12 years ago
|
Assignee: nobody → nattofriends
Keywords: helpwanted
Updated•12 years ago
|
Summary: Ctrl-plus and Ctrl-minus should scale directly-viewed images even when otherwise using "zoom text only" → Ctrl++ and Ctrl+- should scale directly-viewed images even when otherwise using "zoom text only"
Reporter | ||
Updated•12 years ago
|
Summary: Ctrl++ and Ctrl+- should scale directly-viewed images even when otherwise using "zoom text only" → Ctrl-plus and Ctrl-minus should scale directly-viewed images even when otherwise using "zoom text only"
You need to log in
before you can comment on or make changes to this bug.
Description
•