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

VERIFIED FIXED

Status

MailNews Core
MIME
--
enhancement
VERIFIED FIXED
11 years ago
3 months ago

People

(Reporter: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com], Assigned: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com])

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: relnote-seamonkey1.5a)

Attachments

(3 attachments, 3 obsolete attachments)

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.
Created attachment 256766 [details] [diff] [review]
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.
Attachment #256766 - Flags: review?(bienvenu)
<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

11 years ago
Neil is a much better person to ask about this
Created attachment 256878 [details] [diff] [review]
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.
Attachment #256766 - Attachment is obsolete: true
Attachment #256878 - Flags: review?(neil)
Attachment #256766 - Flags: review?(bienvenu)

Comment 5

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

11 years ago
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.
Created attachment 256990 [details] [diff] [review]
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.
Attachment #256878 - Attachment is obsolete: true
Attachment #256990 - Flags: review?(neil)
Attachment #256878 - Flags: review?(neil)
(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

11 years ago
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
Attachment #256990 - Flags: review?(neil) → review+
Created attachment 257094 [details] [diff] [review]
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.
Attachment #256990 - Attachment is obsolete: true
Attachment #257094 - Flags: superreview?(bienvenu)
Attachment #257094 - Flags: review?(neil)

Comment 12

11 years ago
Created attachment 258424 [details] [diff] [review]
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.
Attachment #258424 - Flags: superreview?(mscott)

Comment 13

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

sr=mscott for the thunderbird specific change. I think this would be pretty cool.
Attachment #258424 - Flags: superreview?(mscott) → superreview+

Comment 14

11 years ago
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 16

11 years ago
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)?
Attachment #257094 - Flags: review?(neil) → review+

Comment 17

11 years ago
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) {
Attachment #257094 - Flags: superreview?(bienvenu) → superreview+
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?
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: relnote-seamonkey1.5a
Created attachment 259155 [details] [diff] [review]
what i checked in

Comment 20

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

Comment 21

11 years ago
(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.

Comment 23

11 years ago
(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
Status: RESOLVED → VERIFIED
Depends on: 380736

Updated

11 years ago
Duplicate of this bug: 386182

Updated

10 years ago
Duplicate of this bug: 196482

Updated

10 years ago
Duplicate of this bug: 250114

Updated

10 years ago
Duplicate of this bug: 406884

Updated

4 months ago
Blocks: 534083

Comment 29

4 months ago
...and 8 years later... we also get it right when printing :)
Depends on: 549106

Updated

4 months ago
Depends on: 1381969

Updated

3 months ago
Component: MailNews: Message Display → MIME
OS: Windows XP → All
Product: SeaMonkey → MailNews Core
Hardware: x86 → All
Summary: allow image attachments that are displayed inline to be scaled → Allow image attachments that are displayed inline to be scaled [TB+SM]
You need to log in before you can comment on or make changes to this bug.