This bug is for implementing the new features for new/edit card inline on the message header display (see URL). See web page for details. Some of the work will be done on this bug, some may be moved out to other bugs.

This is the first WIP version for the "Stars" part. Note, some element of this patch require latest trunk (i.e. as of 5 mins ago). Implements: - Empty/Full star depending on contact status in address book - Clicking on star will bring up either of the existing Add Card or Edit Card dialog boxes - Email and image have been separated in terms of focus. Comments/Todo: - Clicking on the name will still bring up the popup menu, which still says "Add to Address Book". - Stars probably need a bit of resizing, we probably also need some css love and other work. - Do we want to drop the triangles? - Bryan, do you want us to always show the email address AND always show the display name from the address book if there is one (as opposed to the display name in the email?)? - I expect it will need some accessibility work. I'll attach an image in a mo.

Mark, can you make sure that the star is vertically centered? Currently its a bit lower than the display name. Further I think we should place it in-front of the display name because with the drop down arrow in between it looks a bit misplaced.

(In reply to comment #3) > Mark, can you make sure that the star is vertically centered? Currently its a > bit lower than the display name. Further I think we should place it in-front of > the display name because with the drop down arrow in between it looks a bit > misplaced. > See my todo comments "we probably also need some css love" and "do we want to drop the triangles.

(In reply to comment #4) > (In reply to comment #3) > > Mark, can you make sure that the star is vertically centered? Currently its a > > bit lower than the display name. Further I think we should place it in-front of > > the display name because with the drop down arrow in between it looks a bit > > misplaced. > > > See my todo comments "we probably also need some css love" and "do we want to > drop the triangles. Sorry, that was a bit harsh. Comments welcome, but please bear in mind what I've already listed above.

Mark, if you mean with css love the linking of the pictures by list-style-image/background-image, then it's okay. If not, please don't use hard links for the images, as it makes it hard for themers to give them other names or do things like hover effects etc.

(In reply to comment #6) > Mark, if you mean with css love the linking of the pictures by > list-style-image/background-image, then it's okay. If not, please don't use > hard links for the images, as it makes it hard for themers to give them other > names or do things like hover effects etc. > Yeah, that's fine, I just did it that way to start off with to get something going.

(In reply to comment #1) > - I expect it will need some accessibility work. Probably Marco can give some advices here, if he has time for.

(In reply to comment #8) > (In reply to comment #1) > > - I expect it will need some accessibility work. > > Probably Marco can give some advices here, if he has time for. Marco, rather than waste your time now, let me get a few more iterations of the patch out - I know the sort of things you need and the markup is far from set in stone.

1) Thunderbird 2.x has such a clean professional look in the headers pane. I think the stars ruin that and make Thunderbird look more like a toy. Besides, I think that stars are generally used for "Favorites", and not all people in my address book are favorites. In any case, I'm hoping that I'll be able to hide the stars with a "display: none" in my userchrome.css (and doing so won't break Thunderbird). > - Bryan, do you want us to always show the email address [...] 2) I'm concerned about the proposals that would remove the option to hide email addresses for known senders. Please let us hide these email addresses instead of undoing all the effort of bug 240138. I wrote more about this in bug 448692 comment 9. > - Bryan, do you want us to [...] always show the display name > from the address book if there is one > (as opposed to the display name in the email?)? 3) I hope that bug 243631 will answer "yes" to this question.

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #1) > > > - I expect it will need some accessibility work. > > Probably Marco can give some advices here, if he has time for. > Marco, rather than waste your time now, let me get a few more iterations of the > patch out - I know the sort of things you need and the markup is far from set > in stone. All right! I'll watch this bug, so just give me a holler if you'd like me to test out a patch. I'm set up to build Thunderbird, so can check out patches right away if needed.

Switching for b1 flags to target milestones, to avoid flag churn.

(In reply to comment #1) > - Clicking on star will bring up either of the existing Add Card or Edit Card > dialog boxes No auto-add contact yet? > Comments/Todo: > - Clicking on the name will still bring up the popup menu, which still says > "Add to Address Book". I'm not too worried about this, even though it's redundant it should provide some legacy functionality. > - Stars probably need a bit of resizing, we probably also need some css love > and other work. This might be some amount of font selection and sizing as well. > - Do we want to drop the triangles? I'm afraid I don't have the triangles in my view, what do they do? Visually they are distracting elements. > - Bryan, do you want us to always show the email address AND always show the > display name from the address book if there is one (as opposed to the display > name in the email?)? I think we should be defaulting to the display name in the address book, but we should have a better solution than to silently hide this kind of information. Not sure what that is now, but it's not nothing. :) > - I expect it will need some accessibility work. Likely similar to the email link we'll need to at least make these focusable.

This patch doesn't have any UI changes on the header display, it only changes some of the popup menu options. In this patch I have concentrated on the mechanics of working out what is in the address book, and coping with the effect of changes in the address on the header display and popup menu. I'd like to get this in its current state especially as it is a big uplift of functionality of what we have so far, and it will let it have some wider testing. The patch depends on the one in bug 452614. This patch does the following: - Synchronises the popup menu (when clicking an email address) on the standalone message window with that on the 3-pane window - Depending on if the email address has a contact associated with it in an address book the popup menu will display "Add to Address Book...", "Edit Contact..." or "View Contact". The view case being for read-only address books. - When data is added/changed/deleted in the address book, the header will be updated in an appropriate fashion. Where possible I have tried to avoid re-searching all address books after a change has been made. Other Notes: - Small tidy up of existing functions - The addition of the new function to the mailWidgets.xml shouldn't affect SM - I've not implemented the function to add to the address book on selection of menu item (i.e. equivalent of clicking the star), I'll do that when I do the stars. - We keep track of the address book a card belongs in and the card, so that we have better potential for updating at the right times. - Searches for cards will work across mork and OS X address books because they are the only ones that currently implement cardForEmailAddress. Requesting ui-review because of the new strings I'm adding.

I forgot to mention, I've also gone for using "Contact" rather than "Card". It seemed to make more sense in this case, and I know Bryan and I have discussed moving towards it previously.

When I click on a address which is in the collectet AB with only the email address, TB offers me now to edit the contact. That's okay, but when I end to edit the card, the contact rests in collected AB. But now it's not only a collected address, it's now a address I want to do more. For this I have to open the address book window and move the edited address to my preferred AB. Better would be if the edit card dialog shows like on new card dialog the menulist with the ABs and I can choose the AB to save in.

(In reply to comment #10) > 1) Thunderbird 2.x has such a clean professional look in the headers pane. I > think the stars ruin that and make Thunderbird look more like a toy. Besides, > I think that stars are generally used for "Favorites", and not all people in my > address book are favorites. In any case, I'm hoping that I'll be able to hide > the stars with a "display: none" in my userchrome.css (and doing so won't break > Thunderbird). I agree about the stars. I think a tiny address book icon would be more meaningful.

(In reply to comment #17) > (In reply to comment #10) > I agree about the stars. I think a tiny address book icon would be more > meaningful. My thought on this is that using the star is a direct relation to Firefox's existing UI for adding to bookmarks. The advantage with this is that users who use Firefox will be immediately familiar with the idea and know what to expect. Although using an address book icon is also a possibility, but I think it is harder to show the not in address book case with that.

Updated patch to fix bitrot (no other changes)

This patch implements the inline edit contact popup. Much of this code is based on Firefox's Edit Bookmark dialog. It implements the dialog as per the spec given in the URL. I've tested this on Mac and Linux and the basics seem to work. I'm requesting reviews as I'd like to get some initial feedback on the code/design as soon as possible, items outstanding that I will check tomorrow are: - Any focus/accesskey problems on Linux - Check through accessibility for the popup (Marco I'll probably ping you tomorrow once I've had a look at a few things). - Sort out License headers - Final check for main review. I'll attach a screenshot of the dialog on Linux in a mo - I'll do one for Mac tomorrow. I'm planning two more patches, the first one will change "Add to Address Book" to save the contact straight away, the second one will implement the new icon.

The Mac version - showing both the Edit and View versions (I was able to do this as the Mac OS X Address Book interface currently only supports read-only mode).

(In reply to comment #19) > Created an attachment (id=336332) [details] > Implement the "mechanics" v2 > > Updated patch to fix bitrot (no other changes) Mark, can you update this patch? It can't be added to the latest trunk code.

(In reply to comment #23) > (In reply to comment #19) > > Created an attachment (id=336332) [details] [details] > > Implement the "mechanics" v2 > > > > Updated patch to fix bitrot (no other changes) > > Mark, can you update this patch? It can't be added to the latest trunk code. Boying, this applies fine for me (I just checked I was up to date). You may need to hg import (or hg qimport) it if it doesn't apply with patch (check the help for the command options).

Updated version for the inline edit contact popup. This fixes some accessibility mistakes that I found (bad spelling), improves what happens with focus and command keys, fixes license headers. Bryan and I need to have a discussion on what to do with an invalid email address (see comment in code). I've also noticed one other problem: changing the email address doesn't "de-link" the email in the header (it does if you switch to a different message and back again). I know why this is - I haven't figured out how to fix it yet (and its not quite part of this patch).

This patch depends on the one in bug 453301 and changes the menu popup actions so that "Add to Address Book" will add the email address/display name to the address book straight away. I've added another string which doesn't have the "..." because this action is now doing something without a dialog. The "..." version of the string is still required for right-clicking addresses within mails (which also brings up a dialog, we may want that to be part of a separate bug).

Comment on attachment 336332 [details] [diff] [review] [checked in] Implement the "mechanics" v2 yeah, this looks great

Comment on attachment 336332 [details] [diff] [review] [checked in] Implement the "mechanics" v2 Checked in, changeset id: 255:bd65ac0df8c2

(In reply to comment #28) > (From update of attachment 336332 [details] [diff] [review]) > Checked in, changeset id: 255:bd65ac0df8c2 Actually, make that changeset id: 256:0902a3a7d1a7

(In reply to comment #25) > Bryan and I need to have a discussion on what to do with an invalid email > address (see comment in code). Yeah, this makes me wonder if we should be including the email address for editing. Seeing it now and the validation problems it's incurring I'm wondering if we should only be showing the name. People might want to remove the NOSPAM additions for newsgroups, but we still need to associate that NOSPAM email with the person if we want to have some kind of user searching / linking that actually works. I'll try to catch you on IRC later today if possible. > I've also noticed one other problem: changing the email address doesn't > "de-link" the email in the header (it does if you switch to a different message > and back again). I know why this is - I haven't figured out how to fix it yet > (and its not quite part of this patch). I'm not too worried about this, seems like a relatively minor thing.

Part 1 is saying "Error: aDocumentNode.cardDetails is undefined Source File: chrome://messenger/content/msgHdrViewOverlay.js Line: 1029" on the first message load, with my tricksy OS X ab with a mailing list in it. I would, of course, do the right thing and file a new bug blocking this one, but hey! this is still open (thanks to my slothful reviewing habits).

(In reply to comment #31) > Part 1 is saying "Error: aDocumentNode.cardDetails is undefined Source File: > chrome://messenger/content/msgHdrViewOverlay.js Line: 1029" on the first > message load, with my tricksy OS X ab with a mailing list in it. This is really because the OS X Address Book interface is sending an onItemAdded alert when it shouldn't be. I've raised bug 454222 for that. I'll probably add an additional check in here, though I may add output if it fails - the code is designed such that all the email address nodes will have a cardDetails attribute when they are defined, if they don't it is broken. In the bug 454222 case we're actually getting a notification whilst we're generating the first node, hence the error - afaik it doesn't actually affect anything.

3.0b1 flag is going away in favour of 3.0 flag and milestone combination.

Comment on attachment 336463 [details] [diff] [review] Implement the inline Edit Contact popup v2 Looks great, except: >+ for each(var key in this._blockedCommands) { twice over. As http://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Statements/for...in#Description says, if a foolish extension extends Array.prototype (as McAfee did, and others doubtless still do), then you also get whatever random properties they add. My first thought, that you could get away with an |if (!elt) continue|, was also wrong, since they could wind up passing you something that happens to be the id of an element. r=me with those both changed to be boring and tiresome old-school numeric for(;;) loops.

Following discussions we've had elsewhere, we have decided to make the email address read-only. We're going to give it a go like this - the email can always be edited in the full dialog anyway. This is the new version of the mac dialog, the top picture shows the effect with the email selected - I've kept it with a grey background if it is read-only, but the focus highlight is still there. The name field goes from white to grey, depending on if it is selected or not. On Windows, the fields don't change colour (as per the FF edit bookmarks dialog), but read-only fields get a grey background. I'm not going to attach a picture of Windows/Linux as it is virtually the same as last time.

New version of the source code - this is similar to the last one, but I changed some items so that we block commands when on the read-only email address, and added some new styling. Therefore requesting review again to check this is still ok.

Comment on attachment 336484 [details] [diff] [review] [checked in] Implement immediate add Now the inline edit contact is almost ready, this one can go in as well. See comment 26 for what I'm doing here and why.

(In reply to comment #35) > This is the new version of the mac dialog, the top picture shows the effect > with the email selected - I've kept it with a grey background if it is > read-only, but the focus highlight is still there. The name field goes from > white to grey, depending on if it is selected or not. So which 'name' field we have here? First name, last name or display name? I would think the latter one. Shouldn't we describe it in a better way and use the full string?

(In reply to comment #35) > Created an attachment (id=338070) [details] > Inline Edit Contact on Mac (read-only email) > > Following discussions we've had elsewhere, we have decided to make the email > address read-only. We're going to give it a go like this - the email can always > be edited in the full dialog anyway. I'm wondering if we should go with "-moz-appearance: none;" for the email textfield so it just looks like a label instead of an entry. I'm a little uncomfortable giving people a read-only text entry as I don't think our interface explains _why_ it's read only.

On Windows the two buttons (Cancel, Done) are in the wrong order. The Cancel should be on the rightmost position. This was also a problem in Firefox.

The dialog buttons are now the right way round on Windows (I'd forgotten about that, so thanks for the reminder), I've also updating the styling to be based around -moz-appearance: none. New images in a while.

The new inline edit/view contact dialog on Linux and Mac.

Comment on attachment 338286 [details] Inline Edit Contact on Linux and Mac These look great! I'd say we're good to go.

Comment on attachment 336484 [details] [diff] [review] [checked in] Implement immediate add Also looks good

Comment on attachment 336484 [details] [diff] [review] [checked in] Implement immediate add r=me with a more semantic change in key than "AddToAddressBook1" - AddToAddressBookNow or Immediately or AddDirectlyToAddressBook (or even in a moment of desperation, 2 - 1 is way, way too easy to miss).

Comment on attachment 336484 [details] [diff] [review] [checked in] Implement immediate add Checked in, changeset id: 326:35d1c8c8e359

Comment on attachment 338283 [details] [diff] [review] [checked in] Implement the inline Edit Contact popup v3 Checked in, changeset id: 325:51131d4c50ad

(In reply to comment #15) > I forgot to mention, I've also gone for using "Contact" rather than "Card". It > seemed to make more sense in this case, and I know Bryan and I have discussed > moving towards it previously. Does this mean, we'll change the wording from "Card" to "Contact" in future builds? If yes, is there an existing bug to do this? I'd vote to change the wording.

(In reply to comment #48) > (In reply to comment #15) > Does this mean, we'll change the wording from "Card" to "Contact" in future > builds? If yes, is there an existing bug to do this? I'd vote to change the > wording. I've just raised bug 455246 for this change.

Not sure how I missed this. I forgot to add the editContactOverlay items to messageWindow.xul which means the standalone message window is broken. This should improve matters (this doesn't fix the issue with show only display name being off).

This patch fixes up the address book email address detection so that we still show the Edit Contact option (and dialog) even if "Show only display name for people in my address book" is switched off. I've also done a few changes so that we work better when the address book is updated.

Comment on attachment 338913 [details] [diff] [review] [checked in] Fix the standalone message window. Checked in, changeset id: 381:c141ff9ff184

Comment on attachment 339044 [details] [diff] [review] [checked in] Make the address book card detection work whether or not the condense pref is set. Checked in a couple of days ago, changeset id: 357:930977fe94b0

couple things: -Left click brings up context menu on the header (also posted that in bug 449691). Not expected, while contacts in header look like links -icon like person shape with little star vs little question mark, maybe different color. Alt text with explanation on that (already in AB/ not in AB) ? -this is more important for adding new card, but in edit contact why would I edit only the name? is that a common usage or just focused now on "add new" part of it?

(In reply to comment #54) > couple things: > -Left click brings up context menu on the header (also posted that in bug > 449691). Not expected, while contacts in header look like links This is part of the design that hasn't been fixed yet (and is actually still the way Thunderbird 2 currently works). The suggestion is that the text will be selectable, with a star, and a drop down. > -icon like person shape with little star vs little question mark, maybe > different color. Alt text with explanation on that (already in AB/ not in AB) ? The icon at the moment reflects the add/edit bookmark UI in Firefox. I expect this was why it was chosen. I'd like to know what Bryan thinks on this point. > -this is more important for adding new card, but in edit contact why would I > edit only the name? is that a common usage or just focused now on "add new" > part of it? For adding a new card, we'll do it automatically (as best we can), so that if you want to remember someone's address, you can just select the icon and add it without further ado. You can then go back later and change it if you want. The most likely thing on a contact you'll want to change after you've added it is the name, if not, you've still got easy access to the full set if properties to edit the whole card.

(In reply to comment #55) > > -icon like person shape with little star vs little question mark, maybe > > different color. Alt text with explanation on that (already in AB/ not in AB) ? > > The icon at the moment reflects the add/edit bookmark UI in Firefox. I expect > this was why it was chosen. I'd like to know what Bryan thinks on this point. Bookmark/star is ok, only I was thinking to show that I'm bookmarking a contact [thus person +star], cause there are also stared messages showing in thread. Confusion .. Otherwise is very good to use and resemble FF panel for all reasons

Some thoughts: If I have more than one contact with the same e-mail address will the display name be matched too? or will I be able to choose which to edit? When creating a new contact, will the first name and last name fields be filled, or only the display name? If the former, do "John Smith" and "Smith, John" both get first name = "John"?

(In reply to comment #57) > Some thoughts: > If I have more than one contact with the same e-mail address will the display > name be matched too? or will I be able to choose which to edit? At the moment it'll just display the first one it finds. I suspect, as you suggest, the best way to sort out duplicates would be to match on the display name as well. I've raised bug 456011 to cover this. > When creating a new contact, will the first name and last name fields be > filled, or only the display name? If the former, do "John Smith" and "Smith, > John" both get first name = "John"? This will work in the same way as address collection currently. Which is splitting based on the space only. We need to improve that, I know there is at least one bug on this, but I can't find it at the moment.

This implements the star icon on the message header display. I have also done some more fixes and tidy up for working if the condense pref is not set, and also to fix other issues I found as I went along. The icons are the same used for the edit bookmarks in Firefox. I haven't implemented the changes for the selection of the text, that were previously mentioned offline, I think this should be good enough for the first iteration.

Comment on attachment 339792 [details] [diff] [review] Implement Star UI on message header ++

Minor change so that we don't change positions of stars when we click to add to address book (just in case of accidental double click).

Here's the design for the drop down arrow placement and hover effect. http://clarkbw.net/designs/add-contacts/email%20address%20header%20mockup.png (note: the gray border is only to indicate padding for the element and is not intended to be included in the implementation)

Comment on attachment 339820 [details] [diff] [review] [checked in] Implement Star UI on message header v2 >diff --git a/mailnews/base/resources/content/mailWidgets.xml b/mailnews/base/resources/content/mailWidgets.xml >--- a/mailnews/base/resources/content/mailWidgets.xml >+++ b/mailnews/base/resources/content/mailWidgets.xml >@@ -463,12 +463,28 @@ > </binding> > > <binding id="mail-emailaddress"> >+#ifndef MOZ_THUNDERBIRD > <content popup="emailAddressPopup" context="emailAddressPopup"> > <xul:description anonid="emailValue" class="emailDisplayButton plain" > xbl:inherits="xbl:text=label,crop" flex="1"/> > <xul:image class="emailDisplayImage" anonid="emailImage" > xbl:inherits="src=image"/> > </content> >+#else >+ <content> >+ <xul:description anonid="emailValue" class="emailDisplayButton plain" >+ context="emailAddressPopup" popup="emailAddressPopup" >+ xbl:inherits="xbl:text=label,crop,tooltiptext=tooltipemail" >+ flex="1"/> >+ <xul:image class="emailStar" anonid="emailStar" >+ context="emailAddressPopup" >+ onclick="onClickEmailStar(event, this.parentNode);" >+ xbl:inherits="hascard,tooltiptext=tooltipstar"/> >+ <xul:image class="emailPopup" anonid="emailPopup" >+ context="emailAddressPopup" popup="emailAddressPopup" >+ xbl:inherits="hascard,tooltiptext=tooltipemail"/> >+ </content> >+#endif Forking the widget using the preprocessor like this seems wrong to me. Why not just add a <property> (maybe "onstarclick"?) that allows the user of the widget to specify whether they want a star or not?

FWIW, in order to implement the fuller UI as described in comment #62, I'm (so far) moving to a more deeply nested content area for that widget in Tb. I don't think having a preprocessor-forked widget is ideal, but the alternatives (different widgets sharing a lot of implementation, XBL subclassing with all its gotchas) are all at least as ugly.

I've got most of the UI that clarkbw describes in comment #62. Still to do: - fix baseline alignment issues (they occur in expanded and compact view, as well as in standalone message view) - fix error on clicking on arrow on hover - clarkbw has some other backgrounds in mind - cleanup - port to qute

(In reply to comment #63) > Forking the widget using the preprocessor like this seems wrong to me. Why not > just add a <property> (maybe "onstarclick"?) that allows the user of the widget > to specify whether they want a star or not? There were two things I saw - first the context/popup menus needed to be different, secondly I am expecting that SeaMonkey will want to pick up something like this UI as well, and hence this would be a temporary fork - but because SeaMonkey is frozen for Alpha, I didn't want to affect them. What we should probably do post alhpa/beta, is to find out if SM want the same UI that we end up with or not, and if SM doesn't, then we branch the mail-emailaddress widget into two xml files (one in suite/ the other in mail/).

With Star UI enabled I see following: open message with starred contacts. The tooltips on the contacts shows the correct addresses. When I now change to a message without starred contacts, the tooltips appears still with the addresses from the starred contacts.

(In reply to comment #65) > Created an attachment (id=339935) [details] > WIP: Implement "green bubbles" star UI > > I've got most of the UI that clarkbw describes in comment #62. Still to do: > I found an error in yesterdays build where the CSS syntax is messed up in the XUL for this new feature. .headerValueBox { margin: 0 0 5px 0; // XXX needed? }

(In reply to comment #64) > FWIW, in order to implement the fuller UI as described in comment #62, I'm (so > far) moving to a more deeply nested content area for that widget in Tb. I > don't think having a preprocessor-forked widget is ideal, but the alternatives > (different widgets sharing a lot of implementation, XBL subclassing with all > its gotchas) are all at least as ugly. (In reply to comment #66) > > What we should probably do post alhpa/beta, is to find out if SM want the same > UI that we end up with or not, and if SM doesn't, then we branch the > mail-emailaddress widget into two xml files (one in suite/ the other in mail/). What's not clear to me is why it wouldn't be dead simple to have a single shared widget, with consumers specifying attributes like displaystar, onstarpopup, and onemailpopup. That said, given where we are in the release process, accepting the ifdef fork seems reasonable for the moment.

Comment on attachment 339820 [details] [diff] [review] [checked in] Implement Star UI on message header v2 >diff --git a/mail/base/content/messenger.css b/mail/base/content/messenger.css >--- a/mail/base/content/messenger.css >+++ b/mail/base/content/messenger.css >@@ -65,11 +65,18 @@ mail-messageids-headerfield { > > mail-emailaddress { > -moz-binding: url("chrome://messenger/content/mailWidgets.xml#mail-emailaddress"); >+} >+ >+.emailDisplayButton { > -moz-user-focus: normal; > } The focus behavior here is odd, in that these elements on Mac now accept visual focus, but it doesn't appear to be possible to activate them once they have it. Can you track this somehow for followon Tb3 timeframe work (spinoff bug, wiki, whatever...)? It's not entirely clear that it makes sense to have both the arrow and the address be focusable, since they do the same thing. >-function UpdateEmailNodeDetails(aEmailAddress, aDocumentNode, aCondenseName, >- aCardDetails) { >+function UpdateEmailNodeDetails(aEmailAddress, aDocumentNode, aCardDetails) { > // The directory for this card has been removed. Re-query to find > // any other cards. As far as I can tell, the above comment is only one of a number of situations that can cause this function to be called. How about just removing it? >+ // When we are adding cards, we don't want to move the display around if the >+ // user has clicked on the star, therefore if it is locked, just exit and >+ // leave the display updates until later. >+ if (aDocumentNode.hasAttribute("updatingUI")) >+ return; This |return| wouldn't appear to ever be executed in the patch as it stands. I assume this is needed for subsequent work? Otherwise looks good. r=dmose with any appropriate tweaks.

Comment on attachment 339820 [details] [diff] [review] [checked in] Implement Star UI on message header v2 Checked in, changeset id 410:7f9f210762cd

(In reply to comment #70) > (From update of attachment 339820 [details] [diff] [review]) > >diff --git a/mail/base/content/messenger.css b/mail/base/content/messenger.css > >--- a/mail/base/content/messenger.css > >+++ b/mail/base/content/messenger.css > >@@ -65,11 +65,18 @@ mail-messageids-headerfield { > > > > mail-emailaddress { > > -moz-binding: url("chrome://messenger/content/mailWidgets.xml#mail-emailaddress"); > >+} > >+ > >+.emailDisplayButton { > > -moz-user-focus: normal; > > } > > The focus behavior here is odd, in that these elements on Mac now accept visual > focus, but it doesn't appear to be possible to activate them once they have it. > Can you track this somehow for followon Tb3 timeframe work (spinoff bug, wiki, > whatever...)? It's not entirely clear that it makes sense to have both the > arrow and the address be focusable, since they do the same thing. I've added this to https://wiki.mozilla.org/User:Dmose:Message_Reader_Polish#Needed_for_Thunderbird_3

Attaching WIP (pinstripe theme only). It "works", except that I can't get the bubble to be bigger without causing alignment problems. Any suggestions welcome, or I'll get back to it tomorrow and talk to clarkbw about alternate design possibilities.

I think that showing bubbles is too loud anyway. For example, will we see all of these bubbles every time we move our mice from the preview pane to the top of the screen (even if we're not currently interested in the contents of the headers pane)? IMO it's more than sufficient to simply change the color of the text when the mouse passes over header fields.

Mark, one other point is that the enter key doesn't work in the edit contact panel, to select Ok.

Found a bug in this feature set. The tooltip is not resetting. After viewing a Signed message in Inbox I switched to Local > Drafts to look at how the Star worked there. On hovering the From: I saw the Tooltip still showing the info from the prior views message. I switched to a news account and hovered the posters display name in the header view. It also was showing the tooltip from the first viewed message.

(In reply to comment #76) > Created an attachment (id=340185) [details] > Screenshot of Tooltip error > > Found a bug in this feature set. The tooltip is not resetting. Please file these bugs as separate issues. Otherwise we'll end up with a bug full of comments and we'll be in danger of missing the problems. The only continuing work for this bug is the bubble ui.

(In reply to comment #77) > (In reply to comment #76) > > Found a bug in this feature set. The tooltip is not resetting. > > Please file these bugs as separate issues. Otherwise we'll end up with a bug > full of comments and we'll be in danger of missing the problems. Do You want the new bug linked to this one?

(In reply to comment #78) > (In reply to comment #77) > > (In reply to comment #76) > > > Found a bug in this feature set. The tooltip is not resetting. > > > > Please file these bugs as separate issues. Otherwise we'll end up with a bug > > full of comments and we'll be in danger of missing the problems. > > Do You want the new bug linked to this one? Probably best to set it blocking bug 456814 as that is the new tracker bug.

The bubble UI has now been moved to bug 457012, therefore this bug is now fixed.

The "Add to addressbook" button can be tabbed to, but neither Space nor Enter activate it. Am I missing something, or is this a case for a followup bug?

(In reply to comment #81) > The "Add to addressbook" button can be tabbed to, but neither Space nor Enter > activate it. Am I missing something, or is this a case for a followup bug? File a new follow up bug. This bug, for the mechanics, is closed as fixed. See Comment #79.

(In reply to comment #81) > The "Add to addressbook" button can be tabbed to, but neither Space nor Enter > activate it. Am I missing something, or is this a case for a followup bug? See bug 458635.

Adding test cases in Litmus : https://litmus.mozilla.org/show_test.cgi?id=7719 https://litmus.mozilla.org/show_test.cgi?id=7720 Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090518 Shredder/3.0b3pre