454235 - IME events not fired when clicking on text input box

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP, removes failed test with #if 0

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?

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: adds isOffScreenBrowser to docshell

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.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: adds isOffScreenBrowser to docshell, cleaned up a bit

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: associated change to fennec

Is this sufficient, or do we need to reset the flag on other events?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

I realized as I submitted that comment that that line should be "currentBrowser.docShell.isOffScreenBrowser = false;"

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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"

Comment 9

[Christian Sejersen]

Moving this to A1, since you can't use Fennec on a N800 unless this works.

Comment 10

[Christian Sejersen]

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

[Antonio Gomes (tonikitoo)]

(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'.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: checks offscreen flag of the docshell as we walk the tree

test to follow.  Any suggestions welcome.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 14

[Boris Zbarsky [: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.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: update based on comments, including tests

Comment 18

[:Gavin Sharp [email: [e-mail]]]

In that case it seems to me like the attribute should be called "ignoreVisibility", or something like that.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

changeset:   19435:c778ecf2d888
tag:         tip
user:        Brad Lassey <[e-mail]>
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

Comment 21

[Neil Deakin]

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

[Olli Pettay [:smaug][[e-mail]]]

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

[Neil Deakin]

(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

[Olli Pettay [:smaug][[e-mail]]]

(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

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

verified in beta3