530047 - message pane header buttons cover From address and also push the right msg page part out of the window pane

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Matěj Cepl]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; cs-CZ; rv:1.9.1.5) Gecko/20091105 Fedora/3.5.5-1.fc12 Firefox/3.5.5
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.5) Gecko/20091119 Fedora/3.0-3.11.rc1.fc12 Lightning/1.0pre Thunderbird/3.0

Plain text messages get wrapped out of the window. See attached screenshot

Reproducible: Always

Steps to Reproduce:
1.just read emails
2.
3.
Actual Results:  
text gets wrapped out of the window

Expected Results:  
text should be wrapped so it whole in the window

Comment 1

[Matěj Cepl]

Attachment: Screenshot: From address, Date and "Other actions" are all gone

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Giving to Blake for further investigation.  What appears to be happening here is that the localized versions of these buttons are so long that not only are they overwriting the value of the "from" field, they're actually bumping up against the label.

Comment 3

[Matěj Cepl]

(In reply to comment #2)
> Giving to Blake for further investigation.  What appears to be happening here
> is that the localized versions of these buttons are so long that not only are
> they overwriting the value of the "from" field, they're actually bumping up
> against the label.

Yeah, that too. It is cs_CZ locale.

Comment 4

[Ben Bucksch (:BenB)]

From the screenshot, it doesn't seem to have anything to do with wrapping, but it's a dup of bug 520249. See also bug 523254 comment 5.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Attachment: Well, we could do this...

Is it totally heinous, or not?

Thanks,
Blake.

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 413705 [details]
Well, we could do this...

wow, that could work.  not the prettiest thing but at least it makes everything work again.

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to wrap the header buttons

And here's the phenomenally large patch to enable that behaviour, on the off chance we think it helps.

Later,
Blake.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

As BenB mentioned in IRC, it doesn't really solve the problem of not letting the message pane wrap because the buttons are too long.  But it does give us some extra wiggle room there.

Comment 9

[Ben Bucksch (:BenB)]

That's precisely what I suggested in bug 520249. Shouldn't we just dup it?

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Reverting Ben's dup-o.  ;)

Comment 11

[Ben Bucksch (:BenB)]

Yup, wasn't intentional to actually carry it out.

This is a nice patch, I thought I had to use CSS absolute positioning and XHTML.

You could let the row of buttons also wrap, in case that alone is too wide (seems likely in this locale, given the screenshot).

Comment 12

[Ben Bucksch (:BenB)]

Changing summary to better reflect real bug.

Drivers: IMHO, this bug is bad enough to actually delay release. The fix is not without risk, but the problem is IMHO severe and can also be seen in en-US and other languages.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 413705 [details]
Well, we could do this...

didn't mean to carry forward the question mark.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

I tried to get the row of buttons to wrap, but it didn't work out for me.  I'ld love to see a patch which allowed that, though.  :)

Later,
Blake.

Comment 15

[Matěj Cepl]

(In reply to comment #6)
> (From update of attachment 413705 [details])
> wow, that could work.  not the prettiest thing but at least it makes everything
> work again.

Much much better! Thanks

Comment 16

[Ben Bucksch (:BenB)]

Attachment: Fix, v2 (for 3.0.1, probably)

With the previous patch, the buttons were left-aligned, tucked to the From address and left a gap on the right towards the window border. This patch fixes this.

I am currently trying to also solve the problem of "Other actions" button covering "more" (see bug 520249), with limited success.

Comment 17

[Ben Bucksch (:BenB)]

Attachment: Screenshot with Fix v2

Comment 18

[Ben Bucksch (:BenB)]

Attachment: Fix v3 WIP - playing with "other actions" button

This is not finished and does not replace Fix v2. I'm not happy yet with how it works, but attaching current state of work.

Comment 19

[Ben Bucksch (:BenB)]

Attachment: Fix v4 WIP - playing with "other actions" button

More or less same as WIP 3

it works in a way, but
1) the date and other actions go underneath the addresses, when there's not enough space. at least they don't cover "more" anymore, but it takes considerably more veritical space.
2) that even happens whenever a subject is long, because the "Other actions" wraps before the subject wraps.

Comment 20

[Ben Bucksch (:BenB)]

Attachment: Fix, v5

This is again the same as Fix v2, but trying to solve the covered "more" button with another idea: I noticed that the biggest problem is that 2 addresses show, and that plus the button is just too much. The addresses also can't wrap (which would be the obvious solution): They are already a <description>, but we force the wrapping off with the singleline="true" hack that implements our show/hide toggle, so that's non-trivial to solve.

So, the cheap solution is to show only one address at first, and show others after "more". I argue that's more useful and also more discoverable than attempting to show 2 and risking that the "more" is not visible at all.

I also think that this patch v3 worth to be included in 3.0. The problems it solved are just too big to ship with, IMHO, and the patch - although not for free, neither riskless nor without tradeoff - simple enough to risk it.

Comment 21

[Ben Bucksch (:BenB)]

Attachment: Screenshot with Fix v5

I think that's looking quite reasonable.

Bryan, please approve to show only one address by default, see last comment.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

I'm also going to send this in email to Bryan, as he may see that sooner:

I downloaded the cs-CZ build on my 1024x768 laptop, and played around with that.  Things look ok in default layout, as long as the window is allowed to be full-screen.  It gets ugly quickly if you shrink it.  Vertical view is unusable, even at full-screen.  Wide view actually looks pretty good.

After a bunch of driver discussion, our current belief is that mixing XUL and HTML box model stuff at this late stage is too risky for 3.0, though we could probably take it for 3.0.1 given some baking time.

As an alternate theory for 3.0, Blake is going to try to put together a patch that causes the Vertical View to default to using icons rather than the text.  Bryan is this something you could live with?

If not, we should probably ship as-is, and relnote the problem, suggesting manually switching to icons mode or using Wide View as workarounds.

Comment 23

[Ben Bucksch (:BenB)]

The problem is not just vertical view. When I make the standalone msg window a size so that it's as wide as the bugzilla wrap width (80 chars?), i.e. a reasonable size, I can't even see "from bugzilla-daemon@mo", and it's already cut off.

Yes, there is risk, but can it get much worse than it already is?

Comment 24

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to default the vertical view to icons-only.

Another option we can choose from.

Comment 25

[Blake Winton (:bwinton) (:☕️)]

Some questions were raised on IRC, which might be germane to your decision, Bryan:

- Shouldn't there be an else clause?
  I didn't think so.  I don't want to set it to anything in particular if we're not switching to vertical mode.

- Right, but what about when we switch out of vertical mode?
  I think we stay in icons-only mode, on the basis of the smallest possible  change.  Otherwise, what if you're in text-and-icons, and go to vertical, then go to text-only?  Do we go back to text-and-icons, or stay in text-only?  I figure it makes the most sense to stay in the icons-only mode, although there is the chance of a problem with users who just "try out" vertical mode and switch back immediately and then don't even realize that button labels are gone and don't know how to get them back.

- Is there a SetAndDoNotPersist()?
  Even if there was, I wouldn't want to call it.  If you switch from icons-and-text to vertical/icons-only, then exit and restart, you should still be in icons-only, I think.

Comment 26

[Blake Winton (:bwinton) (:☕️)]

(Thanks to DMose and BenB for the questions and scenarios.)

Comment 27

[Ben Bucksch (:BenB)]

Comment on attachment 413734 [details] [diff] [review]
Fix, v5

I splitted out the "show only one recipient" idea to bug 530239, because it's safe code-wise.

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #25)
> 
> - Is there a SetAndDoNotPersist()?
>   Even if there was, I wouldn't want to call it.  If you switch from
> icons-and-text to vertical/icons-only, then exit and restart, you should still
> be in icons-only, I think.

It looks to me like UpdateMailPaneConfig gets called from onload, so if there
were one, I think that would actually be better.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

So blake mentioned that he's pretty tired, and I'm starting to lose alertness as well.  To the point where I don't feel comfortable doing a review and jamming this into the tree because I don't think I have a clear enough picture of the details in my head.

I'd like to hear Bryan's opinion at this point, and I'll look in on the bug in the morning to review (or someone else can grab the review from me before that, if they wish), if that's the solution Bryan wants to go with.

That said, I'm liking the idea of relnoting what we've got, and taking one of Ben's fixes for 3.0.1 more and more.

Comment 30

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to not save the pref (for 3.0).

(In reply to comment #28)
> (In reply to comment #25)
> > - Is there a SetAndDoNotPersist()?
> >   Even if there was, I wouldn't want to call it.  If you switch from
> > icons-and-text to vertical/icons-only, then exit and restart, you should
> > still be in icons-only, I think.
> It looks to me like UpdateMailPaneConfig gets called from onload, so if there
> were one, I think that would actually be better.

Okay, so we could do _this_ instead.

It would let people who switch to and from vertical mode by accident to get their old toolbar buttons back by quitting and restarting Thunderbird.  On the other hand, it would make people who like the new icons-only toolbar buttons lose them on restart, if they didn't re-set the option.

I don't know if there are any good answers here, but now you have a few to choose from.  :)

G'night,
Blake.

Comment 31

[Mark Banner (:standard8)]

(In reply to comment #30)
> It would let people who switch to and from vertical mode by accident to get
> their old toolbar buttons back by quitting and restarting Thunderbird.

When switching to a different view, couldn't you reset the value from the saved pref?

> On the
> other hand, it would make people who like the new icons-only toolbar buttons
> lose them on restart, if they didn't re-set the option.

But it wouldn't affect people who had set their toolbar to icons-only, gone into vertical view, then come back out & restarted would it?

Comment 32

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to use the saved value.

(In reply to comment #31)
> (In reply to comment #30)
> > It would let people who switch to and from vertical mode by accident to get
> > their old toolbar buttons back by quitting and restarting Thunderbird.
> When switching to a different view, couldn't you reset the value from the
> saved pref?

Yes, in theory.  What's the inverse call for document.persist?  (i.e. the call that will get the previously persisted value.)

(An hour or two later, I've finally come up with this.  Someone should really throw this code into document.retrieve(), or something, cause wow, is this area ever under-documented.  :)

I assigned the review to dmose, cause he got the previous two, but if you wanted to steal it, Mark, I wouldn't object to that.

> > On the
> > other hand, it would make people who like the new icons-only toolbar buttons
> > lose them on restart, if they didn't re-set the option.
> But it wouldn't affect people who had set their toolbar to icons-only, gone
> into vertical view, then come back out & restarted would it?

No, nor would it affect people who went to the vertical view, came back out, right-clicked and selected icons-only, and then restarted.  It's not terrible behaviour by any means.

Later,
Blake.

Comment 33

[Ben Bucksch (:BenB)]

> Someone should really throw this code into document.retrieve()

Filed bug 530312 about that.

Comment 34

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 413768 [details] [diff] [review]
A patch to not save the pref (for 3.0).

gave this the ok on IRC so setting it up here as well.

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 413722 [details] [diff] [review]
Fix, v2 (for 3.0.1, probably)

Carrying forward ui-review+, since it got that on a previous iteration.

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 413768 [details] [diff] [review]
A patch to not save the pref (for 3.0).

clarkbw has tentatively said this is OK, but he would like to play around with it a bit more in a few hours before giving it his final seal of approval.  Putting back ui-review? so that we have the least potential for confusion about what still needs to happen.

Comment 37

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to save the modes per layout in the dom. (possible 3.0)

What do you all think about this one?

Later,
Blake.

Comment 38

[Blake Winton (:bwinton) (:☕️)]

Attachment: A better patch to save the modes per layout in the dom. (possible 3.0)

This time also saving it when the toolbar's mode gets persisted.

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

The saving via DOM still feels a little risky to jam in at the last second without baking.

So at this point, I feel like the best balance of risk & reward that we're likely to get is to keep moving forward with is "a patch to not save the pref" that clarkbw has tentatively OKed (or, if we decide that we can't live with that, just relnoting and living with what we've got now).

In testing that patch, I've found one other odd behavior:

Start in the default mode (classic view & text + icons)
Switch to vertical mode (forces icons-only) at least until you quit
Start again, still in vertical mode with icons only
Switch to Classic mode
You still have icons-only, not the old-default of icons+text.

This usage pattern feels unusual enough that I suspect we can live with the pain, but that should be factored into the final ui-r decision.

Comment 40

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 413768 [details] [diff] [review]
A patch to not save the pref (for 3.0).

r=dmose

Comment 41

[Mark Banner (:standard8)]

(In reply to comment #39)
> So at this point, I feel like the best balance of risk & reward that we're
> likely to get is to keep moving forward with is "a patch to not save the pref"
> that clarkbw has tentatively OKed (or, if we decide that we can't live with
> that, just relnoting and living with what we've got now).
> 
> In testing that patch, I've found one other odd behavior:

I'm also concerned that if you live in vertical view mode with a wide screen and want text on your icons, you'll be resetting the toolbar on each restart to what you want as the patch forces you to icon mode all the time.

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

Mark: ugh, yes.  I'm continuing to like the idea of living with what we've got now even more.  :-(  I'm interested in clarkbw's thoughts...

Comment 43

[Bryan Clark (DevTools PM) [:clarkbw]]

Yeah I'm feeling like we should just stick with what we have and relnote this problem. We'll have enough time to work out a real fix for the 3.0.1 release.

Comment 44

[Blake Winton (:bwinton) (:☕️)]

As a side note, I tried a quick fix by moving the code inside the if (messagePane.parentNode.id != desiredId) test, but it looks like that test is true on startup, because it didn't fix the bug.

Comment 45

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #43)
> Yeah I'm feeling like we should just stick with what we have and relnote this
> problem. We'll have enough time to work out a real fix for the 3.0.1 release.

Just to expound on this a bit.  I think it can be ok to introduce a slightly odd behavior as a fix to an issue like this.  But given that we're planning on reverting this odd behavior later for a more real fix it seems more cruel than helpful to our users as a whole.  Lets just get a real fixed baked and ready for the 3.0.1 release. :)

Comment 46

[Ben Bucksch (:BenB)]

Blake, can you please formally review Fix v2 (you created the first version of it)? I can check that into trunk, and it can bake there.

Bryan, can you please ui-review bug 530239? This would be very safe, that's why I splitted this out from here.

Comment 47

[Michel]

Hello, 
I'm a former subscriber of bug 520249, on which I added a new comment after trying rc1. Ben Bucksch nicely invited me here and I discovered your interesting discussion.

Lead me to find a very good way of working around it now : right clicking over the offending buttons in the message header pane now offers the choice of displaying only icons. This doesn't affect the general toolbar, only the message pane. This seems perfectly OK for what I can see.

I'm not enough involved in the code to know which patch is applied or not in my build, but some of you will certainly be : Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.1.5) Gecko/20091121 Thunderbird/3.0

Moreover, the pane now wraps correctly the text :-) (with icons and text of course its width was calculated for the buttons, so the thoeretical width was wider than the actual pane.

It seems that you're about to ship it !

Bravo, bravo !

Michel

Comment 49

[Ben Bucksch (:BenB)]

re Fix v2: 
-        <hbox id="expandedHeadersTopBox" flex="1">
+        <hbox id="expandedHeadersTopBox" flex="1" style="display: block;" >

Blake suggested to use <html:div> instead of <hbox> with display:block, to make the code clearer. I think that makes sense, if it doesn't break anything (e.g. themes).

Comment 50

[Wayne Mery (:wsmwk)]

suggest this is major, given intl impact
http://forums.mozillazine.org/viewtopic.php?f=39&t=1637175

Comment 51

[Ben Bucksch (:BenB)]

Wayne, the previous post is about "all headers" mode -> bug 526918

Comment 52

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 413722 [details] [diff] [review]
Fix, v2 (for 3.0.1, probably)

Because of the weirdness with the spacer and the hbox-acting-like-a-div, I can't in good conscience approve this patch.

If you wanted to try a patch with something like what's in comment 49, that might be better.  (Or perhaps better still would be to leave the hbox, and put a div inside of it.  I think that _might_ break fewer extensions.  Then again, maybe not.)

You could also try this patch with a different reviewer.  One who may be more comfortable with this sort of mixing of XUL and (X?)HTML.

Sorry about that,
Blake.

Comment 53

[Ben Bucksch (:BenB)]

Yeah, sure, I'll attach a patch with <div> later.

Comment 54

[Matěj Cepl]

Attachment: screenshot illustrating the issue

Actually I thought, that this is limited to localized environment where too big buttons push the right margin outside of the window. But here I have en_US Thunderbird Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.7) Gecko/20100201 Fedora/3.0.1-1.el6 Lightning/1.0b1 Thunderbird/3.0.1, nothing seems to be squeezed and yet wrapping is off. What's the matter?

Comment 55

[Ben Bucksch (:BenB)]

Comment on attachment 426218 [details]
screenshot illustrating the issue

You mean inside the body? That's because of the HTML of the message itself. If it's in a table, and there's a wide element like the image, the image makes the table wide and with it the text in it. You have the same effect on the web. Has nothing to do with Thunderbird UI.

Comment 56

[Dan Mosedale (:dmosedale, :dmose)]

Since we want some baking on whatever we go with here, I'm going to tentatively set this to block beta1 without yet having talked to Blake.  Blake, what's the next step here -- to request ui-review from Bryan on one or both of the existing patches?

Comment 57

[Blake Winton (:bwinton) (:☕️)]

I think we need to figure out what we want to do.  If only there was a module owner for Toolbars, or the Message Reader…  :)

We didn't want to take Ben's patch because mixing XUL and HTML seemed a little too weird for me at that stage, but the deadlines aren't looming quite as closely now, which changes things.  Could we try Ben's patch from attachment 413722 [details] [diff] [review] (or, more accurately, the new one that uses a div that he mentions in comment 53 but hasn't posted yet), but still leave the option open to revert it if it looks like it's causing unexpected weirdness?

Ben, would that work for you?  I think we could carry forward the ui-r+ unless Bryan speaks up, and I think I'm okay giving it a quick r+, if the change is simple enough, and with the caveat that we might revert it in the future.

Thanks,
Blake.

Comment 58

[Ben Bucksch (:BenB)]

Attachment: Fix, v7 - functionally like v2, but with <div> instead of display:block

As requested by bwinton, here's Fix v2 with <html:div> instead of <hbox style="displaly: block">. Functionally, they are the same, screenshot v2 also applies for Fix v7. I needed to adapt namespaces and stylesheet as well to use the HTML element. I prefer Fix v2, but both are fine with me.

Comment 59

[Ben Bucksch (:BenB)]

Attachment: Fix, v8 - functionally like v2, but with <div> instead of display:block

Forgot the other themes.

Comment 60

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 426691 [details] [diff] [review]
Fix, v8 - functionally like v2, but with <div> instead of display:block

Go, go, gadget module owner!  Let's get UI signoff off on this overall strategy before we spend module owner time reviewing the code, however...

Comment 61

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 413725 [details]
Screenshot with Fix v2

I think I was expecting the buttons to float on top of the from.  Maybe that's just something we talked about?

This looks pretty good though.

Comment 62

[Ben Bucksch (:BenB)]

Bryan, yes, "button float on top of From" was discussed in another bug (which I can't find). I planned to do that with CSS position:fixed, but not sure how feasible that is with XUL.

Comment 63

[Ben Bucksch (:BenB)]

bwinton pointed out that when you click on "more" with this patch applied, the header pane shrinks to a default value. Without the patch, its size stays when clicking on "more". Up to others to decide how severe that is. (I wouldn't invest significantly more time in this bug, but rather in bug 543954.)

Comment 64

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 426691 [details] [diff] [review]
Fix, v8 - functionally like v2, but with <div> instead of display:block

>+++ b/mail/themes/gnomestripe/mail/messageHeader.css
>@@ -64,17 +65,18 @@
>-#expandedHeadersTopBox {
>+/*#expandedHeadersTopBox*/
>+html|div#expandedHeadersTopBox {

I would prefer not to keep the previous selector around in comments.  That's what version control is for.  ;)

But aside from that, this seems like a reasonable patch to try and run with for a while.

(Of course, it'll need Bryan's ui-r first.)

Later,
Blake.

Comment 65

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: odd alignment issues

Comment 66

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 426691 [details] [diff] [review]
Fix, v8 - functionally like v2, but with <div> instead of display:block

we need to fix the alignment issues seen in attachment 426743 [details] the from show align to the middle vertically.

though I can't seem to figure out how to fix it myself.

Comment 67

[Dan Mosedale (:dmosedale, :dmose)]

We want this, but we wouldn't hold b1 for it, so moving out to b2.  BenB has said that he's not willing to put more time into this now, so reassigning to [e-mail] for the moment.  Some ideas he had about how to move things forward here:

BenB: if I was to spend time on it (which I won't), I'd first try a little more, with random ideas, based on CSS+HTML specs, experienced web authors etc.
BenB: for 1-2 hours. if that fails, I'd try a reduced testcase. or abandon the idea and
[5:57pm] try to make the toolbar wrap differently, although that might not work.
[5:58pm] hm, another idea, very hackish: we might be able to hardcode the top margin, if we know the button size.

Comment 69

[Dan Mosedale (:dmosedale, :dmose)]

Blake, is this something you'd be able to find time for in beta 2?  I think it would be helpful for us to have this in time for our "tb2 -> tb3" update test day, which I suspect happens sooner rather than later.

Comment 70

[Blake Winton (:bwinton) (:☕️)]

Sure, why not?  How hard can it be, right?  ;)

Comment 71

[Bryan Clark (DevTools PM) [:clarkbw]]

Just wanted to note that I tried making this line up correctly in the middle for a while but something about the float:right makes this from line all crazy in the cabeza.  If our back is against the wall we could try using some margin/padding on the top to make this from line appear more in the middle.  The values for that cushion will need to be different per OS as the buttons are slightly different sizes.

Comment 72

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with the alignment fixed.

I think I've played with it enough without getting anywhere, so I've fallen back to the padding-top solution, a patch for which I've attached here.  I've also emailed screenshots to Bryan (and Dan) to assist in the ui-review.

Thanks,
Blake.

Comment 73

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 443218 [details] [diff] [review]
The previous patch, with the alignment fixed.

i gave the screenshots a ui-r+ so I should mark that here as well.

Comment 74

[Martin Gregory]

Can you provide an option to put the buttons back at the top of Thunderbird, where they used to be?   Is this something that a plugin could do right now, to work around this problem?

Comment 75

[Martin Gregory]

I just discovered that in my case this "bug" does _not_ happen in safe mode.

It happens when I enable the "show more recipients" plugin.

In this sense it's still a bug (in the sense that TB doesn't do what I need :) ) because I need to be able to see the recipients.

However, I thought someone had asserted this happens in safe-mode: it definitely does not for me: in safe mode the buttons obscure the "from" entry in the header pane, allowing them to come left as far as needed and display all of them and the scroll bar.

It's only with "show more recipients" plugin enabled that the buttons get pushed out to the right by the "from" header, obscuring them and the scroll bard.

Martin

Comment 76

[Martin Gregory]

BTW, why don't you just put the buttons _above_ the From header?  Then they would always be in _exactly_ the same place, which is a highly desireable property of buttons... right now you always have to reach your mouse to a slightly different place to click those buttons.

(And, personally, I'd like the option to put them right back where they used to be, where there was no risk of them moving at all)

Martin

Comment 77

[Ben Bucksch (:BenB)]

Martin, comment 74 and comment 75 are offtopic here.
> why don't you just put the buttons _above_ the From header?

If you mean always: Because we want to save screen space for those users who have bigger windows.
If you mean "when wrap": because that's too hard to implement, at the moment. We may do that later.

Comment 78

[Martin Gregory]

I thought 75 was completely material to the topic.  This thing doesn't happen in safe more, at least for me.

Sorry about 74 & 76... where is the right place for users to discuss how they work around issues in specific tickets: what options might exist, what approaches are being taken to resolve etc?

Martin

Comment 79

[Ben Bucksch (:BenB)]

> where is the right place for users to discuss how they
> work around issues in specific tickets:

forums.mozillazine.org or newsgroups <http://www.mozilla.org/community/forums/>

Comment 80

[Blake Winton (:bwinton) (:☕️)]

Attachment: A screenshot of the patch.

(In reply to comment #77)
> Martin, comment 74 and comment 75 are offtopic here.
> > why don't you just put the buttons _above_ the From header?
> If you mean "when wrap": because that's too hard to implement, at the moment.
> We may do that later.

Uh, like this (from the patch under review)?  ;)

Later,
Blake.

Comment 81

[Martin Gregory]

Yes, just like that.  Thanks.

Interestingly, I asked this question in the support forum, and did not get this answer.  While I appreciate the need to keep "noise" out of tickets, I think that there's some need to be able to ask question about specific tickets _in_ the ticket.   That's where the people who know the most will be, and lets face it the people who put themselves on CC might just appreciate hearing how to deal with it.

Thanks,

Martin

Comment 83

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 443218 [details] [diff] [review]
The previous patch, with the alignment fixed.

In general, looks good, modulo the lack of automated test coverage.

>-#expandedHeadersTopBox {
>+/*#expandedHeadersTopBox*/

The above comment doesn't seem to add much; I'd just remove it.

>+html|div#expandedHeadersTopBox {

I'm not familiar with this usage of | in CSS, and a quick search turns nothing up.  Can you point me to info about how it's intended to work?

>+#expandedHeaderRows {
>+  padding-top: 14px;
>+}
>+

Seems like a relative unit might work better with more font/OS version/monitor combinations both here and in the other themes.  If there's some specific reason we want to use pixels here, how about adding a comment?  In any case, this is minor, if you don't have time to poke at this, I could live with leaving it as is.

In general, looks good.  However, I'm r-ing because, given the impressive level of fragility of the message header overlay, I think we need at least some level of automated test coverage here.

Comment 84

[Ben Bucksch (:BenB)]

+ html|div#expandedHeadersTopBox {
> I'm not familiar with this usage of | in CSS

It's the namespace:

 @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
+@namespace html url("http://www.w3.org/1999/xhtml");

Given that this CSS file is in XUL namespace, a selector for just "div#exp" will not match the <html:div>.

Comment 85

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with a test.

(In reply to comment #83)
> (From update of attachment 443218 [details] [diff] [review])
> In general, looks good, modulo the lack of automated test coverage.

Added.

> >-#expandedHeadersTopBox {
> >+/*#expandedHeadersTopBox*/
> The above comment doesn't seem to add much; I'd just remove it.

Removed.

(In reply to comment #84)
> + html|div#expandedHeadersTopBox {
> > I'm not familiar with this usage of | in CSS
> It's the namespace:
>  @namespace
> url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> +@namespace html url("http://www.w3.org/1999/xhtml");
> 
> Given that this CSS file is in XUL namespace, a selector for just "div#exp"
> will not match the <html:div>.

Added as a comment.

> >+#expandedHeaderRows {
> >+  padding-top: 14px;
> Seems like a relative unit might work better with more font/OS version/monitor
> combinations both here and in the other themes.  If there's some specific
> reason we want to use pixels here, how about adding a comment?  In any case,
> this is minor, if you don't have time to poke at this, I could live with
> leaving it as is.

Bryan and I both believe that the height of the buttons is in pixels, so I've added a comment.

(Carrying forward ui-r=clarkbw.)

Thanks,
Blake.

Comment 86

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 445362 [details] [diff] [review]
The previous patch, with a test.

Looks great; thanks!

Comment 87

[Blake Winton (:bwinton) (:☕️)]

Checked in as <http://hg.mozilla.org/comm-central/rev/92940f8c2767> and <http://hg.mozilla.org/releases/comm-1.9.2/rev/b5c491ddde2c>.

Comment 88

[Blake Winton (:bwinton) (:☕️)]

Attachment: Fix test breakage.

D'oh!  Apparently the Mac test box has smaller fonts than my 10.6 Mac, or something.  Anyways, now we calculate the perfect width to toggle the overflow.

Comment 89

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 445492 [details] [diff] [review]
Fix test breakage.

r=dmose, given testing the stuff mentioned in IRC.

Comment 90

[Blake Winton (:bwinton) (:☕️)]

Committted as:
http://hg.mozilla.org/comm-central/rev/a4f422bf30c9
http://hg.mozilla.org/releases/comm-1.9.2/rev/6847a02f5814

Thanks,
Blake.

Comment 91

[Blake Winton (:bwinton) (:☕️)]

Attachment: Patch v2, this time without lightning-specific code.

Just to keep a track of what's committed.

Comment 92

[Jan Horak [:jhorak]]

Can we get an approval‑thunderbird3.0.5+? This issue is problem for a few of Fedora users and we would like to add this patch to 3.0.5 release.

Comment 93

[Mark Banner (:standard8)]

(In reply to comment #92)
> Can we get an approval‑thunderbird3.0.5+? This issue is problem for a few of
> Fedora users and we would like to add this patch to 3.0.5 release.

This is already practically too late for Thunderbird 3.0.5 - the builds for that are out for testing, and at this stage we'd only really respin them for serious blockers.

For 3.0.x generally, there's two things I'm concerned about and would deny that approval: Firstly it feels like a risky patch/alteration for a stable branch and 3.1 should be out fairly soon so users will be able to pick up the fix there; Secondly, the fact we're changing hbox to html:div and we're re-ordering elements may break extensions who are working with the header pane, and this is definitely a no-go area for a stable branch.

Comment 94

[Mark]

Hi,

I've yet to build the patch so possibly my comment may already have been addressed, although as it may be a while it seems right to add to what I've said at https://bugzilla.mozilla.org/show_bug.cgi?id=563966 , to say that when I put the Email in a new tab, it's clear that the 'thunderbird has blocked content' bar is one main cause of the overly wide header bar that seems to affect message scrollbars; when I click allow 'remote content' the scrollbars appear; naturally the button to allow remote content is itself invisible when the 'remote content' bar has no proper wrapping either; so that may need some css work similarly