460647 - Message Header: Focussing contacts causes respective header and label to move down some px (polish)

Status: VERIFIED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Thomas D. (:thomas8)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1b1pre) Gecko/20081006 Shredder/3.0a3

TB3a3, something to polish:
When moving focus through contacts (email addresses) in the header pane of new message reader, each time you land on a contact, either header captions (in the case of "to") or complete headers below the current header are moving up and down within header pane. I think it's because there is an extra top padding margin within the dotted focus indicator for contact display names.
Moving focus doesn't normally change the position of other elements, so this behaviour, though negligable, might send subconscious signals of instability to the user... ;-)



Reproducible: Always

Steps to Reproduce:
1. In message reader, Move focus within header pane (contact display names of email addresses)
2. 
3.
Actual Results:  
when focus lands on a to-contact, TO-header-caption moves down
when focus lands on any contact within any given header (e.g. "TO"), caption and content of all headers below the current header move down about 1 or 2 pixels

Expected Results:  
moving focus should not move up or down any other elements within header pane.

Comment 1

[Wayne Mery (:wsmwk)]

Thomas, do you still see this?

It's WFM with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081212 Shredder/3.0b2pre

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

I'm not seeing this on Mac nightlies either.

Comment 3

[Thomas D. (:thomas8)]

Unfortunately, this behaviour is still there, display contacts in message reader header shifting up and down when you move focus along using TAB.
Another odd thing is that the focussed yellow star becomes significantly smaller than the unfocussed yellow star - why? Not nice, and adding to the instable impression.
Even more ghost movements plus disappearing focus when it comes to Bug 460648 (Focus dissappears when moving through collapsed header contacts): watch how to-caption and more-caption (for to-header) move up and down while you are tabbing through hidden contacts of to-header...

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081219 Shredder/3.0b2pre

Comment 4

[Thomas D. (:thomas8)]

I am sharing the profile with TB2, but that's not very likely to cause this, or is it? All Addons are disabled because they don't have the right version for the TB3b2pre nightly, which includes "header UI tweaks and more" of Bug 466025.
You need to TAB through several to-contacts and their stars to see this. In my case, it's two rows of to-contacts so "more" button might play a role too.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

OK, I'm seeing this on nightlies now as well.  I don't know that this should block tb3, but it might.

Comment 6

[Thomas D. (:thomas8)]

It's trivial for a reason (obviously it doesn't hurt functionality in any way), but still: In your own interest, make it a blocker. I might be wrong, but you wouldn't want people laughing at you for fixed ui elements hopping round just because focus is being moved. Also, consider the *impression* of instability and poor design this might create.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Agreed; it all depends on the set of things that we have to triage this against.

Comment 8

[David Ascher (:davida)]

I hate the dashed line focus indicator, precisely because of this bug, but I can't figure out an alternative.  Adding two pixels to the height each row to avoid this problem means growing the whole box by a lot of pixels in aggregate.  It'd be nice to handle this as part of a tweak on the box layout of that cursed pane.

Would love a fix, but would we ship if it was the last bug standing? unfortunately, yes.

Comment 9

[ovidiu]

just add a 1px border like this:

.emailDisplayButton { border: 1px dotted transparent; }
   [line 386-390 in messageHeader.css]

to match the one existing only on focus:
.emailDisplayButton:focus {border: 1px dotted Highlight;}
   [line 410-411]

It works. Having a border of 1px transparent to fit the focus border. 
I tried this in bug 466025 as extension.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

ovidiu: doesn't that still have the problem describe in comment 8?  

I notice, though, that the emailDisplayButton element also has 1.1px of padding.  I wonder how it would look if we did both what you suggest _and_ got rid of that padding.  Care to take a run at it?

Comment 11

[Thomas D. (:thomas8)]

Attachment: Screenshot1 of patch proposed in comment#9, compared with current build

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5pre) Gecko/20091008 Shredder/3.0pre

Ovidiu, could you provide a patch with your proposal to fix this?
> .emailDisplayButton { border: 1px dotted transparent; }
>   [line 479-488 in messageHeader.css]

(In reply to comment #8)
> Adding two pixels to the height each row to avoid this problem means growing
> the whole box by a lot of pixels in aggregate.

I have tested this out with the current build as above, and no problems seen as I shall explain below.

(In reply to comment #10)
> @ovidiu: doesn't that still have the problem describe in comment 8?  

Yes and no. Strangely enough, adding { border: 1px dotted transparent; } to email display headers does NOT change the total box height of my test default header in any way (see screenshot). (I had a reply-to included, does that matter?)

I'm glad I've never seen the XUL of that "cursed pane", as David calls it.
But some things like subject and labels seem glued onto the pane, so even though some emailDisplay headers are pushed down by 1px due to their transparent 1px border, it never adds to the total height, probably because the sticky ones serve as a buffer.

> I notice, though, that the emailDisplayButton element also has 1.1px of
> padding.
FTR: This has changed in the current build to a 0.1em padding-top/bottom.

Comment 12

[Thomas D. (:thomas8)]

This is also relavant for
related Bug 521334 -  Nit: Improve header pane metrics (e.g. more actions button too big, right alignment, vertical space)

Comment 13

[ovidiu]

(In reply to comment #11)

> Ovidiu, could you provide a patch with your proposal to fix this?
> > .emailDisplayButton { border: 1px dotted transparent; }
> >   [line 479-488 in messageHeader.css]

Sorry, never did a patch. I just briefly experimented with xpi or simple css.

*to other coments:
At the time I did this I did not put more attention into this as it seamed fine ever since, with only this margin workaround. 
(lately, b4, I run without xpi so dunno)

Comment 14

[Andreas Nilsson (:andreasn)]

Attachment: patch for trunk and 1.9.1 branch

Here is ovidiu proposed changes as a patch. Seems to do the trick on Linux and Vista.

Comment 15

[Andreas Nilsson (:andreasn)]

Attachment: screenshot to ease the review

above before, below after (on linux)

Comment 16

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

Comment on attachment 410813 [details] [diff] [review]
patch for trunk and 1.9.1 branch

Looks like it fixes the problem, and I don't see anything wrong with the code.

(Bryan, do you think the border should be dotted or solid, given that it's transparent, and so probably doesn't matter?)

Comment 17

[Thomas D. (:thomas8)]

Just my 2 cents, if it doesn't matter, perhaps let's leave the transparent border dotted so that future generations can tell why it's there by associating it with the dotted focus border.

Comment 18

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

Comment on attachment 410813 [details] [diff] [review]
patch for trunk and 1.9.1 branch

Assuming it doesn't effect the :focus behaviour then dotted or solid shouldn't matter w/ transparent.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

We want this for 3.1 since it has an easy patch, so marking as blocking that release.  After baking on the trunk, we could consider taking this for early in the 3.0.x release series as well.

Note that this patch still needs an automated test.

Comment 20

[Thomas D. (:thomas8)]

Mark, could we please check in this patch on trunk and branch?
It has review+ and ui-review+ , so it's ready to land.

It's a single-line css layout fix with no dangers whatsoever. Earlier today, there hasn't been any bitrot neither trunk nor 1.9.1 branch (checked against mxr). Trunk and branch messageHeader.css are currently same code.

Please check this in, and let's get rid of the embarassment here that our UI starts to hop around when you tab over it. There's enough embarassing things we won't be able to fix before release.

Also, DavidA "Would love a fix" per comment #8, and per comment #11 we're NOT adding to the total height which was the only ever little fear about this patch.

Trunk:
http://mxr.mozilla.org/comm-central/source/mail/themes/qute/mail/messageHeader.css
http://mxr.mozilla.org/comm-central/source/mail/themes/gnomestripe/mail/messageHeader.css

Branch:
http://mxr.mozilla.org/comm-1.9.1/source/mail/themes/qute/mail/messageHeader.css
http://mxr.mozilla.org/comm-1.9.1/source/mail/themes/gnomestripe/mail/messageHeader.css

Comment 21

[Mark Banner (:standard8)]

(In reply to comment #20)
> Mark, could we please check in this patch on trunk and branch?
> It has review+ and ui-review+ , so it's ready to land.

No, this hasn't landed on trunk and you need to request approval anyway (especially at the moment as I'm effectively holding branch closed due to the RCs).

It would also help to get unit tests in some form as well.

Comment 22

[Mark Banner (:standard8)]

Checked in on trunk: http://hg.mozilla.org/comm-central/rev/f5fb2080727c

Leaving open as Dan's request for unit tests and blocker status I think means we should get those before closing this bug.

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

David's convinced me that the work required to write this test would significant outweight the risk it addresses, so landing as is is fine.

Comment 24

[Andreas Nilsson (:andreasn)]

setting target milestone to 3.1b2 per Dan's suggestion so we can land it there after the tree reopens

Comment 25

[Andreas Nilsson (:andreasn)]

err... 3.1 beta 1 I mean.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Since this is already on the trunk, resolving as fixed.  Mark, does the fact that this has status-thunderbird3.0: wanted+ mean that it doesn't need approval to land there?  If it does still need approval, would you please nominate it or give approval?

Comment 27

[Mark Banner (:standard8)]

Comment on attachment 410813 [details] [diff] [review]
patch for trunk and 1.9.1 branch

Ok, I think this has baked long enough that we can land it on branch. Should be lowish risk anyway.

Comment 28

[Mark Banner (:standard8)]

This actually landed in time for a1.

Comment 29

[Mark Banner (:standard8)]

Checked into 1.9.1:

http://hg.mozilla.org/releases/comm-1.9.1/rev/f8a7076c6ca6