Closed Bug 214306 Opened 17 years ago Closed 16 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bruce, Assigned: bryner)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
I am running Mandrake Linux 9.1 on a 2.4.x kernel. I don't know if other distros
or kernels are affected.
*** Bug 214423 has been marked as a duplicate of this bug. ***
confirming -- kernel 2.0.24 on redhat 9.
*** Bug 216985 has been marked as a duplicate of this bug. ***
confirming.  Thunderbird .1 and Mail/News in Mozilla 1.5b on Windows 2000
0.2 version (thunderbird)  also produces this behavior under Windows XP
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)
OS: Linux → All
Hardware: PC → All
QA Contact: asa
I see this bug in Seamonkey on Windows XP. So this is not Thunderbird-specific.
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...
*** Bug 224965 has been marked as a duplicate of this bug. ***
*** Bug 231216 has been marked as a duplicate of this bug. ***
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.
Summary: No cursor present in the 'add email account' wizard → No cursor (focus) present in the 'add email account' wizard
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
Assignee: mscott → jag
Component: Account Manager → XP Toolkit/Widgets
Product: Thunderbird → Browser
QA Contact: jrgmorrison
Version: unspecified → Trunk
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.
Assignee: jag → events
Component: XP Toolkit/Widgets → Event Handling
QA Contact: jrgmorrison → ian
Attached patch First go at a patch (obsolete) — Splinter Review
Attachment #145611 - Flags: review?(bryner)
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
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.
Well as long as you still fire the blur event on the element that's really
losing the focus...
(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.
Attachment #145611 - Flags: review?(bryner) → review-
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.
Assignee: events → bryner
Attachment #145611 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #145768 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #145768 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #145768 - Flags: superreview?(dbaron)
Attachment #145768 - Flags: superreview?(dbaron) → superreview+
Blocks: 240476
bryner: Did you forget to mark this fixed or do you want to do something more
with this bug?
fixed on the tbird 0.6 branch
*** Bug 241686 has been marked as a duplicate of this bug. ***
*** Bug 213850 has been marked as a duplicate of this bug. ***
*** Bug 216627 has been marked as a duplicate of this bug. ***
I assume Brian just forgot to mark this bug fixed. Doing so now. The fix was
checked in half a year ago.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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...
Attachment #145768 - Flags: approval1.7.x?
(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! :(
> 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.
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).
> make landing changes on both (per current foundation policy) much harder, and

"foundation"?  That policy was from drivers@mozilla.org, specifically roc.

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

/be
> "foundation"?  That policy was from drivers@mozilla.org, 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.
So has this changed baked on aviary and trunk and not caused any problems?
Comment on attachment 145768 [details] [diff] [review]
fix ShiftFocusInternal when aStart != null

1.7.5 has shipped. Moving request to 1.7.6.
Attachment #145768 - Flags: approval1.7.5? → approval1.7.6?
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.
Attachment #145768 - Flags: approval1.7.6? → approval1.7.6-
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.