Closed Bug 289339 Opened 19 years ago Closed 18 years ago

Focus indication left on From: (identity) dropdown for cached HTML compose windows

Categories

(Thunderbird :: Message Compose Window, defect)

Product:
Thunderbird
Component:
Message Compose Window
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: mcow, Assigned: mscott)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file)

Stops the focus being set on the identity menu in onReopen()
788 bytes, patch
Bienvenu
: review+
Details | Diff | Splinter Review 
When opening a new compose window with, e.g., Reply, the From: identity selector 
appears as if it has the focus, when in fact the focus has been properly moved 
to the Subject field.  This happens differently depending on:
  1) which command is used to open the compose window;
  2) whether the account composes in plain or HTML;
  3) whether or not the Shift button is used on a toolbar;

On point 3: commands from the menu, from shortcuts, or from an unshifted toolbar 
all behave the same, but the shift+toolbar causes slightly different results 
depending.

If the window is opening "new" (that is, a plain-text compose window when the 
previous open compose window was HTML, and vice-versa, and for the first window 
opened at startup), I don't see this symptom.

The focus itself always ends up in the correct field.

With TB 1.0+0406, I am seeing the following; "yes" means the focus indication is 
incorrectly on the From:, "no" means it displays as expected.

    Acct composes: plain    HTML
  New:              no       yes
  Shift+New:        yes      no
  Reply:            yes      yes
  Shift+Reply:      yes      yes
  Reply All:        yes      yes
  Shift+Reply All:  yes      yes
  Fwd Inline:       no       yes
  Shift+Fwd/in:     no       yes   [xref bug bug 254931]
  Fwd Attach:       no       yes
  Shift+Fwd/at:     yes      no

  Edit As New:
    HTML:           yes      yes
    plain:          yes      yes

In summary: the bogus focus indication appears for all cases of an HTML window 
being opened; for a plain text window, the symptom does not appear for a New 
(Write) or for a Forward (inline or attachment), but does appear for the Reply, 
Reply All, and Edit As New cases.

I'm reporting this under Thunderbird, but the symptom manifests under Mozilla as 
well.
I am seeing this bug as described with the official 1.0 on WinXP.  I first tried
to remove the focusing of the identity list in
messenger/content/messengercompose/MsgComposeCommands.js lines 216-218 but then
the caret would sometimes not be visible in the focused element (hence the
comments in the code).  Then I tried changing the element to focus on to
'content-frame' and the problem went away.  I am not seeing any messages in the
Javascript console either.  Not sure what that affects though or if another
element should be chosen or if this is bug should be handled differently.
The fix for bug 236219 is in the trunk (at least for TB) and will likely be 
part of the branch in time for 2.0.  That fix has eliminated the problem reported here for replies and edit-as-new; however, the symptom still exists 
for HTML compose windows that are created via New or Forward.

To update the matrix from comment 0:

    Acct composes: plain    HTML
  New:              no       yes
  Shift+New:        yes      no
  Reply:            no       no
  Shift+Reply:      no       no
  Reply All:        no       no
  Shift+Reply All:  no       no
  Fwd Inline:       no       yes
  Shift+Fwd/in:     no       yes   [xref bug bug 254931]
  Fwd Attach:       no       yes
  Shift+Fwd/at:     yes      no

  Edit As New:
    HTML:           no       no
    plain:          no       no
Summary: Focus indication left on From: (identity) dropdown for cached compose windows → Focus indication left on From: (identity) dropdown for cached HTML compose windows
What does Shift+Write do differently to Write? (I presume that's the same as "Shift+New" that you list)

I notice that it not only doesn't show the bug, but the next compose window opened after it also won't show the bug, even if it's supposed to. The one after that will, however. Looks like something else is being cached or saved between compose windows that is meddling with the new window focus.
(In reply to comment #3)
> What does Shift+Write do differently to Write? (I presume that's the same as
> "Shift+New" that you list)

Shift+Write [New], or +Reply, or +Forward (when forwarding as attachment) acts as a toggle to the opposite compose mode (plain or HTML) normally used by the identity.

> I notice that it not only doesn't show the bug, but the next compose window
> opened after it also won't show the bug, even if it's supposed to. The one
> after that will, however. Looks like something else is being cached or saved
> between compose windows that is meddling with the new window focus.

The current incarnation of this problem (e.g. since comment 2) shows the 
problem only for HTML compose windows.  If you are set up to compose-as-HTML 
and were using Shift to test, then you wouldn't see the symptom.
(In reply to comment #4)
> Shift+Write [New], or +Reply, or +Forward (when forwarding as attachment) acts
> as a toggle to the opposite compose mode (plain or HTML) normally used by the
> identity.

Thanks. Learnt something new :)

> The current incarnation of this problem (e.g. since comment 2) shows the 
> problem only for HTML compose windows.  If you are set up to compose-as-HTML 
> and were using Shift to test, then you wouldn't see the symptom.

Oh, no, I mean that AFTER composing one using shift-write (which would mean composing as plain text, in my case), then the next one I bring up using normal write (HTML text) also doesn't have the bad focus. So it looks like something that is saved during/after composition is having an effect on the next window that opens (much like the other focus bug did).

However I can only reproduce that effect using that particular situation. If I open another kind of composition window that doesn't show the bug (say, a reply), it won't affect the next new window I open the same way that the plain text compose did.
Yeah: plain text and HTML composer windows are different, and so if one follows the other, the cache is discarded and a new window opened.  The problem I'm talking about is only when an HTML (New or Forward) follows some other HTML window.  I'm guessing the problem resides in the re-init or the cleanup code, similar to what you were looking at in bug 282669.
Yeah, that makes sense. It basically only seems to occur if the focus is placed on the address window. But tonight I've only had enough time to check that the bug doesn't occur if you make new/forward windows set the focus on the body, instead of calling awSetFocus() (in AdjustFocus() in MsgComposeCommands.js).
Er, I meant address field, not window.
OS: Windows 2000 → All
Hardware: PC → All
I wonder if this is somehow analogous to another bug you reported (and provided a fix for): bug 236219.

In MsgComposeCommands.js AdjustFocus(), just before awSetFocus() is called, if I call document.getElementById("appcontent").focus(); Then the problem never occurs. As if calling top.awInputElement.focus() directly (which is what usually happens in that chain) fails to properly remove the old focus, just as calling window.content.focus() failed to in your previous bug. My guess is if you never put in the fix for the former bug, we'd be seeing the same problem occur for reply messages.

If this is correct, then the questions remain: why is this only happening for a HTML composition window? And why only when it follows another HTML composition window? Is something being saved that impedes the focus ring being removed?
(In reply to comment #9)
> In MsgComposeCommands.js AdjustFocus(), just before awSetFocus() is called,
> if I call document.getElementById("appcontent").focus(); Then the problem
> never occurs. As if calling top.awInputElement.focus() directly (which is
> what usually happens in that chain) fails to properly remove the old focus,
> just as calling window.content.focus() failed to in your previous bug.

Interesting.  It sort-of makes sense that window.content.focus() doesn't 
shift the focus indicator, if window.content isn't an 'element' -- but 
the address field *is* an 'element' and so it should go thru the code that removes the focus indicator from the current element.  I see in addressingWidgetOverlay.js that the actual setting-of-focus takes place via a timer function, I wonder if that has something to do with this.  

I also wondered if that was necessary -- on the face of things, it appears that the timer was used so retries could be attempted, but all the retry code has been commented out.  But when I simply inlined the focus action into awSetFocus(), when the window was opened the focus ended up on the body rather than the address widget.  However: in this case, the focus indicator did not appear on the identity dropdown, so maybe the timer *does* somehow cause this bug.

> My guess is if you never put in the fix for the former bug, we'd be seeing
> the same problem occur for reply messages.

Indeed; see comment 0.


> If this is correct, then the questions remain: why is this only happening
> for a HTML composition window?

The focus setting chain here appears to be:
  ComposeFieldsReady()
    AdjustFocus()
       awSetFocus(0, <field>)
          [set timer]
  timer->
    _awSetFocus()
       <field>.focus()

ComposeFieldsReady() has (before it sets focus):
===========
  // need timeout for reply to work
  if (gMsgCompose.composeHTML)
    setTimeout("loadHTMLMsgPrefs();", 0);
===========
Could be some issue of the two timer-based routines going off at about the same time?


> And why only when it follows another HTML composition window? 
> Is something being saved that impedes the focus ring being removed?

It must have to do with the state of the cached window, but I'm at the ragged edge of my understanding here.


If Neil has some ideas on making this whole process cleaner, I'd like to hear them; otherwise, a patch along the lines you suggest will at least nail this bug.
(In reply to comment #10)
>Interesting.  It sort-of makes sense that window.content.focus() doesn't 
>shift the focus indicator, if window.content isn't an 'element' -- but 
>the address field *is* an 'element' and so it should go thru the code that
>removes the focus indicator from the current element.
I'm sure this bug would be resolved if it removed the focus from the current element like it should, but I couldn't understand the window focus code :-(
Actually, scratch that... I failed to notice that with the extra focus call, the focus doesn't appear to be set anywhere, at all, for write and forward compositions. It's like bug 282669, but all the time.

Regarding the setTimeout("loadHTMLMsgPrefs();", 0); line... I tried commenting that out a couple of days ago and I'm pretty sure it did nothing to affect this bug. It must be something else in the html composition code that is doing it.
(In reply to comment #12)
> Actually, scratch that... I failed to notice that with the extra focus call,
> the focus doesn't appear to be set anywhere, at all, for write and forward
> compositions. It's like bug 282669, but all the time.

Wait... scratch *what*?  Do you mean the patch you suggested isn't working for you?  I've been running with the modification, and I'm seeing the focus set where I expect it.


> Regarding the setTimeout("loadHTMLMsgPrefs();", 0); line... I tried
> commenting that out a couple of days ago and I'm pretty sure it did nothing
> to affect this bug. It must be something else in the html composition code
> that is doing it.

OK.
You're right. Sorry! That focus change doesn't cause the bug I described. At least that explains why I didn't notice it till later. I must have accidentally changed something else.

One thing I've noticed on Mac (trunk) builds is that the focus is even more persistent. The bug also occurs for plain text compositions, and I can't get rid of the focus by clicking on the accounts menu. The only time it's not present is when the composition type is different to the previous one. I wonder if that means it's not necessarily a html compose problem after all.

Ah well, I'll keep looking...
It looks like something else with a time delay (possibly of 0) is screwing with the focus as well. I'm not sure if that's the cause of this bug or it's another one.

If I change the code in MsgComposeCommands.js AdjustFocus() to:
    //awSetFocus(awGetNumberOfRecipients(), element);
    element.focus();

Then the bad focus ring is gone, BUT the focus switches rapidly from the address field to the message body.

If I leave the call to awSetFocus() and place the element.focus() call in there instead, and comment out the call to _awSetFocus(), the exact same thing happens. If I restore the timed out call to _awSetFocus() (which is called with a time out of 0), then the focus returns to the address field (but the identity focus ring is still gone).

So it looks like a timed call from elsewhere is changing the focus, and it looks like the position it switches it to is dependent on its current location.

Supporting evidence, if I place some WhichElementHasFocus().id checks in there:
The focus is "undefined" the first time that a compose window is loaded and AdjustFocus() and awSetFocus() is called. Subsequent (buggy) times, it's "msgIdentity". However, when _awSetFocus() is called by the time out, it's been changed to "undefined" again.
Mike, could you (or anyone else reading this) please check out the following in your own builds? :)

Despite the comment in onReopen(), I can't find any reason to set the focus to the identity menu when the compose window reloads. As far as I've tested so far, there are no "undesirable visual effect"s caused by deleting that code. But there is one desirable effect, in that the bad focus ring on the menu is gone. Perhaps the code has changed a lot since setting that focus was important. Or perhaps I've just missed something.

It looks to me like the presence of that code in onReopen() was the reason the bug only appeared after the same compose window type was opened twice. And so it's probably nothing to do with vars being saved between composes like I originally thought.

Of course, this doesn't fix the underlying (chrome?) bug that doesn't remove the focus ring from around the menu after the focus has moved. I wonder if that's a core issue. But at least this suggested patch should remove the immediate Tbird issue.

Cheers
Attachment #236222 - Flags: review?(mcow)
Comment on attachment 236222 [details] [diff] [review]
Stops the focus being set on the identity menu in onReopen()

Nice!

That patch does work, and I agree there no longer appears to be any good reason for those lines to stay in place.  

It appears you've already tracked this back to bug 141648, where the lines originally came in.  The problem described at that bug sounds like an issue with properly displaying the caret, not a problem with lost focus, but nobody ever said that explicitly.  I don't see that issue with this patch on TB 2a1; and as Jean-Francois said at that bug, the patch was a workaround.  I imagine that the caret-display has been fixed in the interim.

I don't know that I'm allowed to give r+, so I'll pass this over to Neil.  Seamonkey probably wants this fix.
Attachment #236222 - Flags: review?(mcow) → review?(neil)
(In reply to comment #17)
> It appears you've already tracked this back to bug 141648, where the lines
> originally came in.
> and as Jean-Francois said at that bug, the patch was a workaround.  I imagine
> that the caret-display has been fixed in the interim.

Funny, I'd actually forgotten about that bug. But, yeah, it looks like that code has been redundant for some time... The existence of bug 282669 suggests that it was no longer working anyway, and the fix for bug 282669 would have removed any remaining risks to reversing it again.

> I don't know that I'm allowed to give r+, so I'll pass this over to Neil. 
> Seamonkey probably wants this fix.

Thanks :)


(In reply to comment #17)
>I imagine that the caret-display has been fixed in the interim.
Not on the machine I tried this on - when I clicked on mailto:user@domain.invalid?subject=test then closed the compose window (with the cursor flashing in the empty body area) and clicked on the link again then I didn't get a flashing cursor in the body area the second time.
(In reply to comment #19)
> (In reply to comment #17)
> >I imagine that the caret-display has been fixed in the interim.
> Not on the machine I tried this on - when I clicked on
> mailto:user@domain.invalid?subject=test then closed the compose window (with
> the cursor flashing in the empty body area) and clicked on the link again
> then I didn't get a flashing cursor in the body area the second time.

Version?  HTML compose window, right?

With TB 2a1-0828, both as downloaded and as patched with Wayne's code change here, and clicking that URL twice as described, I see the flashing caret in the body with that test, both in HTML and plain.  (Note that bug 141648 was complaining about the caret not appearing in the first address field.)

When I created the test cases for this bug, it didn't occur to me to use the various mailto: forms.  Since this bug only occurs when the caret is supposed to be in the address field, I tried not only the mailto URL Neil provided, but also these:
  mailto:?subject=test
  mailto:?subject=test&body=foo
  mailto:?body=foo
Each of these exhibits the bug (for an HTML compose window opened from cache); and with Wayne's patch in place, the bug doesn't occur.
Neil, did you want to respond to comment 20, or either grant or deny review?
My issue is that caret not displaying correctly when the message has a subject.
Oddly the problem only occurs if I didn't edit the previous message.
I think I have come up with a satisfactory solution, but it would be helpful if other people could try it out too of course. In addition to the existing patch,
a) add window.content.focus() at the start of MsgComposeCloseWindow and 
b) remove the disableEditableFields() from gComposeRecyclingListener onClose()
(In reply to comment #22)
> My issue is that caret not displaying correctly when the message has a subject.
> Oddly the problem only occurs if I didn't edit the previous message.

Neil, I'm having trouble reproducing this. By "when the message has a subject" do you mean it happens when using reply and forward? (when the previous message was sent unedited)

Are you using Thunderbird or SeaMonkey? (I've only tested on Tbird)

Does it depend on where you left the focus before sending the previous message?
(In reply to comment #23)
>Neil, I'm having trouble reproducing this. By "when the message has a subject"
>do you mean it happens when using reply and forward? (when the previous message
>was sent unedited)
I was using mailto: links (which don't link properly in Bugzilla)

>Are you using Thunderbird or SeaMonkey? (I've only tested on Tbird)
SeaMonkey

>Does it depend on where you left the focus before sending the previous message?
I didn't send the previous message, but yes, I left the focus in the body.
OK, I tried it with a mailto: link that set a subject (and with or without a default message body). After each compose window opened, the focus was visibly on the message body. I closed each window (without editing the message body) before launching the next. No focus problems.

Maybe it's a SeaMonkey-only bug.

I'll try building a SeaMonkey build with the patch asap. Are you seeing the bug on the Moz 1.8 branch or the trunk?
(In reply to comment #25)
>Are you seeing the bug on the Moz 1.8 branch or the trunk?
I haven't tried the 1.8 branch.
I can reproduce it on the Moz 1.8 branch of SeaMonkey. Looks like it's SM-specific. Something else must be different in the focus handling of TB and SM. Is that what your fix in comment 22 addresses?
Comment on attachment 236222 [details] [diff] [review]
Stops the focus being set on the identity menu in onReopen()

In that case I can't review this as it doesn't work on SeaMonkey.
Attachment #236222 - Flags: review?(neil)
Comment on attachment 236222 [details] [diff] [review]
Stops the focus being set on the identity menu in onReopen()

OK, switching reviewer request.

Note that Seamonkey doesn't have a patch equivalent to that in bug 236219.

(In reply to comment #24)
> (In reply to comment #23)
> I was using mailto: links (which don't link properly in Bugzilla)

No, they don't, but you could copy a sample problem mailto into the URL field on the bug.
Attachment #236222 - Flags: review?(bienvenu)
(In reply to comment #29)
> Note that Seamonkey doesn't have a patch equivalent to that in bug 236219.

I'm in the middle of a system upgrade and I don't have a working 'diff' command at the moment, but: porting the changes from the patch at 236219, along with Wayne's patch here, the problem of the caret not blinking in the message body doesn't occur.  (There are two changes in the 236219 patch which were TB-specific; also note that part of that patch addresses a second bug, as described there.)
I'm confused - the patch is for TB, right? But I'm not seeing the problem in TB, at least on the trunk...should I be trying the 1.8 branch?
(In reply to comment #31)
> I'm confused - the patch is for TB, right? But I'm not seeing the problem in
> TB, at least on the trunk...should I be trying the 1.8 branch?

Symptoms described in comment 2 are still occurring in Thunderbird on trunk and branch.  I guess I don't have a proper set of steps to repro:

1) Start program fresh, or open a plain-text compose window and close it.
2) Open an HTML compose window; observe From: field's appearance; close window.
3) Open another HTML compose window; observe From: field's appearance

With (3), opening with New or Forward, or a mailto: that doesn't specify a recipient, the From: identity picker looks as if it has focus when it doesn't.


From Neil's comment 19 on, we were trying to figure out why Neil was seeing a slightly different response when using the patch to fix this bug under Seamonkey.  Seamonkey doesn't have a version of the patch from bug 236219; if that gets ported over, along with the patch from this bug, then it too appears to be fixed, at least for the test cases I've looked at.
Comment on attachment 236222 [details] [diff] [review]
Stops the focus being set on the identity menu in onReopen()

ok, I see it now. And the patch seems to fix it.
Attachment #236222 - Flags: review?(bienvenu) → review+
Attachment #236222 - Flags: approval-thunderbird2?
Thanks David! Could we please get this checked in to trunk and 1.8? :)

Neil: Sorry for your trouble. I hadn't tested this patch on SeaMonkey as I hadn't created it with the suite in mind. Hopefully Mike's extended patch will suit.

Cheers
patch landed on the trunk - I'll land it on the branch in a day or two...Thx for working on this, Wayne and Mike!
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
(In reply to comment #35)
> patch landed on the trunk - I'll land it on the branch in a day or two...Thx
> for working on this, Wayne and Mike!

Has it baked long enough to land on the branch now? :)


yes, thx for the reminder and the patch, Wayne. Fixed on 1.8.1 branch.
Keywords: fixed1.8.1
V with TB 2b1-1004 and 3a1-1004.  Thank you, Wayne, and David.
Status: RESOLVED → VERIFIED
(In reply to comment #38)
> V with TB 2b1-1004 and 3a1-1004.  Thank you, Wayne, and David.

Cool :) Mike, were you gonna make a SeaMonkey patch, based on your patch and this one?


Comment on attachment 236222 [details] [diff] [review]
Stops the focus being set on the identity menu in onReopen()

this is already fixed on the 1.8.1 branch.
Attachment #236222 - Flags: approval-thunderbird2?
Blocks: 364768
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: