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.
QA contact updated
QA Contact: gerardok → madhur
Doesn't look like this is getting fixed before the freeze tomorrow night. Pushing out a milestone. Please correct if I'm mistaken.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Moving to 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
CC'ing bryner, who is working on similar things.
0.9.6 has passed, moving to 0.9.7. Load balancing 0.9.7 list shortly
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Moving bugs from 0.9.7 for triaging in 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
*** Bug 127272 has been marked as a duplicate of this bug. ***
Nominating for nsbeta1, since it is blocking bug 28583 which is nsbeta1+.
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.
Assignee: joki → aaronl
Does anyone see the need for more than a PRBool aFocusedWithMouse?
Status: NEW → ASSIGNED
Target Milestone: mozilla1.0.1 → mozilla1.0
-> Pete Zha. We discussed the fix on IRC>
Assignee: aaronl → pete.zha
Status: ASSIGNED → NEW
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.
focus gained by: Mouse, Keyboard, Application? Is there some reason not to use an integer/enum?
We need to decide whether to add a parameter to nsIContent::SetFocus() or add an additional method like nsIContent::SetFocusFrom()
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.
Created attachment 75559 [details] [diff] [review] 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.
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()
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.
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 on attachment 75559 [details] [diff] [review] Use nsIPresContext to carry focused with flag This patch is built on a broken source tree. Ignore it.
Attachment #75559 - Attachment is obsolete: true
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).
aaronl:Is this bug still useful, or we can close it?
-> 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.
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.
No longer blocks: 83552
nsIFocusManager::GetLastFocusMethod can be used now.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.