Closed Bug 454235 Opened 11 years ago Closed 11 years ago

IME events not fired when clicking on text input box

Categories

(Core :: DOM: Navigation, defect)

x86
Linux
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: blassey, Assigned: blassey)

References

Details

(Keywords: inputmethod, mobile)

Attachments

(1 file, 5 obsolete files)

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.
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
Blocks: 406837
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)
Attachment #338268 - Attachment is obsolete: true
Attachment #338271 - Flags: review?(Olli.Pettay)
Attachment #338268 - Flags: review?(Olli.Pettay)
Attached patch associated change to fennec (obsolete) — Splinter Review
Is this sufficient, or do we need to reset the flag on other events?
Attachment #338272 - Flags: review?
Attachment #338272 - Flags: review? → review?(gavin.sharp)
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?
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.
I realized as I submitted that comment that that line should be "currentBrowser.docShell.isOffScreenBrowser = false;"
Blocks: 435446
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-
Flags: blocking-fennec1.0+
Target Milestone: --- → Fennec A2
Moving this to A1, since you can't use Fennec on a N800 unless this works.
Target Milestone: Fennec A2 → Fennec A1
In some situations I have to press the keys twice on the softkeyboard to get one character. Is that related to this as well?
(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'.
test to follow.  Any suggestions welcome.
Attachment #338271 - Attachment is obsolete: true
Attachment #338588 - Flags: review?(Olli.Pettay)
Attachment #338588 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #338588 - Flags: superreview?(bzbarsky)
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 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.
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.
Attachment #338588 - Attachment is obsolete: true
Attachment #338925 - Flags: review?(Olli.Pettay)
In that case it seems to me like the attribute should be called "ignoreVisibility", or something like that.
Attachment #338925 - Flags: review?(Olli.Pettay) → review+
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: 11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 435446
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.
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?
(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.
(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 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)
Component: General → Embedding: GRE Core
Product: Fennec → Core
QA Contact: general → gre
Target Milestone: Fennec A1 → ---
Component: Embedding: GRE Core → Document Navigation
QA Contact: gre → docshell
verified in beta3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.