It's annoying when large images are attached to an email and they can't be viewed well because they don't fit on screen. It would be nice to be able to shrink them.

This is a quick and dirty patch that shows how I am currently expecting to implement this. -We may want a pref to shrink by default (though I don't currently see the use of *not* shrinking by default). -The functionality needs to be keyboard-accessible. -The while loop sucks right now. -if (getAttr) shoudl probably be if (hasAttr). -Some CSS is required to show zoom in/out cursors like we currently do for standalone images in the browser. -I don't know if we should scale to fit both height and width, or just width. I suspect the former.

<NeilAway> CTho: what does that do on small images? also, imho I think we need a better way to detect an inline image <NeilAway> also, what if you resize the width of the message?

Neil is a much better person to ask about this

-We may want a pref to shrink by default (though I don't currently see the use of *not* shrinking by default). -The functionality needs to be keyboard-accessible. -I don't know if we should scale to fit both height and width, or just width. I suspect the former. -When I move the splitter between the preview pane and the message list, the image unscales itself and I don't understand why. -The regex for detecting attachments is not the right way to do this. -It should probably verify that the image loaded successfully.

Perhaps you can reuse the resize pref from Appearance. Just generalize it to be non-browser specific. (I've often thought that pref should be brought out as a View option.) I know it's out of scope for this bug, but sometimes I'd like to be able to scale up images that, for example, have writing that is too small to read.

Comment on attachment 256878 [details] [diff] [review] less quick hack (not for checkin) >+ if (targ.getAttribute("src").replace(/.*file/, "") >+ == attachments[i].getAttribute("attachmentUrl").replace(/.*file/, "")) { I'm not sure what this is trying to do... you probably want the currentAttachments array rather than fiddling around with attributes. >+ // ok, we're on an attachment. toggle shrink. >+ if (targ.hasAttribute("width")) { >+ targ.className = "attached-image-unscaled"; >+ targ.removeAttribute("width"); >+ } >+ else if (targ.naturalWidth > targ.ownerDocument.width) { >+ targ.className = "attached-image-scaled"; >+ targ.setAttribute("width", "100%"); >+ targ.style.maxWidth = targ.naturalWidth + "px"; >+ } Can't you style the width in the CSS?

(In reply to comment #6) > (From update of attachment 256878 [details] [diff] [review]) > >+ if (targ.getAttribute("src").replace(/.*file/, "") > >+ == attachments[i].getAttribute("attachmentUrl").replace(/.*file/, "")) { > I'm not sure what this is trying to do... you probably want the > currentAttachments array rather than fiddling around with attributes. I'll look into it. > >+ // ok, we're on an attachment. toggle shrink. > >+ if (targ.hasAttribute("width")) { > >+ targ.className = "attached-image-unscaled"; > >+ targ.removeAttribute("width"); > >+ } > >+ else if (targ.naturalWidth > targ.ownerDocument.width) { > >+ targ.className = "attached-image-scaled"; > >+ targ.setAttribute("width", "100%"); > >+ targ.style.maxWidth = targ.naturalWidth + "px"; > >+ } > Can't you style the width in the CSS? > As far as I can tell, that doesn't preserve the aspect ratio properly.

-The functionality needs to be keyboard-accessible. -I think just scaling to fit the width is reasonable. -When I move the splitter between the preview pane and the message list, the image scales itself and I don't understand why. -The regex for detecting attachments is not the right way to do this. Where does &type=image/jpeg come from? -It should probably verify that the image loaded successfully.

(In reply to comment #5) > Perhaps you can reuse the resize pref from Appearance. Just generalize it to > be non-browser specific. (I've often thought that pref should be brought out > as a View option.) Unfortunately that pref is browser.*. I don't know if that could/should be changed.

Comment on attachment 256990 [details] [diff] [review] less less quick hack >+ rv = prefSvc->GetBranch("", getter_AddRefs(prefBranch)); Nit: rv unused here You'd have to ask bienvenu for sure but I would have thought there would be some other prefs around that you can use. >+ if (targ.getAttribute("src").replace(/&type=.*?&/, "&") == currentAttachments[i].url) { Nit: convert your src to a url once, then compare it in the loop. (Is it worth just checking the className?) >Index: mozilla/themes/modern/messenger/messageBody.css Don't forget to patch classic too :-P

-Not keyboard accessible. As it turns out, the existing context menu currently isn't anyway... -When I move the splitter between the preview pane and the message list, the image unscales itself (or scales itself, if you toggle the pref the other way) and I don't understand why. Does the document get regenerated? > You'd have to ask bienvenu for sure but I would have thought there would be some other prefs around that you can use. I believe they're all parts of objects I don't have handy at that point. > (Is it worth just checking the className?) My first reaction was "no, not safe", but then I realized that if an HTML email uses one of our class names, we style them anyway... so this is probably fine.

I made this work (I think) for Thunderbird. I made it so only a left click toggles the scaling. I verified that a large image came up scaled and clicking the + cursor over the image restored it to full size, and - scaled it back. I'll wait for Neil's r= before sr'ing - and I'm asking Scott for sr for TB, to see if he's interested in this feature...if he's not, we'd need to change the default for the pref, either by using all-thunderbird.js, or using a SM-specific .js file to set it to true.

Comment on attachment 258424 [details] [diff] [review] thunderbird-specific changes sr=mscott for the thunderbird specific change. I think this would be pretty cool.

yeah, I like it - I just got an e-mail with a large jpeg recently, and it's a much better experience w/ the new code. Thx, Chris!

(In reply to comment #12) > Created an attachment (id=258424) [details] > I made it so only a left click toggles the scaling. That was an unintentional oversight in my patch. Pretend mine was + if (!ceParams && !event.button) { instead of + if (!ceParams) {

Comment on attachment 257094 [details] [diff] [review] patch >+ nsCOMPtr<nsIPrefService> prefSvc(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv)); Nit: passing rv to do_GetService but not using it. >+ if(prefSvc) Nit: needs a space between if and ( > if (!mid->istream) > return nsCRT::strdup("<P><CENTER><IMG SRC=\"resource://gre/res/network/gopher-image.gif\" ALT=\"[Image]\"></CENTER><P>"); Nit: it's not until after this point that you actually need prefix, so don't bother getting prefs etc. until now. (PRBool resize = PR_FALSE; is OK where it is because the compiler can optimise it to when you first use it.) >+ var targ = event.target; Nit: var target please. >+ const Ci = Components.interfaces; Nit: not local style. >+ if (targ.className == "attached-image-scaled") >+ targ.className = "attached-image-unscaled"; >+ else if (targ.className == "attached-image-unscaled") >+ targ.className = "attached-image-scaled"; Hmm... should these be moz-attached-image-* (to reduce the chance of conflicts)?

Comment on attachment 257094 [details] [diff] [review] patch sr=bienvenu, modulo Neil's comments, and the issue I raised earlier about + if (!ceParams && !event.button) {

Checked in my patch. I didn't check in the TB patch b/c it's not mine. Do we want a follow-up bug for scrolling to where you clicked, like nsImageDocument does, or is that less relevant / confusing here?

TB-specific part checked into the trunk. Thx again, Chris, for driving this.

(In reply to comment #18) >Do we want a follow-up bug for scrolling to where you clicked That might be harder because the image isn't the only content.

(In reply to comment #21) > (In reply to comment #18) > >Do we want a follow-up bug for scrolling to where you clicked > That might be harder because the image isn't the only content. > Yes, that was my concern. It's no longer clear where the scroll should end up when you reshrink the image.

(In reply to comment #20) > TB-specific part checked into the trunk. Thx again, Chris, for driving this. > Works fine with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070321 Thunderbird/3.0a1 ID:2007032103 [cairo] Thanks Chris/David (personally it would be great if this were extended to inline images) One small caveat, the feature seems to always be enabled. Toggling mail.enable_automatic_image_resizing", true with ConfigEdit has no effect. Tried a TB restart. It matters little to me but it might for some.

> (In reply to comment #20) > One small caveat, the feature seems to always be enabled. > Toggling mail.enable_automatic_image_resizing", true with ConfigEdit has no > effect. > Tried a TB restart. It matters little to me but it might for some. Note the pref controls the initial state only, the resizability is always enabled. (I also toggled the browser.enable_automatic_image_resizing first by accident when testing, oops...) v trunk tbird 2007-03-22-14Z linux

...and 8 years later... we also get it right when printing :)