Closed Bug 460647 Opened 13 years ago Closed 11 years ago

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

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
trivial

Tracking

(blocking-thunderbird3.0 -, thunderbird3.0 .2-fixed)

VERIFIED FIXED
Thunderbird 3.1a1
Tracking Status
blocking-thunderbird3.0 --- -
thunderbird3.0 --- .2-fixed

People

(Reporter: thomas8, Assigned: andreasn)

References

(Blocks 1 open bug)

Details

(Keywords: polish)

Attachments

(3 files)

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.
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
Version: unspecified → Trunk
I'm not seeing this on Mac nightlies either.
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
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.
OK, I'm seeing this on nightlies now as well.  I don't know that this should block tb3, but it might.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-thunderbird3?
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.
Agreed; it all depends on the set of things that we have to triage this against.
Keywords: polish
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.
Flags: blocking-thunderbird3? → blocking-thunderbird3-
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: --- → Thunderbird 3.0b3
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.
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?
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.
This is also relavant for
related Bug 521334 -  Nit: Improve header pane metrics (e.g. more actions button too big, right alignment, vertical space)
Summary: Message Reader: Focussing contacts in message header causes other headers to move down (polish) → Message Header: Focussing contacts causes respective header and label to move down some px (polish)
Blocks: 521334
(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)
Whiteboard: one-line patch, see comment #9
Here is ovidiu proposed changes as a patch. Seems to do the trick on Linux and Vista.
Attachment #410813 - Flags: ui-review?(clarkbw)
Attachment #410813 - Flags: review?(bwinton)
above before, below after (on linux)
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?)
Attachment #410813 - Flags: review?(bwinton) → review+
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.
Attachment #410813 - Flags: ui-review?(clarkbw) → ui-review+
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.
Attachment #410813 - Attachment description: implemented as patch on trunk → patch for trunk and 1.9.1 branch
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.
Flags: blocking-thunderbird3.1+
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
Keywords: checkin-needed
Whiteboard: one-line patch, see comment #9 → [checkin-needed][trunk and branch]
Target Milestone: Thunderbird 3.0b3 → Thunderbird 3.0rc1
(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.
Flags: in-testsuite?
Whiteboard: [checkin-needed][trunk and branch] → [checkin-needed][trunk]
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.
Assignee: nobody → nisses.mail
Keywords: checkin-needed
Whiteboard: [checkin-needed][trunk] → [checked in on trunk][needs unit tests][possibly land for 3.0.x]
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.1a1
blocking-thunderbird3.0: --- → ?
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.
blocking-thunderbird3.0: ? → -
setting target milestone to 3.1b2 per Dan's suggestion so we can land it there after the tree reopens
Target Milestone: Thunderbird 3.1a1 → Thunderbird 3.1b1
err... 3.1 beta 1 I mean.
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?
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: blocking-thunderbird3.1+ → wanted-thunderbird+
Resolution: --- → FIXED
Whiteboard: [checked in on trunk][needs unit tests][possibly land for 3.0.x] → [checked in on trunk][possibly land for 3.0.x]
Attachment #410813 - Flags: approval-thunderbird3.0.2?
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.
Attachment #410813 - Flags: approval-thunderbird3.0.2? → approval-thunderbird3.0.2+
This actually landed in time for a1.
Target Milestone: Thunderbird 3.1b1 → Thunderbird 3.1a1
Checked into 1.9.1:

http://hg.mozilla.org/releases/comm-1.9.1/rev/f8a7076c6ca6
Whiteboard: [checked in on trunk][possibly land for 3.0.x]
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.