214306 - No cursor (focus) present in the 'add email account' wizard

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Bruce Heerssen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6

When entering text into form fields in the new accounts wizard, the cursor does
not appear in the form fields. Although you can enter text correctly, lack of a
cursor makes it difficult to do so.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Bruce Heerssen]

I am running Mandrake Linux 9.1 on a 2.4.x kernel. I don't know if other distros
or kernels are affected.

Comment 2

[Stephen Walker]

*** Bug 214423 has been marked as a duplicate of this bug. ***

Comment 3

[Matt Williams]

confirming -- kernel 2.0.24 on redhat 9.

Comment 4

[Malte Rücker (mbr)]

*** Bug 216985 has been marked as a duplicate of this bug. ***

Comment 5

[Andrew]

confirming.  Thunderbird .1 and Mail/News in Mozilla 1.5b on Windows 2000

Comment 6

[Jason]

0.2 version (thunderbird)  also produces this behavior under Windows XP

Comment 7

[Brian Ryner (not reading)]

Fun with focus.  Changing bug to all platforms based on last comment and the
fact that I see this on Win2k.  Interesting note is that switching windows and
then switching back makes the caret appear, so this is probably a case of the
editor not receiving the focus event initially for some reason.  (the caret is
turned on when editor receives the event; the fact that you can type indicates
that the EventStateManager's focus state is correct)

Comment 8

[Stefan Borggraefe]

I see this bug in Seamonkey on Windows XP. So this is not Thunderbird-specific.

Comment 9

[Kjeld Borch Egevang]

This bug is still present in Mozilla 5.0 RC 2. I'm running Win2k. Please fix
this bug. When you present someone new to Mozilla this is one of the first
dialogues they see...

Comment 10

[Malte Rücker (mbr)]

*** Bug 224965 has been marked as a duplicate of this bug. ***

Comment 11

[Stephen Walker]

*** Bug 231216 has been marked as a duplicate of this bug. ***

Comment 12

[Stefan Borggraefe]

Adding focus to summary so I'll find this bug faster the next time I'm looking
for it. ;-)

I tried to track down this bug (with Seamonkey) and found the variable focused
is always null here:

http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/wizard.xml#304

Perhaps Neil has an idea how to fix this.

Comment 13

[Stefan Borggraefe]

I discussed with Neil about this bug on IRC. Here is what he found out:

1. A Workaround would be to add aPage.focus(); before
     document.commandDispatcher.advanceFocusIntoSubtree(aPage);
  here:
http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/wizard.xml#300


2. So the cause of this bug is somewhere down the call-chain of   
   AdvanceFocusIntoSubtree(subtreeRoot):

http://lxr.mozilla.org/mozilla/source/content/xul/document/src/nsXULCommandDispatcher.cpp#200

-> Browser/XP Toolkit/Widgets

Comment 14

[[e-mail]]

So, what's happening here, is that ShiftFocusInternal has found something to
focus and tries to ChangeFocus to it. That eventually calls through to
SendFocusBlur, however for whatever reason that resets the focus to
gLastFocusedContent. This confuses ChangeFocus into thinking that an event
listener reset the focus.

Note that calling aPage.focus() works around the issue by setting
gLastFocusedContent to the element that we're trying to shift focus from.

So, bryner, what's the fix? I suspect we might need to initialize prevFocus to
gLastFocusedContent instead of mCurrentFocus which is actually the element we're
trying to advance focus from.

Comment 15

[[e-mail]]

Attachment: First go at a patch

Comment 16

[Scott MacGregor]

bryner, I'd be curious about your thoughts on this bug and the potential risk.
This is actually a high visibility issue with the acount wizard and it would be
a really nice fix to have for 1.7 if the risk level and correctness are there

Comment 17

[Brian Ryner (not reading)]

This is an unfortunate side-effect of redundant focus state.  Basically, the
following should always hold true:

gLastFocusedContent == <active ESM>->mCurrentFocus

In this case, I think the problem stems from what happens when
ShiftFocusInternal is passed a starting element, which comes from the call in
wizard.xml:

document.commandDispatcher.advanceFocusIntoSubtree(aPage);

(where aPage is the wizardpage element).

This basically means we want to act as if focus was on the wizardpage and the
user pressed tab.  However, in ShiftFocusInternal(), if given a node to start
from, we do this:

  if (aStart) {
    SetFocusedContent(aStart);
    TabIndexFrom(mCurrentFocus, &mCurrentTabIndex);
  }

SetFocusedContent only updates mCurrentFocus, which means gLastFocusedContent is
out of sync (it's still set to the Next button, the last focused element).  I
would say we probably want to update gLastFocusedContent at this point too (if
gLastFocusedDocument == mDocument) rather than intentionally leaving
mCurrentFocus and gLastFocusedContnet out of sync.

I'm not entirely sure why SendFocusBlur resets the ESM's mCurrentFocus to
gLastFocusedContent, either... given the above "invariant" it should be
redundant, since gLastFocusedContent's ESM must be the active ESM.

Comment 18

[[e-mail]]

Well as long as you still fire the blur event on the element that's really
losing the focus...

Comment 19

[Brian Ryner (not reading)]

(In reply to comment #18)
> Well as long as you still fire the blur event on the element that's really
> losing the focus...

Yeah, I guess that's a problem.  Maybe the real solution is to make
ShiftFocusInternal not mess with the current focus just because of aStart, i.e.
make GetNextTabbableContent depend entirely on a focused element that's passed
in and not on mCurrentFocus.

Comment 20

[Brian Ryner (not reading)]

Attachment: fix ShiftFocusInternal when aStart != null

This patch makes it so finding the next tabbable content doesn't require
mCurrentFocus to be set to the content you're tabbing from, which eliminates
the entire problem.  I tested that it fixes the wizard bug, and ran some basic
tests for handling of tabindex, tabbing into and out of documents and iframes,
and tabbing through XUL dialogs.

Comment 21

[[e-mail]]

Comment on attachment 145768 [details] [diff] [review]
fix ShiftFocusInternal when aStart != null

>+  nsIContent *startContent = nsnull;
> 
>-  if (selectionFrame) 
>+  if (aStart) {
>+    startContent = aStart;
>+    presShell->GetPrimaryFrameFor(startContent, &curFocusFrame);
>+    if (!curFocusFrame) {
>+      // We were asked to navigate from this content, but the given content
>+      // has no frame.  Fall back to navigating from the document root.
>+      startContent = nsnull;
>+    }
Nit: reversing the sense of the test might neaten this up (but don't copy the
code I pasted on IRC, I mixed up my variable names).

>+  } else  if (!mCurrentFocus) {  // Get tabindex ready
Nit: double space between else and if.

Comment 22

[Stefan Borggraefe]

bryner: Did you forget to mark this fixed or do you want to do something more
with this bug?

Comment 23

[Scott MacGregor]

fixed on the tbird 0.6 branch

Comment 24

[Scott MacGregor]

*** Bug 241686 has been marked as a duplicate of this bug. ***

Comment 25

[Stefan Borggraefe]

*** Bug 213850 has been marked as a duplicate of this bug. ***

Comment 26

[Stefan Borggraefe]

*** Bug 216627 has been marked as a duplicate of this bug. ***

Comment 27

[Stefan Borggraefe]

I assume Brian just forgot to mark this bug fixed. Doing so now. The fix was
checked in half a year ago.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145768 [details] [diff] [review]
fix ShiftFocusInternal when aStart != null

Looks like mscott landed this on the aviary branch back in May (nice checkin
comment with no mention of the bug number, too), which really helps with those
merge conflicts.

We should probably either land this on 1.7 branch or back it out of aviary.  I
assume mscott is an aviary driver (though there seems to be no way to verify
this), so no need to request post-facto aviary approval...

Comment 29

[Scott MacGregor]

(In reply to comment #28)
> (From update of attachment 145768 [details] [diff] [review])
> Looks like mscott landed this on the aviary branch back in May (nice checkin
> comment with no mention of the bug number, too), which really helps with those
> merge conflicts.

If you had read the comment you would have seen that this came from a merging of
the tbird 0.6 branch onto the aviary 1.0 branch after I created the aviary 1.0
branch. When I do large scale branch landings I don't write down nor track down
every bug number being merged into a checkin comment. My compliments to you if
you actually have the time to do all of that when merging branches. 

Back in May we weren't even doing aviary branch approvals so your comment about
backing it out because I didn't approve my branch landing is not very helpful.
Are we going to retroactively approve every change that went into a branch
before we started going through an approval process for said branch? Wow, that
would realy suck! :(

Comment 30

[Boris Zbarsky [:bzbarsky]]

> so your comment about backing it out because I didn't approve my branch landing

The comment was because we're keeping Gecko on 1.7 and Aviary in sync, not
because you didn't get approval.  I thought I pretty clearly stated that I was
assuming you could do a=mscott as needed.  I said that precisely to avoid this
misunderstanding.

I appreciate your difficulty with merges, but differences between the branches
make landing changes on both (per current foundation policy) much harder, and
putting bug numbers in makes for easier tracking down of changes as needed.

Comment 31

[Boris Zbarsky [:bzbarsky]]

To clarify further, the part about backing out is only if the branch drivers and
the patch author decide that this patch is not acceptable for the 1.7 branch (f
or risk reasons, etc).

Comment 32

[Brendan Eich [:brendan]]

> make landing changes on both (per current foundation policy) much harder, and

"foundation"?  That policy was from [e-mail], specifically roc.

Are the changes here, long baked on aviary, really that worrisome, or is this
more of a process question?

/be

Comment 33

[Boris Zbarsky [:bzbarsky]]

> "foundation"?  That policy was from [e-mail], specifically roc.

Ah, ok.  My apologies.

> Are the changes here, long baked on aviary, really that worrisome

No clue.  I rather assume they're not and that this will land on 1.7 branch,
which is why I felt the need to clarify that part later...  But I'm in no
position to evaluate these changes; I assume bryner is.

Comment 34

[Mike Kaply [:mkaply]]

So has this changed baked on aviary and trunk and not caused any problems?

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 145768 [details] [diff] [review]
fix ShiftFocusInternal when aStart != null

1.7.5 has shipped. Moving request to 1.7.6.

Comment 36

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 145768 [details] [diff] [review]
fix ShiftFocusInternal when aStart != null

Too late for 1.7.6.  Also a friendly reminder that something like this would
need to go on both branches if it is to go in at all.