380736 - Inline image attachment resizing (zoom / hourglass) is always enabled, even when useless

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Serge Gautherie (:sgautherie)]

Inline image attachment resizing (zoom / hourglass) is always enabled, even when useless:
the mouse pointer changes from '+' <-> '-', while the image rests unchanged.

This is confusing.

Regressed between
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/20070320 SeaMonkey/1.5a] (2007-03-20-08-trunk) (W2Ksp4)
and
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/20070321 SeaMonkey/1.5a] (2007-03-21-08-trunk) (W2Ksp4)

<http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&sortby=Date&hours=2&date=explicit&mindate=2007-03-20+08&maxdate=2007-03-21+09&cvsroot=%2Fcvsroot>
Likely culprit: [Bug 372080 – allow image attachments that are displayed inline to be scaled].

Read bug 372080 comment 23...

Comment 1

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

Oh, that's not a case I had tested.

Comment 2

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

Attachment: work in progress

Still need to:
1) scale the image on message load, if it doesn't fit.  getElementsByTagName, check for the class name?
2) when you resize the window/splitters, I need to check if each image is overflowing or not.

Comment 3

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

Attachment: patch

This seems to work.  I didn't test messageWindow, just the pane in messenger.

Comment 4

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

(I can do a tbird patch once the fix is OK'd for SeaMonkey)

Comment 5

[David :Bienvenu]

Comment on attachment 269553 [details] [diff] [review]
patch

Thx for doing this, Chris. It looks OK to me, but it's more up to Neil. For TB, I wouldn't want the K&R braces style, especially at the function () level.

Comment 6

[[e-mail]]

Comment on attachment 269553 [details] [diff] [review]
patch

Note: while I was reading this, I thought of a possible alternative: don't change the existing code, but on load and resize simply set an attribute on those images that are overflowing that you key off for the CSS i.e.
.moz-attached-image-unscaled[overflowing="true"] {
  cursor: -moz-zoom-out;
}
.moz-attached-image-scaled[overflowing="true"] {
  cursor: -moz-zoom-in;
  max-width: 100%;
}

>+  var imgs = doc.getElementsByTagName("img");
document.images

>+      if (img.naturalWidth > doc.width) {
Would you mind reversing this test so you can do if else if else if?

>+      if (/^moz-attached-image/.test(target.className)) {
>+        var shouldFit = target.hasAttribute("shrinktofit");
>+        var isShrunk = target.hasAttribute("isshrunk");
>+        var overflowing = target.naturalWidth > target.ownerDocument.width;
>+        if (overflowing && shouldFit) {
>+          if (isShrunk) {
>+            // currently shrunk to fit, so unshrink it
>+            target.removeAttribute("isshrunk");
>+            target.removeAttribute("shrinktofit");
>+            target.style.cursor = "-moz-zoom-out";
>+            target.style.maxWidth = "";
>+          }
>+          else {
>+            // currently full size, so shrink it
>+            target.setAttribute("isshrunk", "yes");
>+            target.setAttribute("shrinktofit", "yes");
>+            target.style.cursor = "-moz-zoom-in";
>+            target.style.maxWidth = "100%";
>+          }
>+        }
>+        else if (overflowing && !shouldFit) {
>+          // user wants to shrink now
>+          target.setAttribute("isshrunk", "yes");
>+          target.setAttribute("shrinktofit", "yes");
>+          target.style.cursor = "-moz-zoom-in";
>+          target.style.maxWidth = "100%";
>+        }
>+      }
I think this is overly complicated. The ImageDocument code works like this:
If the image is shrunk, then unshrink it, and turn off automatic resizing.
Otherwise, turn on automatic resizing, and shrink it to fit if overflowing.

Comment 7

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

Attachment: patch v2

Maybe this is better.

Comment 8

[[e-mail]]

Comment on attachment 273247 [details] [diff] [review]
patch v2

>+        img.setAttribute("overflowing", "yes");
Actually I think we tend to use "true" values for things like this.

>+        var shouldFit = target.hasAttribute("shrinktofit");
You don't seem to use this.

>+        var isShrunk = target.hasAttribute("isshrunk");
>+        var doesntFit = target.naturalWidth > target.ownerDocument.width;
You only use these once, you might as well inline them. Also, can't you use target.hasAttribute("overflowing") here?

>+        if (isShrunk) {
>+          // currently shrunk to fit, so unshrink it
>+          target.removeAttribute("isshrunk");
>+          target.removeAttribute("shrinktofit");
>+          target.setAttribute("overflowing", "yes");
>+        }
>+        else if (doesntFit) {
>+          // user wants to shrink now
>+          target.setAttribute("isshrunk", "yes");
>+          target.setAttribute("shrinktofit", "yes");
>+          target.removeAttribute("overflowing");
>+        }
This doesn't match with what onresize does - it doesn't clear overflowing on images shrunk to fit; you probably remove both overflowing lines here.

Comment 9

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

Attachment: v3 (checked in)

Ok, I made "isshrunk" mutually exclusive with "overflowing" (to avoid dependencies on the ordering within the CSS files).

Comment 10

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

Comment on attachment 274103 [details] [diff] [review]
v3 (checked in)

Checked in for SeaMonkey.  I'm pretty sure I just broke the feature in Thunderbird... so I guess I have to fix that too before resolving this.

Comment 11

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

Attachment: patch for tbird

Comment 12

[Scott MacGregor]

Comment on attachment 275370 [details] [diff] [review]
patch for tbird

thanks Chris!

Comment 13

[jag (Peter Annema)]

Comment on attachment 275370 [details] [diff] [review]
patch for tbird

+  function messagePaneOnResize(event) {

When in Rome ...

Comment 14

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

Attachment: what I checked in for tb

Checked in tbird version; fixed newline-before-brace for both mailnews and tbird.