+++ This is a split-off for a regression caused by bug 489609 +++ This is an *alternative* to bug 525225. Bug 525225 would be far better, but it's blocked by bug 492645. From bug 489609: ------- Comment #75 From Ben Bucksch 2009-10-29 07:52:38 PDT (-) [reply] ------- Private Mark, whether 1) the growing header pane after clicking "more" with lots of recipients or 2) a cut-off subject is worse is a judgment call. I don't think either are inherently worse than the other. I like neither of them. ------- Comment #76 From Ben Bucksch 2009-10-29 07:55:46 PDT (-) [reply] ------- Private (But if it was my call, I think I'd live 1 rather than 2.) ------- Comment #77 From Bryan W Clark (:clarkbw) 2009-10-29 09:40:40 PDT (-) [reply] ------- Private Before just regular comments. I just did a quick test with the patch inline at the bottom of this comment. By using the (more) link to fake out the all headers mode it seems to me that we can force a scroll bar when someone clicks on the more link and then on cleanup we reset the mode back to what it was. Note that in my patch here I didn't save the original mode and set it back which would likely be the more correct thing to do. In my testing I looked at a message with a lot of addresses that would right now fill up the entire message area when more was clicked. After clicking more I selected another message with a long subject and it renders correctly as the mode gets reset. I did this in both All and Normal header modes selected. The only issue I see is that whatever the size of the header at the time you click (more) is what it will render with, it doesn't expand the headers to a larger height and give you the scroll bars but I find it acceptable. Is something like this a possible option for TB3? [Patch here] ------- Comment #78 From Ben Bucksch 2009-10-29 10:07:19 PDT (-) [reply] ------- Private I like that idea. I'll play with it. ------- Comment #79 From rsx11m 2009-10-29 11:01:58 PDT (-) [reply] ------- Private What happens if you remove show_header_mode before this.clearChildNodes() and set it after this.fillAddressesNode() instead? The idea being that you allow the header pane to flex in height while refilling the headers after "more" has been clicked, and once this is done, allow it to be cropped to max-height and the scrollbar to appear if necessary? This may be a delicate timing though. ------- Comment #80 From Ben Bucksch 2009-10-29 12:28:33 PDT (-) [reply] ------- Private If there's a truely long header (I made an artificial test with a post to 30 newsgroups), the header pane in normal mode takes the majority of the message pane, without scrollbar, without ever having clicked "more" (the Newsgroups header has no "more" button). The same would be with a subject with 500+ characters, although you could argue that the subject then is the message ;-).

This is basically Bryan's idea, code in first hunk is the same. I changed the way it's resetted to the original value. I *really* don't understand why: - that AdjustHeaderView() call was in InitViewHeadersMenu(). If the headers were not fine before the user went into the menu, why fix them at this point? Once the user select a menu item and changes the mode, MsgViewAll/NormalHeaders() is called and it's set, so that's not why. - Why the "show_header_mode" attribute persists, even across restarts. There is no "persist" on the "expandedHeaderView" in msgHdrViewOverlay.xul. In other words, I think the last hunk should be needed even without the patch, but it worked without. (Now I need it, to reset it to correct state when we load a new message after we messed with it.) This works fine for me. This does *not* fix the problem mentioned in comment 80 above.

dmose, if you have no time, can you hand over the review to bwinton, please? > I *really* don't understand why: FWIW, both should be fine and correct after this patch, I'm just a bit puzzled why it worked before.

Comment on attachment 409168 [details] [diff] [review] Fix, v2 After discussion with Standard8 and clarkbw, we've come to the conclusion that continue to accept changes to this code that we don't absolutely have to is too risky because the code is so fragile. We believe that the right thing to do is to back out bug 489609 (which may also require backing out the changes from bug 524800). To compensate for the fragility of this code going forward, I believe that we need to start requiring mozmill tests for all changes to the msgHeaderOverlay code from here on out, so r- based on that.

Comment on attachment 409168 [details] [diff] [review] Fix, v2 What about trunk? Requesting review.

I think what I described in comment 2 and 3 is exactly what causes bug 526292 (it indeed did *not* work), i.e. this patch here probably fixes that bug.

FWIW, even with review, I am not going to check this in (into trunk or 3.0) without permission from dmose or drivers.

Here's the change of behavior that this bug does (in addition to fixing some code problems which cause bug 526292, but are needed here, too): With View | Headers | Normal mode, the header pane should be exactly as large as needed. There is no maximum. That's a problem when you click "more" on many recipients. It will grow as large as the whole message pane, pushing out the body. So, what this bug changes is: Once you click on "more", it temporarily changes to "All" headers mode, which is: As large as needed, but no more than 14em. If these are exceeded, it should stay at 14em and get a scrollbar. Once you click on another message, you should be back to Normal headers mode.

A particularly painful aspect of this bug is that restarting doesn't make the problem go away; the workaround is that the user has to turn off "Show All Headers" mode and turn it back on without the standalone window involvement. It does, however, appear to go away on Mac if a menu is opened that causes a submenu to open over the message header pane thereby forcing a repaint. Or something like that. I haven't yet been able to nail it down precisely. If we ship without taking this fix and without backing out bug 489609, we should probably relnote the workaround.

dmose, I think your comment belongs to bug 526292.

FWIW, the reason for this: > It does, however, appear to go away on Mac if a menu is opened that causes a > submenu to open over the message header pane thereby forcing a repaint. Or > something like that. I haven't yet been able to nail it down precisely. is completely clear. It's exactly the problem I described in comment 2. If you look at the patch here, it's obvious.

Ben: whoops; quite right, thanks!

I ran into more situations during development where the storage of the all headers attribute gets lost. The patch here is *really* needed. Please allow it in. If you decide to back out the subject wrap patch, we can back this one out as well.

(In reply to comment #4) > To compensate for the fragility of this code going forward, I believe that we > need to start requiring mozmill tests for all changes to the msgHeaderOverlay > code from here on out, so r- based on that. So, here are some tests. They aren't comprehensive by any stretch of the imagination, but perhaps someone could take them and add in some more stuff while I look into bug 527111 and eat dinner. (Ben, if you wanted to just write the (pseudo-)code for the tests, I'ld be happy to take it and get it running and cleaned up.) (Also, while I'm here, I've got a couple of questions about the previous patch, >+ try { >+ AdjustHeaderView(pref.getIntPref("mail.show_headers")); >+ } catch (e) {} The "catch (e) {}" construction always kinda wigs me out. (Probably due to bad experiences with the similar thing in Java in a previous job.) What are you expecting that to throw? Why don't we need something similar in InitViewHeadersMenu? Is just throwing away the exception without logging it really what we want to do there? And for: >+ // HACK Bug 525302 >+ // Workaround for bug 492645, which prevents bug 525225. >+ // Fake the "All Headers" mode, so that we get a scroll bar >+ // Will be reset when a new message loads >+ document.getElementById("expandedHeaderView").setAttribute("show_header_mode","all"); Can't you just call "AdjustHeaderView(Components.interfaces.nsMimeHeaderDisplayTypes.AllHeaders)" there? It seems to me like that's pretty much all AdjustHeaderView does. Thanks, Blake.

> [try/catch] What are you expecting that to throw? Pref-reading can always fail and throw, if neither user pref nor default pref are set. It shouldn't happen, but you need to consider that in the code. Because if it throws here, we can break a good part of mail reading - no good. That's all this try/catch is trying to prevent, really. I'd log the error, if it was super-easy, but I don't think we have super-easy and concise (10-20 chars max) and safe (never throwing itself) logging functions, so I just don't bother when Adjust() fails. > Can't you just call > AdjustHeaderView(Components.interfaces.nsMimeHeaderDisplayTypes.AllHeaders) Probably. I hope this function will never change to persist it or have other effects, given that this is a hack that only wants to temporarily set it, but I guess it's OK, so I'll change it.

(In reply to comment #15) > > [try/catch] What are you expecting that to throw? > Pref-reading can always fail and throw, if neither user pref nor default pref > are set. It shouldn't happen, but you need to consider that in the code. > Because if it throws here, we can break a good part of mail reading - no good. > That's all this try/catch is trying to prevent, really. Sounds fair, but I'ld want to know if it did fail, cause it could indicate a bigger problem. > I'd log the error, if it was super-easy, but I don't think we have super-easy > and concise (10-20 chars max) and safe (never throwing itself) logging > functions, so I just don't bother when Adjust() fails. It looks like there's a logException function somewhere, which is pretty short. And if it's not safe, we should really fix that. > > Can't you just call > > AdjustHeaderView(Components.interfaces.nsMimeHeaderDisplayTypes.AllHeaders) > Probably. I hope this function will never change to persist it or have other > effects, given that this is a hack that only wants to temporarily set it, but I > guess it's OK, so I'll change it. Cool.

> It looks like there's a logException function somewhere That sounds perfect. I'll use that, thanks. It's also already used in the same file, so already imported. It indeed does *not* look throw-safe, but as you say, we should fix that (in another bug).

This adds logException(e) I tested it and it works, shows the exception on stdout, nice. Thanks for the tip. I did not change to calling Adjust...(), because 1) This is the mailWidgets.js, and I'd depend on mailHdrViewOverlay.js to be included. Sure, we currently only use it in that overlay, but I don't like such implicit dependencies. 2) It doesn't help anything. It's just one line. All that the Adjust..() function does is to convert the IDL const to setting a XUL attribute. If the Adjust() function ever does more than that, e.g. saving or whatever, we might not want that here, given that it's a hack with a specific intended result of *temporarily* fixing the height and adding scrollbars, nothing more. So, I changed my mind to keep it as is. I hope that's OK / reasonable. bwinton, please re-review. Thanks for the tests. I haven't looked at them. Your patch with the tests is in addition to mine.

Comment on attachment 410965 [details] [diff] [review] Fix, v3 Well, the code looks pretty good to me. I can't see anything wrong with it, but the header is fragile enough that it also wouldn't surprise me if I missed something. (Which is one of the reasons I debated so long before giving it the r+. I've tested it with all the cases I could think of, but how much testing is enough?) One thing I did notice was that it worked kind of oddly if I added or removed tags (with the "1"/"2"/"3" keys) after clicking the "more" link. And so I think I want Bryan to go over it, and see if he's happy with how that behaves, or see what he would expect to happen. A related curiosity is shrinking the long subject to take two lines, then clicking the more button, which curiously makes the header smaller than it was. And having written that out, I think I've changed my mind. That last user experience is just a little too odd for me to give the patch an r+. (Although if Bryan says that he's happy with how that works, I would change my vote to an r+.) Thanks, Blake. One thing I did notice was that it worked kind of oddly if I added or removed tags (with the "1"/"2"/"3" keys) after clicking the "more" link. And so I think I want Bryan to go over it, and see if he's happy with how that behaves, or see what he would expect to happen. A related curiosity is shrinking the long subject to take two lines, then clicking the more button, which curiously makes the header smaller than it was. Okay, I've changed my mind, that last one is just a little too odd for me to r+ it. (Although if Bryan says that he's happy with all that, I will change that to an r+.) Thanks, Blake.

> if Bryan says that he's happy with all that, I will change that to an r+. This bug was Bryan's idea to start with. He created the first patch (I just attached it here). I just improved it technically, didn't change the behaviour.

Then it should be fairly easy to get an r+ from him. :) (Bryan, if/when you r+ it, just change my review to an r+ as well, 'kay?) Thanks, Blake.

Comment on attachment 410965 [details] [diff] [review] Fix, v3 yeah, the sizing with tabs is not the best but those issues are only happening after you've clicked (more); a situation I'm not too worried about. I don't really like how it shrinks up to a smaller size either but this is a workaround to get much more visible and problematic bug fixed. I'm going to + both as you requested.

Comment on attachment 410965 [details] [diff] [review] Fix, v3 Thanks. I'm requesting approval for 3.0 branch from dmose.

Comment on attachment 410965 [details] [diff] [review] Fix, v3 a=dmose

I will file a new bug for Blake's tests, given that I am not allowed to check them in without review. Thanks all. http://hg.mozilla.org/comm-central/rev/024ec25562c0 http://hg.mozilla.org/releases/comm-1.9.1/rev/20a06c02be38 FIXED

Comment on attachment 410875 [details] [diff] [review] A patch that adds some more message header tests. Filed bug 527550 for this test