372080 - Allow image attachments that are displayed inline to be scaled [TB+SM]

Status: VERIFIED FIXED

(MailNews Core :: MIME, enhancement)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

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.

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: quick hack (not for checkin)

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.

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

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

Comment 3

[David :Bienvenu]

Neil is a much better person to ask about this

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: less quick hack (not for checkin)

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

Comment 5

[Rich Gray (:rbgray)]

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 6

[[e-mail]]

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?

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

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

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: less less quick hack

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

Comment 9

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(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 10

[[e-mail]]

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

Comment 11

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

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

Comment 12

[David :Bienvenu]

Attachment: thunderbird-specific changes

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 13

[Scott MacGregor]

Comment on attachment 258424 [details] [diff] [review]
thunderbird-specific changes

sr=mscott for the thunderbird specific change. I think this would be pretty cool.

Comment 14

[David :Bienvenu]

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!

Comment 15

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(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 16

[[e-mail]]

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 17

[David :Bienvenu]

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) {

Comment 18

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

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?

Comment 19

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: what i checked in

Comment 20

[David :Bienvenu]

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

Comment 21

[[e-mail]]

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

Comment 22

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

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

Comment 23

[Joe Sabash [:JoeS1]]

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

Comment 24

[Tuukka Tolvanen (sp3000)]

> (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

Comment 29

[Thomas D. (:thomas8)]

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