Closed
Bug 532422
Opened 15 years ago
Closed 15 years ago
n900: Cannot select any characters from sym map
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Tracking
()
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | final-fixed |
fennec | 1.0+ | --- |
People
(Reporter: aakashd, Assigned: masayuki)
References
Details
Attachments
(3 files, 5 obsolete files)
(deleted),
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
Build Id:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091202 Firefox/3.6b5pre Fennec/1.0b6pre
Steps to Reproduce:
1. Click on the urlbar.
2. Press the blue arrow key + "sym"
3. Select any character from the sym map and press enter
Actual Results:
The selected character isn't added to the urlbar
Expected Results:
The selected character should be added to the urlbar
Flags: in-litmus?
Comment 1•15 years ago
|
||
In talking with Stuart, current theory is that this is a focus problem -- the symbol dialog sends its keystroke when focus is no longer on the URL bar or page field.
Updated•15 years ago
|
tracking-fennec: ? → 1.0+
Comment 2•15 years ago
|
||
Here's the xev dump for selecting the '<' key from the symbol map. In theory, we should reenter the window correctly, but I think we lose focus somehow before the key is handled.
LeaveNotify event, serial 26, synthetic NO, window 0x3800001,
root 0x44, subw 0x0, time 6343404, (360,7), root:(360,63),
mode NotifyNormal, detail NotifyNonlinear, same_screen YES,
focus NO, state 0
EnterNotify event, serial 26, synthetic NO, window 0x3800001,
root 0x44, subw 0x0, time 6349619, (86,156), root:(86,212),
mode NotifyNormal, detail NotifyNonlinear, same_screen YES,
focus NO, state 0
KeymapNotify event, serial 26, synthetic NO, window 0x0,
keys: 68 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
Updated•15 years ago
|
Assignee: nobody → mark.finkle
Updated•15 years ago
|
Assignee: mark.finkle → combee
Status: NEW → ASSIGNED
Comment 3•15 years ago
|
||
Masayuki Nakano, do you have any advice for Ben in helping track down this problem?
Comment 4•15 years ago
|
||
I've tracked a bit further. I'm seeing that we get IMEComposeStart, IMEComposeEnd from the GTK layer for the symbol popup, but it doesn't turn into a character.
Here's my log output from bringing up the symbol dialog and hitting a key
IMELoseFocus 0x40f2ccd0
--- InputEvent: blur
--- InputEvent: blur
--- InputEvent: blur
IMEComposeStart [0x40f2ccd0]
WARNING: mReferenceWidget is null: file /home/bcombee/src/mozilla-central/widget/src/gtk2/nsWindow.cpp, line 6785
IMEComposeText
WARNING: mReferenceWidget is null: file /home/bcombee/src/mozilla-central/widget/src/gtk2/nsWindow.cpp, line 6785
IMEComposeEnd [0x40f2ccd0]
IMESetFocus 0x40f2ccd0
--- InputEvent: focus
--- InputEvent: focus
--- InputEvent: focus
I'm worried about those warnings... it seems like the IME is trying to set the cursor in the input widget, but that's not available.
Assignee | ||
Comment 5•15 years ago
|
||
(In reply to comment #4)
> I'm worried about those warnings... it seems like the IME is trying to set the
> cursor in the input widget, but that's not available.
At that time, IMESetCursorPosition uses 'this' for the reference widget.
Probably, the focus was already lost. If so, we could fix by to change the NS_TargetUnfocusedEventToLastFocusedContent in nsGUIEvent.h.
However, there are bug 519913 and bug 497839, so, our event handling is completely broken when all windows are deactive.
Updated•15 years ago
|
Comment 6•15 years ago
|
||
Patch in bug 519913 doesn't apply cleanly on 1.9.2, and even with edits to allow the diff to work, it relies on behavior that's only in moz-central.
Assignee | ||
Comment 7•15 years ago
|
||
I'm not sure this can build and has no bugs.
Comment 8•15 years ago
|
||
This fixes the problem
Assignee: combee → nobody
Component: Linux/Maemo → Event Handling
Product: Fennec → Core
QA Contact: maemo-linux → events
Comment 9•15 years ago
|
||
Masayuki: how safe is this to land on 1.9.2? Should we ifdef it for mobile only? Using the hildon ifdefs for now?
Flags: blocking1.9.2?
Updated•15 years ago
|
Assignee: nobody → masayuki
Assignee | ||
Comment 10•15 years ago
|
||
Currently, we don't handle the events which NS_TargetUnfocusedEventToLastFocusedContent() returns TRUE by when all windows are deactive. And the events are retargetted to the focused element if the window is deactive.
My patch sends the events to the last focused content on the window.
So, my patch changes the event dispatching model drastically. The patch should be reviewed by smaug. And I recommend that it should be only enabled on the Hildon for minimizing the risk.
Assignee | ||
Comment 11•15 years ago
|
||
This patch only for the 1.9.2 branch (only changes the behavior on Hildon), we should fix the bug on m-c later.
This bug is critical for mobile. So, this patch is pretty risky but we need to fix this bug.
My patch retargets all key/IME related events to the (last) focused DOM window/content in the top level window.
Attachment #417877 -
Attachment is obsolete: true
Attachment #417965 -
Flags: review?(Olli.Pettay)
Comment 12•15 years ago
|
||
(In reply to comment #11)
> This bug is critical for mobile. So, this patch is pretty risky but we need to
> fix this bug.
I wonder whether we need something similar for FF3.6.
Anyway, reviewing...
Comment 13•15 years ago
|
||
Comment on attachment 417965 [details] [diff] [review]
Patch v1.0
>+#ifdef NS_FIX_BUG532422
>+ // This returns the focused window and the focused element under our top
>+ // level window. I.e., when we are deactive, this returns the *last* focused
>+ // element and window.
>+ nsresult GetFocusedContent(nsIContent** aFocusedContent,
>+ nsPIDOMWindow** aFocusedWindow);
>+#endif
This returns always the last focused element and window under the top window of
mDocument. Should this actually return the focused/active window if there is such?
That way we'd change the behavior only if there wasn't any active/focused window.
It should be less regression risky. This one looks pretty scary.
Comment 14•15 years ago
|
||
Comment on attachment 417965 [details] [diff] [review]
Patch v1.0
Neil should look at this too.
Attachment #417965 -
Flags: review?(enndeakin)
Comment 15•15 years ago
|
||
Is the bug here that:
1. a key event needs to fire when a window is deactive, or
2. a key event needs to fire at a child window that isn't focused but may be active, or
3. an ime event needs to be handled in either if those cases
Point 1 is handled by this patch but the patch also retargeted a lot of other cases that probably shouldn't be.
Point 2 could be fixed by just disabling the if (!sDontRetargetEvents ...) line.
I don't know about 3.
4. Or is the fix actually just to change the NS_TargetUnfocusedEventToLastFocusedContent and NS_IsEventTargetedAtFocusedWindow macros to handle deactive windows differently or perhaps correctly if there is a bug in them. That is what those macros were added for, no?
As an aside, calling the static nsFocusManager::GetFocusedDescendant would make the code in the patch much simpler.
Comment 16•15 years ago
|
||
Masayuki, what is the difference between the patch in this bug and the patch
for Bug 519913?
Bug 519913 looks almost like something which should block 1.9.2.
Comment 17•15 years ago
|
||
Smaug: I believe this is just the 1.9.2 version of that patch.
Comment 18•15 years ago
|
||
Blocking 1.9.2, though we need to make a decision about whether or not we want it to be IFDEF'd for Fennec only.
Flags: blocking1.9.2? → blocking1.9.2+
Assignee | ||
Comment 19•15 years ago
|
||
Bug 519913 is for trunk and I fixed some bugs of the patch in this bug's patch.
If we need to fix bug 519913, we need to retarget in the deactive window rather than to the active window because Mac needs such behavior on IME committing.
If we only fix this bug (not fix bug 519913), it may be good thing that we retarget only when there are no active window.
(In reply to comment #15)
> 1. a key event needs to fire when a window is deactive, or
> 2. a key event needs to fire at a child window that isn't focused but may be
> active, or
> 3. an ime event needs to be handled in either if those cases
>
> Point 1 is handled by this patch but the patch also retargeted a lot of other
> cases that probably shouldn't be.
> Point 2 could be fixed by just disabling the if (!sDontRetargetEvents ...)
> line.
> I don't know about 3.
1 and 3 are needed.
> 4. Or is the fix actually just to change the
> NS_TargetUnfocusedEventToLastFocusedContent and
> NS_IsEventTargetedAtFocusedWindow macros to handle deactive windows differently
> or perhaps correctly if there is a bug in them. That is what those macros were
> added for, no?
No, because the key/IME events are discarded in the
|if (!sDontRetargetEvents && NS_IsEventTargetedAtFocusedWindow(aEvent))| block.
If there are no focused window, it does nothing.
Assignee | ||
Comment 20•15 years ago
|
||
Attachment #417965 -
Attachment is obsolete: true
Attachment #417965 -
Flags: review?(enndeakin)
Attachment #417965 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 21•15 years ago
|
||
Attachment #418135 -
Attachment is obsolete: true
Comment 22•15 years ago
|
||
I tried https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-try-4686755845eb/maemo/ on my N900 and sym map did seems to work.
I think that build is with patch 2.0.
Assignee | ||
Comment 23•15 years ago
|
||
Assignee | ||
Comment 24•15 years ago
|
||
Comment on attachment 418139 [details] [diff] [review]
Patch v2.1
O.K. So, this fixes:
1. retargetting to the last focused content when the all windows are deactive.
2. The IME composition is committed when a Fx window lost focus (on Windows (and perhaps on Linux too)).
However, this cannot fix the 2nd bug on Mac because the commit events are fired after completely deactivated the window.
Attachment #418139 -
Flags: review?(Olli.Pettay)
Comment 25•15 years ago
|
||
Comment on attachment 418152 [details] [diff] [review]
Patch v2.1 for trunk
>+nsPIDOMWindow*
>+PresShell::GetFocusedDOMWindowInOurWindow()
>+{
>+ nsCOMPtr<nsPIDOMWindow> window =
>+ do_QueryInterface(mDocument->GetWindow());
>+ NS_ENSURE_TRUE(window, nsnull);
>+
>+ nsCOMPtr<nsPIDOMWindow> rootWindow = window->GetPrivateRoot();
>+ NS_ENSURE_TRUE(rootWindow, nsnull);
>+ nsPIDOMWindow* focusedWindow;
>+ nsCOMPtr<nsIContent> content =
>+ nsFocusManager::GetFocusedDescendant(rootWindow, PR_TRUE, &focusedWindow);
>+ return focusedWindow;
>+}
This leaks.
Make this method to return already_AddRefed<nsPIDOMWindow>
and change nsCOMPtr<nsIContent> content to
nsIContent* content
with that, r=me
I can't really think of anything safer for 1.9.2.
Neil should review this too.
Attachment #418152 -
Flags: review+
Comment 26•15 years ago
|
||
Comment on attachment 418139 [details] [diff] [review]
Patch v2.1
Same thing with this patch.
Attachment #418139 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 27•15 years ago
|
||
Thank you, Olli.
Enn: See bug 519913 comment 14.
We should change only when all windows are deactive for the safe.
Attachment #418139 -
Attachment is obsolete: true
Attachment #418173 -
Flags: review?(enndeakin)
Assignee | ||
Comment 28•15 years ago
|
||
Attachment #418152 -
Attachment is obsolete: true
Attachment #418175 -
Flags: review?(enndeakin)
Comment 29•15 years ago
|
||
> No, because the key/IME events are discarded in the
> |if (!sDontRetargetEvents && NS_IsEventTargetedAtFocusedWindow(aEvent))| block.
> If there are no focused window, it does nothing.
So what are these macros for then?
Comment 30•15 years ago
|
||
Comment on attachment 418173 [details] [diff] [review]
Patch v2.2
Seems OK for now, but it would be good to test this a bit more. Especially with regards to things like pressing keys/using ime before and after minimizing windows and opening dialogs and so forth.
Attachment #418173 -
Flags: review?(enndeakin) → review+
Updated•15 years ago
|
Attachment #418175 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 31•15 years ago
|
||
(In reply to comment #29)
> > No, because the key/IME events are discarded in the
> > |if (!sDontRetargetEvents && NS_IsEventTargetedAtFocusedWindow(aEvent))| block.
> > If there are no focused window, it does nothing.
>
> So what are these macros for then?
Before the focus refactoring, the events are not discarded.
(In reply to comment #30)
> (From update of attachment 418173 [details] [diff] [review])
> Seems OK for now, but it would be good to test this a bit more. Especially with
> regards to things like pressing keys/using ime before and after minimizing
> windows and opening dialogs and so forth.
I'll do it tomorrow before I land them.
Assignee | ||
Comment 32•15 years ago
|
||
It's 3:00 AM JST, so, I cannot watch tinderbox even if I land them now. If the mobile team cannot wait my check-in, please land them yourself.
Comment 33•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/8ad4bf85501b
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/216cb2db5f62
Thanks Masayuki for the awesome help and Neil for the quick reviews. Fennec loves you.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
status1.9.2:
--- → final-fixed
Resolution: --- → FIXED
Assignee | ||
Comment 34•15 years ago
|
||
(In reply to comment #31)
> (In reply to comment #30)
> > (From update of attachment 418173 [details] [diff] [review] [details])
> > Seems OK for now, but it would be good to test this a bit more. Especially with
> > regards to things like pressing keys/using ime before and after minimizing
> > windows and opening dialogs and so forth.
>
> I'll do it tomorrow before I land them.
I tested easily, but I didn't find any problems, thank you.
And thank you, dougt.
Reporter | ||
Comment 35•15 years ago
|
||
verified FIXED on build:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091221 Firefox/3.6b5pre Fennec/1.0b6pre
Status: RESOLVED → VERIFIED
Comment 36•15 years ago
|
||
Comment on attachment 418175 [details] [diff] [review]
Patch v2.2 for trunk
>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>+ nsIContent* content =
>+ nsFocusManager::GetFocusedDescendant(rootWindow, PR_TRUE, &focusedWindow);
>+ return focusedWindow;
>+}
This change introduced the following build warning on mozilla-central:
>../../../mozilla/layout/base/nsPresShell.cpp: In member function ‘already_AddRefed<nsPIDOMWindow> PresShell::GetFocusedDOMWindowInOurWindow()’:
>../../../mozilla/layout/base/nsPresShell.cpp:5962: warning: unused variable ‘content’
If |content| is intentionally unused, perhaps the first quoted line above ("nsIContent* content =") should just be removed? There's no point in storing the return value in a local variable, if you're not going to use it.
Or, is |content| actually supposed to be used? (null-checked as a measure of success, or anything like that?)
Assignee | ||
Comment 37•15 years ago
|
||
Ah, just we can remove it. I'll post the followup patch.
Assignee | ||
Comment 38•15 years ago
|
||
Attachment #420277 -
Flags: review?(Olli.Pettay)
Updated•15 years ago
|
Attachment #420277 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs landing attachment 420277]
Assignee | ||
Comment 39•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b919e8a48890
landed the additional patch only on trunk.
Whiteboard: [needs landing attachment 420277]
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•