Closed Bug 534083 Opened 15 years ago Closed 11 years ago

Provide image scaling for embedded images

Categories

(Thunderbird :: Mail Window Front End, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 28.0

People

(Reporter: dale223223, Assigned: alta88)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: [tb31features])

Attachments

(1 file, 6 obsolete files)

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.
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 ?
Component: General → Mail Window Front End
QA Contact: general → front-end
No error messages, still happening in safe mode. I am sending one that is doing it right now to: ludovic@mozillamessaging.com.
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
For some reason I get no errors on original e-mail
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:judy@courierjournal.net">&lt;judy@courierjournal.net&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:artdepartment@courierjournal.net">&lt;artdepartment@courierjournal.net&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:willowsinfo@gmail.com">willowsinfo@gmail.com</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:judy@courierjournal.net">judy@courierjournal.net</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:part1.06020006.02080100@courierjournal.net"></blockquote>

</div>
<br>
<br>
</body>
</html>
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.
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.
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.
Severity: major → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Summary: Thunderbird 3 image scaling doesn't work sometimes. → Provide image scaling for embedded images
Hardware: x86_64 → All
Version: unspecified → Trunk
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?
See Also: → 641438
See Also: → 740772
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.
Flags: needinfo?(bwinton)
Sounds good to me!  Be careful that we don't expand images that don't take up the full width, though.  :)

Thanks,
Blake.
Flags: needinfo?(bwinton)
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).
Attached patch imageScaling.patch (obsolete) — Splinter Review
Assignee: nobody → alta88
Attachment #818013 - Flags: review?(bwinton)
Attached patch imageScaling.patch (obsolete) — Splinter Review
some tweaks.
Attachment #818013 - Attachment is obsolete: true
Attachment #818013 - Flags: review?(bwinton)
Attachment #818107 - Flags: review?(bwinton)
Attached patch imageScaling.patch (obsolete) — Splinter Review
some images may hardcode a height attribute, use height: auto to ensure proportional scaling.
Attachment #818107 - Attachment is obsolete: true
Attachment #818107 - Flags: review?(bwinton)
Attachment #818502 - Flags: review?(bwinton)
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.
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.
Attached patch imageScaling.patch (obsolete) — Splinter Review
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).
Attachment #818502 - Attachment is obsolete: true
Attachment #818502 - Flags: review?(bwinton)
Attachment #819904 - Flags: review?(bwinton)
Attached patch imageScaling.patch (obsolete) — Splinter Review
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.
Attachment #819904 - Attachment is obsolete: true
Attachment #819904 - Flags: review?(bwinton)
Attachment #820045 - Flags: review?(bwinton)
Blocks: TB2SM
ping?
(Sorry, new computer can't build Thunderbird yet!  I'll try to get to it on the old computer this weekend!)
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?
Attachment #820045 - Flags: review?(bwinton) → review+
(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.
Attached patch imageScaling.patch (obsolete) — Splinter Review
updated for comments.
Attachment #820045 - Attachment is obsolete: true
Attachment #830210 - Flags: review+
Keywords: checkin-needed
tiny tweak to handle broken images.
Attachment #830210 - Attachment is obsolete: true
Attachment #830240 - Flags: review+
https://hg.mozilla.org/comm-central/rev/e78d12404a63
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 28.0
Blocks: 777679
Blocks: 939540
Whiteboard: [tb31features]
No longer blocks: 777679
Depends on: 372080
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.
Flags: needinfo?(iann_bugzilla)
Depends on: 1381969
I would say yes
Flags: needinfo?(iann_bugzilla)
Depends on: 1382133
(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.
(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.
You need to log in before you can comment on or make changes to this bug.