1381338 - Remove some unnecessary space in body attachment view

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Richard Marti (:Paenglab)]

This bug is related to bug 699000 but only for the attachment part and not the HTML body part.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: attachmentView.patch

Jörg, what do you think. No need to print something. It's already visible with plain text messages in the message pane and in print preview.

It removes only some space and does not group the attachment title with the attachment that they are always printed on the same page. This I don't know how to do.

Comment 2

[Thomas D. (:thomas8)]

Turns out that whitespace inflation was an HTML syntax mistake in the first place, because the suffix is wrong:
> const char *suffix = "\"></CENTER><P>";
Obviously, this was meant to close the <P> from praefix using </P>, not to add another <P> in suffix, where both of them never get closed afasics...

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8886878 [details] [diff] [review]
attachmentView.patch

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

Fine, but please consider the last comment.

::: mail/themes/windows/mail/messageBody.css
@@ +150,5 @@
>  .mimeAttachmentHeader {
>    border-style: none;
>    border-top: 1px solid GrayText;
> +  padding-block-start: 0.625em;
> +  padding-block-end: 0.2em;

So this reduces the space between the header of an inline attachment and the attachment content. I'm fine with this.

@@ +171,5 @@
>  }
>  
>  .mimeAttachmentTable tr + tr > td {
>    border-top: 1px solid GrayText;
> +  padding-top: 0.25em;

This appears to reduce the space between the attachments summary header "Attachments" and the listed attachments. Fine.

::: mailnews/mime/src/mimei.cpp
@@ +1723,5 @@
>            opt->state->separator_suppressed_p = false;
>            if (lstatus < 0) return lstatus;
>        }
>  
> +      sep = "</FIELDSET>";

Yes, reduces space after attachment title. Fine.

::: mailnews/mime/src/mimemoz2.cpp
@@ +1090,5 @@
>      (mime_image_stream_data *) image_closure;
>  
>    const char *prefix;
>    /* Wouldn't it be nice if attributes were case-sensitive? */
> +  const char *scaledPrefix = "<CENTER><IMG CLASS=\"moz-attached-image\" shrinktofit=\"yes\" SRC=\"";

Yes, removing extra empty paragraphs before and after the image. Fine. I looked at it in the DOM Inspector.

@@ +1100,5 @@
>    PR_ASSERT(mid);
>    if (!mid) return 0;
>  
>    /* Internal-external-reconnect only works when going to the screen. */
>    if (!mid->istream)

Well, what about the line that follows:
  if (!mid->istream)
    return strdup("<P><CENTER><IMG SRC=\"resource://gre-resources/loading-image.png\" ALT=\"[Image]\"></CENTER><P>");

You want to kill those <P>s, too?

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #2)
> Obviously, this was meant to close the <P> from praefix using </P>, not to
> add another <P> in suffix, where both of them never get closed afasics...
<P> is also stand-alone and (obviously) <p><center>huhu</center></p> would be invalid HTML just as <p><div>huhu</div></p> would be invalid.
https://stackoverflow.com/questions/8397852/why-p-tag-cant-contain-div-tag-inside-it

Comment 5

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #4)
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #2)
> > Obviously, this was meant to close the <P> from praefix using </P>, not to
> > add another <P> in suffix, where both of them never get closed afasics...
> <P> is also stand-alone and (obviously) <p><center>huhu</center></p> would
> be invalid HTML just as <p><div>huhu</div></p> would be invalid.
> https://stackoverflow.com/questions/8397852/why-p-tag-cant-contain-div-tag-
> inside-it

So I was (obviously) wrong... ;) Thanks for educating me.

Since <center> is a /deprecated/ block-level element, would this be the time to replace it by something standards-conform, e.g.
> <div text-align:center>...</div>?
And why are we hard-coding this anyway?

Couldn't we just omit the <center> tags and then have the following in messagebody.css:

> .moz-attached-image {
>   display: block;
>   margin: 0 auto;
> }

Comment 6

[Richard Marti (:Paenglab)]

Attachment: attachmentView.patch

(In reply to Jorg K (GMT+2) from comment #3)
> Comment on attachment 8886878 [details] [diff] [review]
> attachmentView.patch
> 
> Review of attachment 8886878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Well, what about the line that follows:
>   if (!mid->istream)
>     return strdup("<P><CENTER><IMG
> SRC=\"resource://gre-resources/loading-image.png\"
> ALT=\"[Image]\"></CENTER><P>");
> 
> You want to kill those <P>s, too?

Yeah, makes sense to be consistent. But I'm not sure when this is shown, during loading from server? And does it change to the final image when it's loaded?

I don't change the <CENTER> as this is also widely used in M-C and I haven't seen a bug to remove the support.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8887118 [details] [diff] [review]
attachmentView.patch

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

::: mailnews/mime/src/mimemoz2.cpp
@@ +1101,5 @@
>    if (!mid) return 0;
>  
>    /* Internal-external-reconnect only works when going to the screen. */
>    if (!mid->istream)
> +    return strdup("<CENTER><IMG SRC=\"resource://gre-resources/loading-image.png\" ALT=\"[Image]\"></CENTER><P>");

Hmm, what about the <P> at the end?

Comment 8

[Richard Marti (:Paenglab)]

Attachment: attachmentView.patch

Removed trailing <P>. Sorry for the additional round.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8887151 [details] [diff] [review]
attachmentView.patch

No problem. Please don't land it now, I'm waiting for a review and will take care of both.

Comment 10

[Richard Marti (:Paenglab)]

Thanks

Comment 11

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #6)
> I don't change the <CENTER> as this is also widely used in M-C and I haven't
> seen a bug to remove the support.
Can I make myself a real nuisance. <center> in not part of HTML5 but of course supported by browsers for compatibility reasons. <center> not not widely used in M-C, in fact, I see seven uses in tests.

While we're here, we should remove the tag and use a <div> as Thomas suggested. Since the class is moz-attached-image, we might do the centring in CSS instead. SM already has CSS for that and we should add: text-align: center for them.

<CENTER><IMG SRC=\"resource://gre-resources/loading-image.png\" ALT=\"[Image]\"></CENTER>
doesn't have a class, so we may add one or use an inline style, ... or leave the <CENTER>.

Comment 12

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #11)
> (In reply to Richard Marti (:Paenglab) from comment #6)
> > I don't change the <CENTER> as this is also widely used in M-C and I haven't
> > seen a bug to remove the support.
> Can I make myself a real nuisance. <center> in not part of HTML5 but of
> course supported by browsers for compatibility reasons. <center> not not
> widely used in M-C, in fact, I see seven uses in tests.

When Jörg and I agree... no chance for Richard ;P

> While we're here, we should remove the tag and use a <div> as Thomas
> suggested. Since the class is moz-attached-image, we might do the centring
> in CSS instead.

Imho, CSS is much cleaner and more versatile than <div>.
As I said, why hard-code without need. Someone might wish to push their pictures to the left, who knows.

> SM already has CSS for that and we should add: text-align:
> center for them.

Assuming that they are also using moz-attached-image class on their <img> tags, using text-align: center on the image may or may not work, but is also invalid syntax, because we're not centering inline elments of the <img>.
I think the cleanest solution is what I suggested above:

> .moz-attached-image {
>   display: block;
>   margin: 0 auto;
> }

> <CENTER><IMG SRC=\"resource://gre-resources/loading-image.png\"
> ALT=\"[Image]\"></CENTER>
> doesn't have a class, so we may add one

With my css above, we can just add CLASS=\"moz-attached-image\" here, isn't it?

> or use an inline style, ... or leave the <CENTER>.

Hahaha... anything goes... from Jörg!?

Let me intervene on his behalf: let's be perfectionist and sustainable, and keep things correct and nicely separated as I suggested above... ;)

Comment 13

[Richard Marti (:Paenglab)]

Attachment: attachmentView.patch

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #12)
> (In reply to Jorg K (GMT+2) from comment #11)
> 
> When Jörg and I agree... no chance for Richard ;P

This smiley made me almost leave the project. ;-)

> > SM already has CSS for that and we should add: text-align:
> > center for them.

Yes, but only instead of our img which was changed from alta88 to let apply the scaling to every image. And like Thomas wrote, text-align: center doesn't work inside the DIV whithout a child.

> I think the cleanest solution is what I suggested above:
> 
> .moz-attached-image {
>   display: block;
>   margin: 0 auto;
> }

I'm using this. :-)
I also added the code to SM to not break them.

> > or use an inline style, ... or leave the <CENTER>.

Ah shoot. I saw this comment after I removed the <CENTER>. :-)

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8887348 [details] [diff] [review]
attachmentView.patch

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

Nice, but needs another round as per comment #11 and comment #12. Or am I missing something?

::: mailnews/mime/src/mimemoz2.cpp
@@ +1101,5 @@
>    if (!mid) return 0;
>  
>    /* Internal-external-reconnect only works when going to the screen. */
>    if (!mid->istream)
> +    return strdup("<DIV><IMG SRC=\"resource://gre-resources/loading-image.png\" ALT=\"[Image]\"></DIV>");

Sorry, but this doesn't work. Options:
1) Put the <CENTER> back.
2) Add a class that has CSS
3) Add an inline style.

Comment 15

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #14)
> Sorry, but this doesn't work.

What doesn't work?

Comment 16

[Jorg K (CEST = GMT+2)]

Well, although we don't know how this code is triggered, I know that if it is triggered, this image isn't centred:
"<DIV><IMG SRC=\"resource://gre-resources/loading-image.png\" ALT=\"[Image]\"></DIV>"
What am I missing?

Comment 17

[Richard Marti (:Paenglab)]

Attachment: attachmentView.patch

Not good to work on such bugs after little sleep and working since 5:00. The previous solution also let the images be cut through on page feed when printing.

Comment 18

[Jorg K (CEST = GMT+2)]

No good working at 2 AM or on (non-)moving trains either :-(

Comment 19

[Thomas D. (:thomas8)]

Comment on attachment 8887348 [details] [diff] [review]
attachmentView.patch

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

This is going the right way. I can't test, but I have some questions from reading the code. I'll be happy to learn.

::: mail/themes/linux/mail/messageBody.css
@@ +176,5 @@
>  }
>  
>  .mimeAttachmentTable tr + tr > td {
>    border-top: 1px solid GrayText;
> +  padding-top: 0.25em;

Why this change? Does it actually change anything? From my reading (pls correct me), before we had 0.25 padding-top and padding-bottom from .mimeAttachmentFile and .mimeAttachmentSize. Now we have 0.25 padding-top from row, and 0.25 padding-bottom from those two classes. Is that better?
If we could introduce a new rule for all rows and get rid of the rule for the two classes, maybe that might simplify things:
.mimeAttachmentTable tr {
padding: 0.25 0;
}
Would that work?
Same for the other copies of messageBody.css.

::: mailnews/mime/src/mimemoz2.cpp
@@ +1091,5 @@
>  
>    const char *prefix;
>    /* Wouldn't it be nice if attributes were case-sensitive? */
> +  const char *scaledPrefix = "<DIV><IMG CLASS=\"moz-attached-image\" shrinktofit=\"yes\" SRC=\"";
> +  const char *unscaledPrefix = "<DIV><IMG CLASS=\"moz-attached-image\" SRC=\"";

Hmmm. I don't understand. Why do we need this <DIV>? Since we are now positioning the image block-style, I thought just <img> would be sufficient?

@@ +1101,5 @@
>    if (!mid) return 0;
>  
>    /* Internal-external-reconnect only works when going to the screen. */
>    if (!mid->istream)
> +    return strdup("<DIV><IMG SRC=\"resource://gre-resources/loading-image.png\" ALT=\"[Image]\"></DIV>");

Let's do 2) please, just add CLASS=\".moz-attached-image\" as I already suggested before.

Comment 20

[Richard Marti (:Paenglab)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #19)
> Comment on attachment 8887348 [details] [diff] [review]
> attachmentView.patch
> 
> Review of attachment 8887348 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is going the right way. I can't test, but I have some questions from
> reading the code. I'll be happy to learn.
> 
> ::: mail/themes/linux/mail/messageBody.css
> @@ +176,5 @@
> >  }
> >  
> >  .mimeAttachmentTable tr + tr > td {
> >    border-top: 1px solid GrayText;
> > +  padding-top: 0.25em;
> 
> Why this change? Does it actually change anything? From my reading (pls
> correct me), before we had 0.25 padding-top and padding-bottom from
> .mimeAttachmentFile and .mimeAttachmentSize. Now we have 0.25 padding-top
> from row, and 0.25 padding-bottom from those two classes. Is that better?

It's to make the first line of the table nearer to the title.

> Hmmm. I don't understand. Why do we need this <DIV>? Since we are now
> positioning the image block-style, I thought just <img> would be sufficient?

No, because the display: block; makes that when printing, an image can be cut through and placed on two pages.

Comment 21

[Thomas D. (:thomas8)]

Comment on attachment 8887373 [details] [diff] [review]
attachmentView.patch

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

Great! One question remaining.

::: mail/themes/linux/mail/messageBody.css
@@ +97,5 @@
>    }
>  }
>  
> +.moz-attached-image-container {
> +  text-align: center;

Nice and easy.

@@ +175,5 @@
>  }
>  
>  .mimeAttachmentTable tr + tr > td {
>    border-top: 1px solid GrayText;
> +  padding-top: 0.25em;

My question and suggestion on this change still stands, see last review.

@@ +190,5 @@
>  }
>  
>  .mimeAttachmentFile,
>  .mimeAttachmentSize {
> +  padding: 0 0 0.25em;

dito.

::: mailnews/mime/src/mimemoz2.cpp
@@ +1090,5 @@
>      (mime_image_stream_data *) image_closure;
>  
>    const char *prefix;
>    /* Wouldn't it be nice if attributes were case-sensitive? */
> +  const char *scaledPrefix = "<DIV CLASS=\"moz-attached-image-container\"><IMG CLASS=\"moz-attached-image\" shrinktofit=\"yes\" SRC=\"";

Like this, I'm happy with the <DIV>. I believe it's technically not needed as we could do the centering on the image, but I guess this makes it more easily customizable, which is good.

Comment 22

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #20)

You're too fast, too many midair-collisions here... ;)
So code-wise, lgtm now.

I hope we haven't removed too much of whitespace now. Could you offer a screenshot with 3 attachments, including one multi-line filename?

> It's to make the first line of the table nearer to the title.

Oh, I see.
 
> No, because the display: block; makes that when printing, an image can be
> cut through and placed on two pages.

Oh, I see.

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8887373 [details] [diff] [review]
attachmentView.patch

This looks good, I tested it with multiple attachments. A large image wasn't broken at the paper border in the print preview.

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8887373 [details] [diff] [review]
attachmentView.patch

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

::: mailnews/mime/src/mimemoz2.cpp
@@ +1101,5 @@
>    if (!mid) return 0;
>  
>    /* Internal-external-reconnect only works when going to the screen. */
>    if (!mid->istream)
> +    return strdup("<DIV CLASS=\"moz-attached-image-container\"<IMG SRC=\"resource://gre-resources/loading-image.png\" ALT=\"[Image]\"></DIV>");

Closing > for DIV missing, I'll fix it when checking in.

Comment 25

[Jorg K (CEST = GMT+2)]

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