457939 - messengerWindow needs an attribute to indicate the layout view type (classic, wide or vertical) for CSS selection proposes

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Pardal Freudenthal (:ShareBird)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3pre) Gecko/2008091306 Firefox/3.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080928120750 Shredder/3.0b1pre

On Thunderbird 2.0 it was possible to differentiate if the type of layout in use with some CSS "exercises" like:

/*classic view*/
#mailContentWrapper[orient="horizontal"] #messagesBox[orient="vertical"]{}
/*wide view*/
#mailContentWrapper[orient="vertical"] #messagesBox[orient="vertical"]{}
/*vertical view*/
#mailContentWrapper[orient="horizontal"] #messagesBox[orient="horizontal"]{}

Now, this approach doesn't work anymore. Actually it would be better if messengerWindow (or maybe the tabmail) would have an attribute indicating which view is in use to make possible to skin elements inside it (tabmail) according to the view (for example, margins, paddings and so on)

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Phil Ringnalda (:philor)]

Yeah, if the goal is to be able to style at the level of messagesBox (or anywhere above messagepanebox), I don't see any way with bug 389730, since now the only difference is which element is messagepanebox's parent.

I vote that someone else fixes it, though, since offhand I can't think of any elegant way.

Comment 2

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #1)
> Yeah, if the goal is to be able to style at the level of messagesBox (or
> anywhere above messagepanebox), I don't see any way with bug 389730, since now
> the only difference is which element is messagepanebox's parent.
> 
> I vote that someone else fixes it, though, since offhand I can't think of any
> elegant way.


Thank you Phil for commenting and pointing to that bug. I'm not the expert for this stuff, but maybe something like:

Index: msgMail3PaneWindow.js
===================================================================
--- msgMail3PaneWindow.js	(revision 49)
+++ msgMail3PaneWindow.js	(working copy)
@@ -648,8 +648,11 @@
 // aMsgWindowInitialized: false if we are calling from the onload handler, otherwise true
 function UpdateMailPaneConfig(aMsgWindowInitialized) {
   const dynamicIds = ["messagesBox", "mailContent", "threadPaneBox"];
-  var desiredId = dynamicIds[gPrefBranch.getIntPref("mail.pane_config.dynamic")];
+	var layoutView = gPrefBranch.getIntPref("mail.pane_config.dynamic");
+  var desiredId = dynamicIds[layoutView];
   var messagePane = GetMessagePane();
+	var layout;
+	var mailContent = document.getElementById("mailContent");
   if (messagePane.parentNode.id != desiredId) {
     ClearAttachmentList();
     var messagePaneSplitter = GetThreadAndMessagePaneSplitter();
@@ -670,6 +673,20 @@
         gDBView.reloadMessage();
     }
   }
+	switch (layoutView) {
+		case 0:
+		layout = "standard";
+		break;
+		case 1:
+		layout ="wide";
+		break;
+		case 2:
+		layout = "vertical";
+		break;
+	}
+	if(mailContent) {
+		mailContent.setAttribute("layout", layout);
+	}
 }
 
 const MailPrefObserver = {


could solve the problem. (??)

Comment 3

[Pardal Freudenthal (:ShareBird)]

Attachment: proposed patch

This patch adds a "layout" attribute to mailContent indicating which layout is being used.

Comment 4

[Pardal Freudenthal (:ShareBird)]

Attachment: patch v1.1

added white-space after "if" as suggested by JST Reviewer Simulacrum...

Comment 5

[Phil Ringnalda (:philor)]

Comment on attachment 354213 [details] [diff] [review]
patch v1.1

Rather than using a switch, you can just use another const array like dynamicIds to hold the layout names, and then since you want to set your layout attribute whether or not parentNode.id != desiredId, go ahead and set it up there at the top of the function, rather than separating your declarations from your use of them - nothing good for you can happen by letting other code get between when you declare a var and where you use it.

You can also skip the if check for mailContent: if we're switching layouts when mailContent isn't in the document, we want (and should certainly expect) error messages screaming that fact at the skies. Just assume, like desiredParent does, that you can forge ahead with document.getElementById("mailContent").setAttribute("layout", layouts[desiredId]) without checking.

Comment 6

[Pardal Freudenthal (:ShareBird)]

Attachment: patch v1.2

Thanks. I've modified the patch.

Comment 7

[Phil Ringnalda (:philor)]

Comment on attachment 355334 [details] [diff] [review]
patch v1.2

r=me, with a little wrapping I already did, since I accidentally fed you a line more than 80 characters long.

Comment 8

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/comm-central/rev/d931814567e5