83496 - Need mechanism that allows us to figure out how focus was gained

Status: RESOLVED FIXED

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

Description

[kinmoz]

This is an offshoot of bug #28583.

Right now there is no way for text widgets to tell if focus was gained as the 
result of the user tabbing between form widgets, or by any other means.

We need a mechanism in the event system that allows the textwidget to tell 
how/why it got focus. This is neccessary because on some platforms, tabbing into 
a textwidget is supposed to select all it's contents, and any other method of 
focus gain leaves the selection/caret in the textwidget where it was before.

Comment 1

[Madhur Bhatia]

QA contact updated

Comment 2

[Blake Ross]

Doesn't look like this is getting fixed before the freeze tomorrow night.
Pushing out a milestone.  Please correct if I'm mistaken.

Comment 3

[saari (gone)]

->0.9.5

Comment 4

[joki (gone)]

Moving to 0.9.6

Comment 5

[Aaron Leventhal]

CC'ing bryner, who is working on similar things.

Comment 6

[joki (gone)]

0.9.6 has passed, moving to 0.9.7.  Load balancing 0.9.7 list shortly

Comment 7

[joki (gone)]

Moving bugs from 0.9.7 for triaging in 0.9.8

Comment 8

[Kevin McCluskey (gone)]

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

Comment 9

[Kevin McCluskey (gone)]

Nominating for nsbeta1, since it is blocking bug 28583 which is nsbeta1+.

Comment 10

[Aaron Leventhal]

I'm going to fix this the right way, the way the title of the bug suggests.

I need the "how focused was gained" mechanism to fix bug 131139 as well.

Comment 11

[Aaron Leventhal]

Does anyone see the need for more than a PRBool aFocusedWithMouse?

Comment 12

[Aaron Leventhal]

-> Pete Zha.

We discussed the fix on IRC>

Comment 13

[Aaron Leventhal]

Pete, I added dependencies for all the bugs this can easily help fix.
The fix for bug 132099 and 131139 is the same. In
nsEventStateManager::ChangeFocus(), we need to not call MoveCaretToFocus() if
focus was gained via the mouse.

One extra detail: if we use this method to suppress MoveCaretToFocus(), we need
to make sure we move from the focus location instead of the selection the next
time we call nsEventStateManager::ShiftFocus(). This is because the focus has
changed, but the selection could still be in a completely different place. An
exception to this is if the focus is on the document -- in that case we always
want to move from the selection location if we have a selection.

This probably sounds really hard - it's not. If you post a patch I'll help you
with these extra tweaks.

Comment 14

[timeless]

focus gained by: Mouse, Keyboard, Application?

Is there some reason not to use an integer/enum?

Comment 15

[Aaron Leventhal]

We need to decide whether to add a parameter to nsIContent::SetFocus() or add an
additional method like nsIContent::SetFocusFrom()

Comment 16

[Pete Zha]

I think maybe we don't need to add a new interface or change the old one...
We can add something to mPresContext to pass the focusWith flag.
Like add Get/SetFocusedWith function into nsIPresContext.h and add mFocusedWith 
to nsPresContext.h.
And when we ChangeFocus, we can pass the flag to nsPresContext first.
I will upload a patch for this solution later.

Comment 17

[Pete Zha]

Attachment: Use nsIPresContext to carry focused with flag

This is a workarround patch.
Just work for me. With this patch, I can know where the focus from. Like
Keyboard, Click or other application.

Comment 18

[Aaron Leventhal]

I recommend posting one patch that shows the fixes for bug 28583, bug 131139,
bug 132099 and this one, so we can see it and review it all together.

Also, in nsPresContext.cpp, why are you adding 
+NS_IMETHODIMP
+nsPresContext::GetUseFocusColors(PRBool& aUseFocusColors)

instead of ::SetFocusedWith()

Comment 19

[Aaron Leventhal]

More comments:

Let's use an enum, not an int. It doesn't make sense to go 1,2,4,8 unless the
focus can be gained several different ways at the same time. Therefore, use
0,1,2,3...

aFocusedWithMouse should be aFocusedWith, since it's not a bool anymore that
just relates to where it was focused with the mouse. It's holding the enum.

I spoke with Bryner. We feel it's not necessary to bloat pres context with this.
If anything, this should be a global/static value, since there's only 1 global
focus at a time. However, we feel it might be better to insert this information
in the event or change ::SetFocus(presContext) to add a how param.

CC'ing Chris Saari and Tom Pixley for some fresh perspective.   

Comment 20

[saari (gone)]

It looks okay to fix 131139 and 132099 using the ChangeFocus modification, but
doing a more general solution is something I haven't wrapped my head around yet,
at least not a good way to fix it. The problem becomes nasty when you consider
that the event system finds out about focus events often from OS native events
which may or may not have come from our system (mouse down handling or tab or
script). So we get in this spiral of tracking event origination in a global
way... which is the exact sort of thing I'm trying to do less of in the focus code.

We're trying to not change any pulbic APIs before mozilla 1.0, so we'd like to
revisit this after mozilla 1.0 branches.

Comment 21

[Pete Zha]

Comment on attachment 75559 [details] [diff] [review]
Use nsIPresContext to carry focused with flag

This patch is built on a broken source tree. Ignore it.

Comment 22

[Pete Zha]

The script calls element's focus function by asm code. Not from 
eventStateManager. 
Maybe we can modify codes in nsGenericHTMLElement::SetElementFocus(PRBool 
aDoFocus) which called by script and will call content->SetFocus(presContext). 

Comment 23

[Pete Zha]

aaronl:Is this bug still useful, or we can close it?

Comment 24

[Aaron Leventhal]

-> Reassign to Blizzard as this is part of the focus refactoring in bug 178324.

Note, we don't need this anymore to fix bug 28583.

Comment 25

[Aaron Leventhal]

Removing as direct dependency of bug 83552 since it is dependent on focus
refactoring bug. We don't need this for anything in particular but it would be
good to do if the focus logic was ever refactored.

Comment 26

[Neil Deakin]

nsIFocusManager::GetLastFocusMethod can be used now.