Closed
Bug 968991
Opened 11 years ago
Closed 11 years ago
OOP Keyboard issues with Nuwa
Categories
(Firefox OS Graveyard :: Gaia::Keyboard, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: fabrice, Assigned: kats)
References
Details
(Whiteboard: [3rd-party-keyboard][ft:system-platform])
Attachments
(6 files, 4 obsolete files)
(deleted),
text/x-log
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
We had to disable OOP keyboards to keep Nuwa on. Let's investigate and fix the issues.
Comment 1•11 years ago
|
||
Blocks a 1.4 committed feature - oop keyboard support
Blocks: 3rd-party-keyboard, Nuwa
blocking-b2g: --- → 1.4+
Comment 2•11 years ago
|
||
As pointed out by bug 963584 comment 28, when this issue occurs, we cannot get correct height of the keyboard because window.innerHeight is 0.
And we have 3 workarounds as follows, while I think #2, #3 are talking about the same thing,
1. Disable APZ.
2. or Disable Nuwa.
3. (the current workaournd) Disable keyboard OOP.
--
BTW, this issue won't occur after I re-enable keyboard OOP and also make keyboard not to be pre-launch at boot time.
See the attachment for the exact setting entries.
Updated•11 years ago
|
Whiteboard: [3rd-party-keyboard]
Comment 3•11 years ago
|
||
Hi Kats,
Could you help shed some light on why we could get "window.innerHeight == 0" when:
APZC is on and we launch keyboard app immediately after boot?
Thanks.
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 4•11 years ago
|
||
When APZC is on, the innerHeight is computed in TabChild::HandlePossibleViewportChange. Can you check to see if that function is getting run for the keyboard process, and if so, what it passes to SetCSSViewport at [1]?
[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#550
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(rlu)
Comment 5•11 years ago
|
||
Before the following timestamp, the viewport size of keyboard is still (0, 0).
Note: The keyboard app is prelaunched after boot, and not triggered by the user yet.
---
02-10 06:42:10.529: I/Gecko(881): early return for no screenW or no screenH
But after I clicked on an input field, we could see that the viewport has been set as (320, 480).
However, the keyboard app still get 0 from window.innerHeight.
--
02-10 06:43:21.229: I/Gecko(881): In TabChild::HandlePossibleViewportChange()
02-10 06:43:21.229: I/Gecko(881): screenW: 320.000000 screeH: 480.000000
02-10 06:43:21.229: I/Gecko(881): viewport.width: 320.000000, viewport.height: 480.000000
Flags: needinfo?(rlu)
Comment 6•11 years ago
|
||
Kats,
Could you please help check comment 5 and let us know what else we should look into?
Thank you very much.
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 7•11 years ago
|
||
If the viewport is set to a 480 height, then that is what should be coming out of window.innerHeight. If that is not then you'll have to trace into nsGlobalWindow::GetInnerHeight to see why that's not the case.
Flags: needinfo?(bugmail.mozilla)
Comment 8•11 years ago
|
||
I found that nsIPresShell::SetScrollPositionClampingScrollPortSize() was called with (0, 0) in TabChild::HandlePossibleViewportChange(), which result the 0 innerHeight.
At the end, I traced back to the following line and found the mZoom would be zero here.
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/FrameMetrics.h#165
--
02-11 22:12:32.319: I/Gecko(2740): FrameMetrics::CalculateCompositedRectInCssPixels(), width: 320, height: 480, zoom: 0.000000
You may find the full log of both failure or success cases in the attachment.
Comment 9•11 years ago
|
||
This dump also contains the APZ debug logs, but seems before APZ update anything about mZoom, mZoom is already 0.
Kats, could you help take a look what else should we look at to know how mZoom was set for FrameMetrics?
Thanks.
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 10•11 years ago
|
||
Hm, interesting. metrics.mZoom is updated just a little farther up in HandlePossibleViewportChange:
metrics.mZoom.scale *= metrics.CalculateIntrinsicScale().scale / oldIntrinsicScale;
which seems to imply that either metrics.mZoom was already 0, or metrics.CalculateIntrinsicScale().scale is 0, which in turn means that the composition bounds width is zero. But that's obviously not the case from your logs. So it seems probably that metrics.mZoom was 0 before this, which means mLastRootMetrics.mZoom was 0, which goes all the way back to mInnerSize being zero at the time the before-first-paint event was dispatched.
Can you verify that mInnerSize is 0 inside the BEFORE_FIRST_PAINT handler where mLastRootMetrics is initialized? That seems like an underlying bug to me but I could be wrong - that might be normal. In which case we should just update this block of code:
float oldScreenWidth = mLastRootMetrics.mCompositionBounds.width;
if (!oldScreenWidth) {
oldScreenWidth = mInnerSize.width;
}
to something like this:
if (!mLastRootMetrics.mCompositionBounds.width) {
mLastRootMetrics.mCompositionBounds.SizeTo(mInnerSize);
mLastRootMetrics.mZoom = mLastRootMetrics.CalculateIntrinsicScale();
}
float oldScreenWidth = mLastRootMetrics.mCompositionBounds.width;
and hopefully that will avoid the problem. Please give that a shot and let me know if it works.
Flags: needinfo?(bugmail.mozilla)
Comment 11•11 years ago
|
||
I tried to apply the change and it works.
Comment 12•11 years ago
|
||
(In reply to Cervantes Yu from comment #11)
> I tried to apply the change and it works.
Hmm, it works "to some extent". The keyboard sometimes works, sometimes not.
Updated•11 years ago
|
Whiteboard: [3rd-party-keyboard] → [3rd-party-keyboard][tarako]
Updated•11 years ago
|
Whiteboard: [3rd-party-keyboard][tarako] → [3rd-party-keyboard][tarako][ft:system-platform]
Updated•11 years ago
|
Assignee: nobody → cyu
Comment 13•11 years ago
|
||
This is not the correct way to fix the problem, but works the problem around.
Comment 14•11 years ago
|
||
We found that TabChild::RecvUpdateDimensions() happens after TabChild::Observe() that calculates mLastRootMetrics.mZoom. If we can update mZoom when we really needs to show, then the keyboard app works. This looks to be a timing issue. :kats, would you take a look at the WIP and work on the right fix?
Assignee: cyu → nobody
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 15•11 years ago
|
||
You said that the change from comment 11 works sometimes, but that your patch in comment 13 always works? That seems odd to me, because every time your code in comment 13 gets hit, the code path in comment 11 should also get hit, because RecvUpdateDimensions calls HandlePossibleViewportChange. The only real difference between the two patches is that mine checks for a zero composition bounds and yours does not. I guess it might be that ProcessUpdateFrame is getting called between the before-first-paint handler and the RecvUpdateDimensions, and that is setting the composition bounds to something else; but if that is the case the zoom should also be getting set to something else.
But yeah I guess I can look at this. Can you give me full steps to reproduce? Do I just need to apply the patch in comment 2, or is there something else that needs to be done as well?
Flags: needinfo?(bugmail.mozilla)
Comment 16•11 years ago
|
||
Hi Kats,
Please try this patch to re-enable keyboard OOP in Gaia, it should be easy to reproduce this issue, just click any input field after the keyboard process is already launched, and then you would see the keyboard won't show up.
Thanks for your help on this issue.
Attachment #8372117 -
Attachment is obsolete: true
Flags: needinfo?(bugmail.mozilla)
Comment 17•11 years ago
|
||
Moving to 1.5? as third party keyboard is moving out of scope for 1.4
blocking-b2g: 1.4+ → 1.5?
Comment 18•11 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> You said that the change from comment 11 works sometimes, but that your
> patch in comment 13 always works?
I tested both patches today and yes, patch in comment 11 works sometimes and the one from comment 13 works always.
Assignee | ||
Comment 19•11 years ago
|
||
Unfortunately I'm unable to reproduce this on my Peak device on master. I tried both debug and non-debug builds and in both cases the keyboard seems to work fine. Can you apply this patch and reproduce the problem and post the logcat?
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 20•11 years ago
|
||
Oh, my mistake. I used the old patch to re-enable the keyboard, which had some other pref change as well. With the right patch applied I can repro this problem.
Comment 21•11 years ago
|
||
Jason, how many times is third party keyboard support going to be pushed to the next version? I have developers anxiously waiting to develop third party keyboards.
Assignee | ||
Comment 22•11 years ago
|
||
This patch seems to fix the problem reliably. I'm not that confident in my knowledge of this code though so it may have bad side effects.
Assignee | ||
Updated•11 years ago
|
Attachment #8378161 -
Attachment is obsolete: true
Assignee | ||
Comment 23•11 years ago
|
||
Attachment #8379004 -
Attachment is obsolete: true
Assignee | ||
Comment 24•11 years ago
|
||
Cervantes and/or Rudy, can you apply the patch from attachment 8379120 [details] [diff] [review] to see if it reliably fixes the problem for you? If not, can you also apply the updated logging patch and get a full log from startup to the keyboard not showing? Thanks.
Comment 25•11 years ago
|
||
This reliably fixes the problem on my unagi.
Assignee | ||
Updated•11 years ago
|
Attachment #8379120 -
Flags: review?(botond)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → bugmail.mozilla
Comment 26•11 years ago
|
||
Comment on attachment 8379120 [details] [diff] [review]
Patch
Review of attachment 8379120 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/ipc/TabChild.cpp
@@ +375,5 @@
> + // In some cases before-first-paint gets called before
> + // RecvUpdateDimensions is called and therefore before we have an
> + // mInnerSize value set. In such cases defer initializing the viewport
> + // until we we get an inner size.
> + if (mInnerSize.width && mInnerSize.height) {
It might enhance readability to introduce a HaveValidInnerSize() function and use it or its negation as appropriate.
Attachment #8379120 -
Flags: review?(botond) → review+
Assignee | ||
Comment 27•11 years ago
|
||
Updated to pull out a HasValidInnerSize function as per review comments. Pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=ba1bed82e0a3; if that's green I'll land the patch.
Attachment #8379120 -
Attachment is obsolete: true
Attachment #8379869 -
Flags: review+
Assignee | ||
Comment 28•11 years ago
|
||
Comment 29•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 30•11 years ago
|
||
move to 1.3T? triage to decide if this is wanted for tarako
if not, let's move this back to 1.5?
blocking-b2g: 1.5? → 1.3T?
Updated•11 years ago
|
Whiteboard: [3rd-party-keyboard][tarako][ft:system-platform] → [3rd-party-keyboard][ft:system-platform]
Updated•11 years ago
|
Component: General → Gaia::Keyboard
Updated•11 years ago
|
blocking-b2g: 2.0? → ---
Comment 32•11 years ago
|
||
Verified it. Cannot reproduce this bug.
Also, no failure shows on keyboard testing result (Gaia UI test).
Thanks.
* Build Information:
- Gaia f55fc5c507312c7aac51ec9bb73061fd4ed5c5fb
- Gecko https://hg.mozilla.org/mozilla-central/rev/3285e030d9c0
- BuildID 20140504160202
- Version 32.0a1
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•