The default bug view has changed. See this FAQ.

Ctrl-plus and Ctrl-minus should scale directly-viewed images even when otherwise using "zoom text only"

RESOLVED FIXED in mozilla12

Status

()

Core
General
--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Josh Triplett, Assigned: Timothy Zhu)

Tracking

unspecified
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [goodfirstbug])

Attachments

(1 attachment, 1 obsolete attachment)

1.96 KB, patch
Gavin
: review+
sicking
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
(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

6 years ago
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

6 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"?
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

6 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

6 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

6 years ago
Created attachment 573752 [details] [diff] [review]
Patch
Attachment #573752 - Flags: review?

Comment 7

6 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

5 years ago
review ping?
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-
Timothy, the second link I provided in comment 5 is where mouse zooming is handled.
(Assignee)

Comment 11

5 years ago
Created attachment 575104 [details] [diff] [review]
Patch

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

5 years ago
review ping?
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.
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 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

5 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+
http://hg.mozilla.org/integration/mozilla-inbound/rev/043c16767e33

Thanks for the patch, Timothy! Sorry it took so long from start to finish.
https://hg.mozilla.org/mozilla-central/rev/043c16767e33
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12

Updated

5 years ago
Assignee: nobody → nattofriends
Keywords: helpwanted
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

5 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.