Closed
Bug 454235
Opened 16 years ago
Closed 16 years ago
IME events not fired when clicking on text input box
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: blassey, Assigned: blassey)
References
Details
(Keywords: inputmethod, mobile)
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
When you click on a text box in fennec, not IME events are fired, which means the software keyboard does not pop up (with the latest patch). I believe this is because we're actually clicking on a canvas and not a text box.
Assignee | ||
Comment 1•16 years ago
|
||
we test if the prescontext of the content is active, which is normally fine. But because we're viewing the browser through a canvas, the prescontext of the browser isn't active and we fail this test every time. This patch simply cuts the test out. I assume we'd want to do this with a pref, so fennec can by pass the test, but other xul apps can have a "normal" xr. Alternatively, we can add a property to prescontext so fennec can mark it as pseudo-active. Smaug, can you comment?
Assignee: nobody → blassey
Assignee | ||
Comment 2•16 years ago
|
||
smaug, I know this isn't your module per se, but it is meant to fix an event related bug so I wanted to get your take on it. If its the right thing to do you in your mind, I'll as for sr from bs, bz or jst.
Attachment #337615 -
Attachment is obsolete: true
Attachment #338268 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 3•16 years ago
|
||
Attachment #338268 -
Attachment is obsolete: true
Attachment #338271 -
Flags: review?(Olli.Pettay)
Attachment #338268 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 4•16 years ago
|
||
Is this sufficient, or do we need to reset the flag on other events?
Attachment #338272 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #338272 -
Flags: review? → review?(gavin.sharp)
Comment 5•16 years ago
|
||
We'd need to switch it when selecting a new tab, right? Is |view->GetVisibility() == nsViewVisibility_kHide| true when the tab is in the background (i.e. hidden by the deck), or does that rely on the parent check too?
Assignee | ||
Comment 6•16 years ago
|
||
I'm not sure to be honest, but I'm clearing the bit when we switch tabs so it shouldn't matter. + currentBrowser.isOffScreenBrowser = false; I was more thinking that when we load or refresh a page we might get a new docshell.
Assignee | ||
Comment 7•16 years ago
|
||
I realized as I submitted that comment that that line should be "currentBrowser.docShell.isOffScreenBrowser = false;"
Comment 8•16 years ago
|
||
Comment on attachment 338271 [details] [diff] [review] adds isOffScreenBrowser to docshell, cleaned up a bit This all is something bz should look at. > // otherwise, we must walk up the document and view trees checking >- // for a hidden view. >+ // for a hidden view. unless, we're an off screen browser, which >+ // would make this test meaningless > > nsCOMPtr<nsIDocShellTreeItem> treeItem = this; > nsCOMPtr<nsIDocShellTreeItem> parentItem; > treeItem->GetParent(getter_AddRefs(parentItem)); >- while (parentItem) { >+ while (parentItem && !mIsOffScreenBrowser) { You should still handle the case where offscreen browser is under non-visible docshell. The method should return false in that case. >@@ -438,14 +438,21 @@ interface nsIDocShell : nsISupports You should update IID when changing an interface. Do we want to change nsIDocshell in 1.9.1? Or should there be yet another interface? >+ * If true, this browser is not visable in the traditional sence, but >+ * is actively being rendered to the screen (ex. painted on a canvas) >+ * and should be treated accordingly. >+ **/ s/visable/visible/ s/sence/sense/ And this kind of change needs testcases. In principle I like the idea of an "offscreen browser"
Attachment #338271 -
Flags: review?(Olli.Pettay) → review-
Updated•16 years ago
|
Flags: blocking-fennec1.0+
Target Milestone: --- → Fennec A2
Comment 9•16 years ago
|
||
Moving this to A1, since you can't use Fennec on a N800 unless this works.
Target Milestone: Fennec A2 → Fennec A1
Comment 10•16 years ago
|
||
In some situations I have to press the keys twice on the softkeyboard to get one character. Is that related to this as well?
Comment 11•16 years ago
|
||
(In reply to comment #10) > In some situations I have to press the keys twice on the softkeyboard to get > one character. Is that related to this as well? christian, it is because of bug 435885 , which has a patch r'ed+ by roc and 'checkin-needed'.
Assignee | ||
Comment 12•16 years ago
|
||
test to follow. Any suggestions welcome.
Attachment #338271 -
Attachment is obsolete: true
Attachment #338588 -
Flags: review?(Olli.Pettay)
Updated•16 years ago
|
Attachment #338588 -
Flags: review?(Olli.Pettay) → review+
Comment 13•16 years ago
|
||
Comment on attachment 338588 [details] [diff] [review] checks offscreen flag of the docshell as we walk the tree > // otherwise, we must walk up the document and view trees checking >- // for a hidden view. >+ // for a hidden view. unless, we're an off screen browser, which >+ // would make this test meaningless Sentences should start with capital letter and end to a '.' ;) And one extra space before 'meaningless'. >diff --git a/docshell/base/nsIDocShell.idl b/docshell/base/nsIDocShell.idl >--- a/docshell/base/nsIDocShell.idl >+++ b/docshell/base/nsIDocShell.idl I still wonder if we want to change nsIDocShell in 1.9.1. I would guess that some binary extensions do use nsIDocShell. But because Bug 396519 already did change the interface, this patch isn't breaking anything new. So ok for now, but please ask on mozilla.dev.platform what others think. If you change an interface, you should update iid.
Assignee | ||
Updated•16 years ago
|
Attachment #338588 -
Flags: superreview?(bzbarsky)
![]() |
||
Comment 14•16 years ago
|
||
Comment on attachment 338588 [details] [diff] [review] checks offscreen flag of the docshell as we walk the tree >+++ b/docshell/base/nsDocShell.cpp >+ // for a hidden view. unless, we're an off screen browser, which >+ // would make this test meaningless Comma before the "unless" not after. >+NS_IMETHODIMP >+nsDocShell::SetIsOffScreenBrowser(PRBool aIsOffScreen) { Curly on next line, please. >+nsDocShell::GetIsOffScreenBrowser(PRBool *aIsOffScreen) { And here. With that plus smaug's comments (about uuid, especially) addressed, looks good.
Attachment #338588 -
Flags: superreview?(bzbarsky) → superreview+
Comment 15•16 years ago
|
||
Comment on attachment 338272 [details] [diff] [review] associated change to fennec Seems to me that Fennec's browsers should just always be marked as offscreen, shouldn't they? That's why I asked the question in comment 5.
Assignee | ||
Comment 16•16 years ago
|
||
I guess I didn't understand your original comment. All the browsers are offscreen in that they're not visible in the traditional sense, but only the currently focused tab is being drawn to the canvas and meets the second criteria of the IsOffScreenBrowser property's comment. Because of this we need to set and unset the property when we switch tabs.
Assignee | ||
Comment 17•16 years ago
|
||
Attachment #338588 -
Attachment is obsolete: true
Attachment #338925 -
Flags: review?(Olli.Pettay)
Comment 18•16 years ago
|
||
In that case it seems to me like the attribute should be called "ignoreVisibility", or something like that.
Updated•16 years ago
|
Attachment #338925 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 19•16 years ago
|
||
changeset: 19435:c778ecf2d888 tag: tip user: Brad Lassey <blassey@mozilla.com> date: Fri Sep 19 12:57:56 2008 -0400 summary: Bug 454235 - IME events not fired when clicking on text input box, adds IsOffScreenBrowser property to DocShell r=smaug sr=bz
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 21•16 years ago
|
||
This isn't really the right fix for this. It's the view that becomes hidden not the docshell itself. There are a number of other calls to AreAncestorViewsVisible for instance that will still return false.
Comment 22•16 years ago
|
||
I had the impression that the .isOffscreenBrowser/etc. was enough for mobile. If that is not the case, there could be a followup bug to make views to handle "offscreen". Do we need a new nsViewVisibility enum value or some such?
Comment 23•16 years ago
|
||
(In reply to comment #22) > I had the impression that the .isOffscreenBrowser/etc. was enough for mobile. > If that is not the case, there could be a followup bug to make > views to handle "offscreen". Well, bug 455891 for instance where element.focus() doesn't work. Also, tabbing and accesskeys probably don't work although that perhaps isn't an issue for mobile. Do we need a new nsViewVisibility enum value or > some such? Possibly. Seems like nsDeckFrame could check for an attribute or something and flag the view. Or, find a way for AreAncestorViewsVisible to handle this case.
Comment 24•16 years ago
|
||
(In reply to comment #23) > Well, bug 455891 for instance where element.focus() doesn't work. I'm still not too familiar with the strange setup fennec has. One option might be to keep the views (and docshells) visible and remove the whole .isOffscreenBrowser. Fennec uses deckframe? There is a deck for a <canvas> and a decks for "tabs"? Or what is the setup? Could deckframe handle <canvas> + 1 visible deck. > Also, tabbing > and accesskeys probably don't work although that perhaps isn't an issue for > mobile. True. Accesskey handling needs non-hidden views. No idea if Fennec supports any accesskeys.
Comment 25•16 years ago
|
||
Comment on attachment 338272 [details] [diff] [review] associated change to fennec Huh, I thought this had already landed. Looks like we're going to revert this in bug 455891 though.
Attachment #338272 -
Attachment is obsolete: true
Attachment #338272 -
Flags: review?(gavin.sharp)
Updated•16 years ago
|
Component: General → Embedding: GRE Core
Product: Fennec → Core
QA Contact: general → gre
Target Milestone: Fennec A1 → ---
Updated•16 years ago
|
Component: Embedding: GRE Core → Document Navigation
QA Contact: gre → docshell
Updated•14 years ago
|
Keywords: inputmethod
You need to log in
before you can comment on or make changes to this bug.
Description
•