497895 - ugly iframe under feed messages viewed as "summary" (gnomestripe css change forgotten)

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Bug 438429 forgot that we now have a gnomestripe theme too... For feed messages viewed as "summary" there's an ugly empty iframe under each message.

http://hg.mozilla.org/comm-central/diff/ed1011d1e791/mail/themes/qute/mail/messageBody.css

alta88: I'm curious what the msgIframe iframe is supposed to do anyway though...? I even tried removing it and things appeared to worked fine afaict.

Comment 1

[alta88]

;)

the msgIframe in the new feed message construct does not do anything and is not used.  it is there in case a web page needs to be loaded in an iframe rather than loadURI(), as per original method, so that the stored message body need not be altered and the src attr merely set on the placeholder iframe element.  contingency proofing.

there still seem to be issues about content in messagepane, and it seems certain things like multimedia and even Acid pages[1] need to be enveloped in an iframe, even in Tb3.  but i'm not up on the latest.  you're free to remove this placeholder if you want.

[1] http://groups.google.com/group/mozilla.test.multimedia/browse_thread/thread/e6b75c8ee377150e#

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

The message pane itself is a <browser> element, which, as I understand it, is really just a specialization of an <iframe> in Gecko's mind.  Adding Standard8, as he's been doing work with various content display stuff lately and may have some opinions.

Comment 3

[alta88]

yes, as far as Gecko goes.  but i'm not sure that the messagepane <browser> document is (was) treated by Tb the same as a child iframe <browser> document.

Comment 4

[Mark Banner (:standard8)]

Web pages loaded as web content do not need additional wrapping.

I believe content in messages is processed via libmime which messes around with things (css mainly I believe) and therefore to avoid the problems can be wrapped in a child iframe.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 382968 [details] [diff] [review]
proposed fix

This is fine as far as it goes and fixes the problem in my tests, so r=myk.

But it seems like we should remove the iframe entirely, given that it isn't being used, and as I recall we decided to load feed messages into the message pane <browser> element itself, fixing any content loading issues that arise when we do so, instead of continuing to load messages into an internal iframe.

(Note: originally the iframe was just a hack to get these pages to load without a bunch of complicated message pane hacking back when this feature was an extension.)

As an aside, it's strange that this file uses mostly [CR][LF] instead of [LF] EOL markers.

Comment 6

[Magnus Melin [:mkmelin]]

I've fixed the line endings, and pushed this change
changeset:   2887:64aaeb609c9a
http://hg.mozilla.org/comm-central/rev/64aaeb609c9a

Leaving this bug open to get rid of the iframe...

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, part2 (remove the unused iframe)

Get rid of the unused iframe, and the css to hide/show it.

This shows the ugly iframe for old messages downloaded since bug 438429 landed. But given we haven't even had a beta since then i think that's reasonable.

Comment 8

[[e-mail]]

Comment on attachment 384101 [details] [diff] [review]
proposed fix, part2 (remove the unused iframe)

> /* Style new format rss summary vs web page */
>-body[selected="false"],
>-iframe[selected="false"] {
>+body[selected="false"] {
>   display: none;
When do we need selected="false"? In View as Web Page mode, you just load the page directly, and in View as Summary mode, you want to show the body...

> #_mailrssiframe {
I guess we need this in the case of (importing) old Tb2 feeds?

>     <div id=\"msgSummary\">\n\
>       %CONTENT%\n\
>     </div>\n\
Do we need the <div>? (I don't see any references to it)

>-    <iframe id=\"msgIframe\" selected=\"false\" src=\"\">\n\
>   </body>\n\
> </html>\n\
Bah, who reviewed this invalid (no </iframe>) HTML?

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to comment #8)
> (From update of attachment 384101 [details] [diff] [review])
> > /* Style new format rss summary vs web page */
> >-body[selected="false"],
> >-iframe[selected="false"] {
> >+body[selected="false"] {
> >   display: none;
> When do we need selected="false"? In View as Web Page mode, you just load the
> page directly, and in View as Summary mode, you want to show the body...

Apparently yes. Without it the web page won't show.
 
> > #_mailrssiframe {
> I guess we need this in the case of (importing) old Tb2 feeds?

Yeah, added a comment.

> >     <div id=\"msgSummary\">\n\
> >       %CONTENT%\n\
> >     </div>\n\
> Do we need the <div>? (I don't see any references to it)

Seems unneeded to me. 

I notice view (summary) as simple html / plain text doesn't work with or without this patch...

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, part2 (remove the unused iframe), v2

Also fixed an 
Warning: assignment to undeclared variable folder
Source File: chrome://messenger-newsblog/content/FeedItem.js
Line: 364

Comment 11

[alta88]

(In reply to comment #9)
> 
> I notice view (summary) as simple html / plain text doesn't work with or
> without this patch...

Bug 458606...

Comment 12

[[e-mail]]

Comment on attachment 384432 [details] [diff] [review]
proposed fix, part2 (remove the unused iframe), v2

>+/* New style feed summary body. */
>+body[selected="false"] {
I tried removing this and I see now that the summary flashes up briefly before the full page loads. While I do think it's a trifle fragile, I realise that anyone who wants to abuse this rule could abuse anything we replace it with. (And I wonder what effect view as simple html/text is going to do...)

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 384432 [details] [diff] [review]
proposed fix, part2 (remove the unused iframe), v2

This looks great, r=myk.

Comment 14

[Magnus Melin [:mkmelin]]

changeset:   2928:887e3c41d8dc
http://hg.mozilla.org/comm-central/rev/887e3c41d8dc

->FIXED

Testers beware that messages downloaded with old nightlies will show the iframe under the feed summary.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

mkmelin: will that effect people upgrading from Tb2 to Tb3 as well?

Comment 16

[Magnus Melin [:mkmelin]]

No, bug 438429 already changed the format from tb2->tb3 (and that format change is handled gracefully). This fix only cleaned up the new format. Only nightly users should be affected.

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #14)
> Testers beware that messages downloaded with old nightlies will show the iframe
> under the feed summary.

For reference, this is anything that got downloaded between the landing of bug 438429 on 1st May 2009, and the 24th June 2009. Anything downloaded before or after that period won't have the iframe.