236219 - Unreliable highlighting in compose window when tabbing thru

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Mike Cowperthwaite]

Bug 215846 fixed the compose window so that if the attachments panel contains 
anything, it becomes part of the tab order.  (At this writing, requiring 
F6/Shift-F6 or Ctrl-Tab/Shift-Ctrl-Tab; just plain Tab does not pass thru the 
panel.)

Tabbing from Subject to Attachments to Body (using F6 or Ctrl-Tab), when the 
focus arrives at the attachments box, the attachment highlight goes from grey to 
black, but on leaving the attachments box), it remains black.

Reversing the process with Shift-F6/Shift-Ctrl-Tab handles the highlighting 
correctly.


To reproduce: Open a compose window.  Attach a file; then single-click the 
attachment entry to select it.  Move focus to the Subject box.

Type F6, F6.

Actual result:
Attachment selection becomes black on first F6, remains black on second F6.

Expected result:
Attachment selection becomes black on first F6, becomes grey on second F6.


xref bug 205666.

Comment 1

[Mike Cowperthwaite]

On further examination with Mozilla 1.7.11, the problem of the highlight not 
being cleared is not restricted to the move from Attachment box to Body; it also 
occurs when using Shift+F6 to move from the From field to the Body.  
Interestingly, shifting the focus from Subject to Body with F6 (when there is no 
attachment in the list) does clear the highlight from the Subject.

Comment 2

[Mike Cowperthwaite]

This is analogous to the issue where F6/Ctrl+Tab navigation of the main window 
does not clear the focus ring when the focus is shifting into the message pane 
with F6/Shift+F6 (in 1.7.x and earlier -- this was fixed in the suite as part of 
bug 203386).

Comment 3

[Mike Cowperthwaite]

Attachment: patch for TB (sorry, not a CVS diff -- this is against the 0805 build)

I did the same thing here that I did in the patch for bug 203386 to force the
focus-ring updating amongst the panes.	Simple, works great.

This also fixes the Reply and Edit-as-New cases described in bug 289339 -- a
compose window that opens to put the focus in the body initially will now have
the 'From' focus indicator cleared.

I'm not sure if it's worth caching the result of 
  document.getElementById("appcontent")
as is done for so many other elements such as with GetMsgIdentityElement(). 
Maybe it's more efficient, maybe it's not; it sure doesn't make the code any
easier to read, in my eyes.


Neil, I don't know if this patch drops in for the suite, but it's short and
easy.  I got the "appcontent" element name from the .xul file.

Comment 4

[Mike Cowperthwaite]

Comment on attachment 191891 [details] [diff] [review]
patch for TB (sorry, not a CVS diff -- this is against the 0805 build)

David, can I get a review from you on this?

Comment 5

[David :Bienvenu]

Comment on attachment 191891 [details] [diff] [review]
patch for TB (sorry, not a CVS diff -- this is against the 0805 build)

seems OK - I'll try it out in a bit.

Comment 6

[Mike Cowperthwaite]

(In reply to comment #5)
> (From update of attachment 191891 [details] [diff] [review] [edit])
> seems OK - I'll try it out in a bit.

David, any further feedback on this?

Scott, can you sr?

Comment 7

[Mike Cowperthwaite]

Attachment: diff -w vs. 0216 trunk distro

Added an additional fix for an erroneous reference to a nonexistent 
"bucketTree" that shows up while testing my changes.

The other bug that shows up while testing this (when trying to F6/Shift+Tab 
into the addressing widget) is bug 311217.  At the moment, that's beyond my
capacity to fix.

Comment 8

[Mike Cowperthwaite]

Attachment: diff -u vs. trunk (1.6a1) 0216 nightly distro

Thanks to Aleks Totic's work at bug 276075, I've now got a working Venkman for TB installed, and I was able to fix the problem at bug 311217 as well.  Asking for re-review and sr.

Comment 9

[David :Bienvenu]

Mike, I'll give this a try once I've cleaned up my mess on the other f6 patch

Comment 10

[David :Bienvenu]

Comment on attachment 212507 [details] [diff] [review]
diff -u vs. trunk (1.6a1) 0216 nightly distro

the one thing I noticed while testing this is that the selection always goes to the first attachment when cycling through with f6, even if a different attachment was selected (e.g., attachment #2 [details] [diff] [review]). I doubt this is a regression, but I'm just pointing it out. I'll attach the diff I have in my tree...

Comment 11

[David :Bienvenu]

Attachment: cvs diff

Comment 12

[Mike Cowperthwaite]

(In reply to comment #10)
> the one thing I noticed while testing this is that the selection always goes
> to the first attachment when cycling through with f6, even if a different
> attachment was selected (e.g., attachment #2 [details] [diff] [review] [edit]). I doubt this is a
> regression, but I'm just pointing it out. I'll attach the diff I have in 
> my tree...

Yeah it does -- the function that gets called is (and has been all along) 
|FocusOnFirstAttachment()|.  I have some ideas on fixing that, which I'll put into bug 303712 after this stuff gets checked in.

Comment 13

[Scott MacGregor]

Comment on attachment 212507 [details] [diff] [review]
diff -u vs. trunk (1.6a1) 0216 nightly distro

can we just remove 

+//  var element = document.getElementById("msgRecipient#" + awGetNumberOfRecipients());

instead of commenting it out?

Comment 14

[Mike Cowperthwaite]

(In reply to comment #13)
> can we just remove [code] instead of commenting it out?

Definitely; David's CVS diff already has done, in fact.  Thank you, Scott.

David, would you do the honors of checking that in?

Comment 15

[David :Bienvenu]

Comment on attachment 212610 [details] [diff] [review]
cvs diff

Mike, I'm assuming we want this for 2.0 - are there any other patches that this patch is dependent on?

Comment 16

[Mike Cowperthwaite]

(In reply to comment #15)
> are there any other patches that this
> patch is dependent on?

No, this is standalone, as far as I know.  It's the only patch I've made that affects the compose window.

Comment 17

[David :Bienvenu]

k, thx. Marking fixed on trunk. We'll let it bake a little and then put it on the branch.

Comment 18

[Scott MacGregor]

I've moved this to the branch.

Comment 19

[Mike Cowperthwaite]

Thanks, Scott.