534083 - Provide image scaling for embedded images

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Dale]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.5) Gecko/20091204 Thunderbird/3.0

It's pretty simple. Sometimes an image in the e-mail viewing window actually scales down horizontally to the size of the window and gives you the option to click on it to view at 100% zoom. Sometimes it does not... more often not.

Makes for a pretty unreliable feature.

Reproducible: Sometimes

Steps to Reproduce:
1. Open an e-mail with a large (too big to fit window) image.

2. Get lucky and it might resize to fit the window horizontally.

3. Try clicking on the e-mail over and over again, resize the application window, call it names, etc.


Actual Results:  
Give up and just open in Photoshop or other external previewer because it probably won't resize but you might get lucky.

Expected Results:  
What it is suppose to do.... resize the image horizontally to fit the e-mail viewing area.

I just downloaded this and I am using the previous account info from Thunderbird 2.

Comment 1

[Ludovic Hirlimann [:Usul]]

Can you share one email with us that behaves that way ?

Do you have the issue in -safe-mode (http://kb.mozillazine.org/Safe_mode) ?

When this happens , are there any error messages in Tools -> Error console ?

Comment 2

[Dale]

No error messages, still happening in safe mode. I am sending one that is doing it right now to: [e-mail].

Comment 3

[Dale]

This comes up on the SENT message that I just sent.... it comes up 8 times at once (I am in safe mode):



Warning: Cannot specify value for internal property.  Error in parsing value for '-x-system-font'.  Declaration dropped.
Source File: mailbox:///Users/station4/Library/Thunderbird/Profiles/pp05zwlm.default/Mail/Local%20Folders/Sent?number=-1965717595
Line: 0

Comment 4

[Dale]

For some reason I get no errors on original e-mail

Comment 5

[Ludovic Hirlimann [:Usul]]

I have receied a forwarded email which is forwarded.

Html code of it is :
<table class="moz-email-headers-table" border="0" cellpadding="0"
 cellspacing="0">
  <tbody>
    <tr>
      <th align="RIGHT" nowrap="nowrap" valign="BASELINE">Subject: </th>
      <td>Fwd: willows ad</td>
    </tr>
    <tr>
      <th align="RIGHT" nowrap="nowrap" valign="BASELINE">Date: </th>
      <td>Thu, 3 Dec 2009 12:20:13 -0600</td>
    </tr>
    <tr>
      <th align="RIGHT" nowrap="nowrap" valign="BASELINE">From: </th>
      <td>Judy <a class="moz-txt-link-rfc2396E" href="mailto:[e-mail]">&lt;[e-mail]&gt;</a></td>
    </tr>
    <tr>
      <th align="RIGHT" nowrap="nowrap" valign="BASELINE">To: </th>
      <td>Courier Journal Art Department

<a class="moz-txt-link-rfc2396E" href="mailto:[e-mail]">&lt;[e-mail]&gt;</a></td>
    </tr>
  </tbody>
</table>
<br>
<br>
<br>
<div><br>
<div>Begin forwarded message:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
  <div style="margin: 0px;"><font
 style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; font-size: 12px; line-height: normal; font-size-adjust: none; font-stretch: normal; -x-system-font: none; color: rgb(0, 0, 0);"
 color="#000000" face="Helvetica" size="3"><b>From: </b></font><font
 style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; font-size: 12px; line-height: normal; font-size-adjust: none; font-stretch: normal; -x-system-font: none;"
 face="Helvetica" size="3">Heather Fuller &lt;<a moz-do-not-send="true"
 href="mailto:[e-mail]">[e-mail]</a>&gt;</font></div>
  <div style="margin: 0px;"><font
 style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; font-size: 12px; line-height: normal; font-size-adjust: none; font-stretch: normal; -x-system-font: none; color: rgb(0, 0, 0);"
 color="#000000" face="Helvetica" size="3"><b>Date: </b></font><font
 style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; font-size: 12px; line-height: normal; font-size-adjust: none; font-stretch: normal; -x-system-font: none;"
 face="Helvetica" size="3">December 3, 2009 10:19:08 AM CST</font></div>
  <div style="margin: 0px;"><font
 style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; font-size: 12px; line-height: normal; font-size-adjust: none; font-stretch: normal; -x-system-font: none; color: rgb(0, 0, 0);"
 color="#000000" face="Helvetica" size="3"><b>To: </b></font><font
 style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; font-size: 12px; line-height: normal; font-size-adjust: none; font-stretch: normal; -x-system-font: none;"
 face="Helvetica" size="3"><a moz-do-not-send="true"
 href="mailto:[e-mail]">[e-mail]</a></font></div>
  <div style="margin: 0px;"><font
 style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; font-size: 12px; line-height: normal; font-size-adjust: none; font-stretch: normal; -x-system-font: none; color: rgb(0, 0, 0);"
 color="#000000" face="Helvetica" size="3"><b>Subject: </b></font><font
 style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; font-size: 12px; line-height: normal; font-size-adjust: none; font-stretch: normal; -x-system-font: none;"
 face="Helvetica" size="3"><b>willows ad</b></font></div>
  <div style="margin: 0px; min-height: 14px;"><br>
  </div>
Here's the pic for the ad. <br>
Thanks<br>
Amber<br>
The Willows Day Spa<br>
  <img src="cid:[e-mail]"></blockquote>

</div>
<br>
<br>
</body>
</html>

Comment 6

[Joe Sabash [:JoeS1]]

A little background:
That feature was added in bug 372080 and was designed to work for attached
images only, and not inline images.
The example mail is using an inline image, which has never used re-scaling for me.
(see my comments in that bug)

You can see the feature in action if you send yourself a large attached image.
There is also a hidden pref involved (browser.enable_automatic_image_resizing)
which seems to control the initial state of the re-scale.

So, unless someone would like to use this bug to extend the feature to inline
images, this bug should be resolved as invalid.

Comment 7

[Dale]

It really needs to be implemented as a feature then if possible because in real world use it seems like the feature doesn't work correctly.

For instance where I work I receive e-mail from many clients and if only some of them send pictures as actual attachments then it makes the feature kind of pointless since I am going to have to open Photoshop or Preview anyway for the other "inline" attachments.

But some is better than none I suppose.

Anyway, thank you for the prompt responses to my "bug" report.

Comment 8

[Joe Sabash [:JoeS1]]

I take your comments as permission to change this bug to an enhancement request.

Summary:
There is no easy way to view an over-sized image that has been inserted into an HTML composition.
Bug 372080 provided that for images sent as attachments, but I would argue that
if a new user is sending an HTML message, what more discoverable way is there
than to well, Insert Image.

I think that a right-click context menu item would suffice.

Comment 13

[ron rosenthal]

It also happens in Seamonkey.  2.1 works fine (earlier versions worked too), anything higher  than 2.1 doesn't work at all. Since it has been working for a long time, this doesn't sound like an "enhancement"
how has the code changed for this since 2.1?

Comment 14

[alta88]

Auto shrink to fit should be the default for all <img> elements, and should behave just like inline image attachments do currently when overflowing the messagepane/window.  Brief spec is:

1. The current restriction of this functionality on <img> due to class = moz-attached-image would be removed.
2. Current moz-attached-image css would apply to all <img>.
3. The pref "mail.enable_automatic_image_resizing" would be removed (as would associated code in mimemoz2.cpp).  It's easy enough to click to get original size.
4. This would apply to all <img> in mail, nntp, feed (summary only, not web page) content.

Blake, if you agree with the above I can do a patch.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Sounds good to me!  Be careful that we don't expand images that don't take up the full width, though.  :)

Thanks,
Blake.

Comment 16

[alta88]

The existing methods already handle that.  The one thing is link images; I think in messagepane it's more relevant to size the image.  So if an image is in zoom mode, a right click context will be needed to Open in Browser (if it's not autoshrunk, it will behave as a usual link click).

Comment 17

[alta88]

Attachment: imageScaling.patch

Comment 18

[alta88]

Attachment: imageScaling.patch

some tweaks.

Comment 19

[alta88]

Attachment: imageScaling.patch

some images may hardcode a height attribute, use height: auto to ensure proportional scaling.

Comment 20

[Joe Sabash [:JoeS1]]

Thanks for doing this. I ran with this in a local build and tested a variety of inline images. Worked fine except for one little nit. If you have an image with a natural width high enough to cause overflow, but an inline style that shrinks the image, then the re-size widget appears but is ineffective. Maybe you could avoid that by using "computed style width" instead of naturalwidth to determine if it re-sizable. I'm sure this is an edge case, but could cause confusion for some.

Comment 21

[alta88]

thanks for testing joe.  the solution to the inline style case is not immediately clear; changing naturalWidth to clientWidth solves the problem on initial display, but results in bad behavior on resizing, as clientWidth is alternately reported as either the style width or actual width (seems like a deeper bug) and results in constant scrollbar on/off on each resize event if a true shrink is needed per messagepane size.

Comment 22

[alta88]

Attachment: imageScaling.patch

this should fix inline styles and resizing.  the extreme tester will note that if a resize makes pane and styled image exactly pixel equal, zoom will appear; nothing to be done as if = is used the problem reappears.  also note that !important is added to enforce the shrinktofit (a really stubborn style can add its own).

Comment 23

[alta88]

Attachment: imageScaling.patch

handle the case where the image is already shrunk (width: 100%) to allow zoom in to naturalWidth, to have a consistency of experience regardless of inline css.

Comment 24

[alta88]

ping?

Comment 25

[Blake Winton (:bwinton) (:☕️)]

(Sorry, new computer can't build Thunderbird yet!  I'll try to get to it on the old computer this weekend!)

Comment 26

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 820045 [details] [diff] [review]
imageScaling.patch

Review of attachment 820045 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, so aside from the things I mention below, I like this patch.
r=me, with nits fixed, and question answered!  ;)

Thanks,
Blake.

::: mail/base/content/contentAreaClick.js
@@ +54,5 @@
>    }
>  
>    function messagePaneOnResize(aEvent)
>    {
> +    // Scale any overflowing images, exclude http content.  

Please remove the trailing spaces here.
(And the other places they occur.  :)

@@ +57,5 @@
>    {
> +    // Scale any overflowing images, exclude http content.  
> +    let browser = getBrowser();
> +    let doc = browser && browser.contentDocument ? browser.contentDocument : null;
> +    let imgs = doc && !doc.URL.startsWith("http") ? doc.images : [];

The use of doc.images here is kinda neat!  :)

@@ +63,3 @@
>      {
> +      if ((img.clientWidth + doc.body.clientWidth - doc.body.offsetWidth) >=
> +             doc.body.clientWidth &&

I think it would be easier to check "img.clientWidth - doc.body.offsetWidth >= 0"…  (Removing doc.body.clientWidth from both sides of the inequality.)

@@ +67,5 @@
>        {
> +        if (img.getAttribute("shrinktofit") == "false")
> +          img.setAttribute("overflowing", true);
> +        else
> +          img.setAttribute("isshrunk", true);

I sort of want to combine these two into a single attribute, called something like "toobig".  (Or, you know, something better than that. ;)

::: mail/themes/linux/mail/messageBody.css
@@ +79,4 @@
>    image-orientation: from-image;
>  }
>  
> +img[overflowing="true"] { 

If we merged the attributes, this would change to something along the lines of:
img[toobig="true"]:not([shrinktofit]) {
and the next one would be:
img[toobig="true"][shrinktofit] {

::: mailnews/mailnews.js
@@ +753,3 @@
>  // automatically scale attached images that are displayed inline
>  pref("mail.enable_automatic_image_resizing", true);
>  

Why was this change made?

Comment 27

[alta88]

(In reply to Blake Winton (:bwinton) from comment #26)
> Comment on attachment 820045 [details] [diff] [review]
> imageScaling.patch
> 
> Review of attachment 820045 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Okay, so aside from the things I mention below, I like this patch.
> r=me, with nits fixed, and question answered!  ;)
> 
> Thanks,
> Blake.
> 
> ::: mail/base/content/contentAreaClick.js
> @@ +54,5 @@
> >    }
> >  
> >    function messagePaneOnResize(aEvent)
> >    {
> > +    // Scale any overflowing images, exclude http content.  
> 
> Please remove the trailing spaces here.
> (And the other places they occur.  :)
> 

done.

> @@ +57,5 @@
> >    {
> > +    // Scale any overflowing images, exclude http content.  
> > +    let browser = getBrowser();
> > +    let doc = browser && browser.contentDocument ? browser.contentDocument : null;
> > +    let imgs = doc && !doc.URL.startsWith("http") ? doc.images : [];
> 
> The use of doc.images here is kinda neat!  :)
> 

sure, let the dom devs do the work..

> @@ +63,3 @@
> >      {
> > +      if ((img.clientWidth + doc.body.clientWidth - doc.body.offsetWidth) >=
> > +             doc.body.clientWidth &&
> 
> I think it would be easier to check "img.clientWidth - doc.body.offsetWidth
> >= 0"…  (Removing doc.body.clientWidth from both sides of the inequality.)
> 

"I could have been a polynomial!"

> @@ +67,5 @@
> >        {
> > +        if (img.getAttribute("shrinktofit") == "false")
> > +          img.setAttribute("overflowing", true);
> > +        else
> > +          img.setAttribute("isshrunk", true);
> 
> I sort of want to combine these two into a single attribute, called
> something like "toobig".  (Or, you know, something better than that. ;)
> 
> ::: mail/themes/linux/mail/messageBody.css
> @@ +79,4 @@
> >    image-orientation: from-image;
> >  }
> >  
> > +img[overflowing="true"] { 
> 
> If we merged the attributes, this would change to something along the lines
> of:
> img[toobig="true"]:not([shrinktofit]) {
> and the next one would be:
> img[toobig="true"][shrinktofit] {
> 

done.

> ::: mailnews/mailnews.js
> @@ +753,3 @@
> >  // automatically scale attached images that are displayed inline
> >  pref("mail.enable_automatic_image_resizing", true);
> >  
> 
> Why was this change made?

that pref is only for img attachments inline, used by the mime emitter.  all img get the same treatment now so it's superfluous.  so only suite/ needs it.

by the way, if suite takes this patch, they should remove the pref and also clean up the code here:
http://mxr.mozilla.org/com m-central/source/mailnews/mime/src/mimemoz2.cpp#1217

imo, the class can be kept as it can't hurt to identify img attachments for whoever might need it.

Comment 28

[alta88]

Attachment: imageScaling.patch

updated for comments.

Comment 29

[alta88]

Attachment: imageScaling.patch

tiny tweak to handle broken images.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/e78d12404a63

Comment 31

[Thomas D. (:thomas8)]

Ian, does SeaMonkey want this? Nice feature, simple patch...
Which would allow us to do the cleanup mentioned below...

(In reply to alta88 from comment #27)
> by the way, if suite takes this patch, they should remove the pref and also
> clean up the code here:
> http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.cpp#1217

https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.cpp#1107-1115
...and some other spots related to that.

> imo, the class can be kept as it can't hurt to identify img attachments for
> whoever might need it.

Comment 32

[Ian Neal]

I would say yes

Comment 33

[rsx11m]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #31)
> Ian, does SeaMonkey want this? Nice feature, simple patch...
> Which would allow us to do the cleanup mentioned below...

That's part of bug 939540, just nobody got around yet to come up with a patch.

Comment 34

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #33)
> That's part of bug 939540, just nobody got around yet to come up with a
> patch.

Good catch, thanks, hadn't seen the dependency.