914331 - [MP] Defect - Soft keyboard requires two taps on a form inputs to display

Status: RESOLVED FIXED

(Core Graveyard :: Widget: WinRT, defect, P2)

Description

[Jim Mathies [:jimm]]

Win 8.1 specific. In tapping form inputs I'm not getting the soft keyboard on the first tab, only the second. Also, tapping outside an input requires two taps to hide the keyboard.

Comment 1

[Jim Mathies [:jimm]]

Appears to be some sort of a timing issue with accessibility.

Comment 2

[Jim Mathies [:jimm]]

I was concerned that the async events work might have broken this, but it doesn't appear to. We query after all events are processed, for some reason accessibility returns the document instead of the input element - 

mozilla::widget::winrt::MetroInput::OnPointerEntered
mozilla::widget::winrt::MetroInput::OnPointerPressed
mozilla::widget::winrt::MetroInput::OnPointerReleased
mozilla::widget::winrt::MetroInput::OnTapped
mozilla::widget::winrt::MetroInput::HandleSingleTap
mozilla::widget::winrt::MetroInput::DispatchAsyncEventIgnoreStatus
mozilla::widget::winrt::MetroInput::DispatchAsyncEventIgnoreStatus
mozilla::widget::winrt::MetroInput::DispatchAsyncEventIgnoreStatus
mozilla::widget::winrt::MetroInput::DispatchAsyncEventIgnoreStatus
mozilla::widget::winrt::UIABridge::get_ProviderOptions
UIABridge::GetPropertyValue: idProp=UIA_NativeWindowHandlePropertyId
mozilla::widget::winrt::UIABridge::Navigate
UIABridge::Navigate NavigateDirection_Parent
mozilla::widget::winrt::UIABridge::get_ProviderOptions
mozilla::widget::winrt::UIABridge::get_ProviderOptions
mozilla::widget::winrt::UIABridge::Navigate
UIABridge::Navigate NavigateDirection_Parent
mozilla::widget::winrt::UIABridge::GetFocus
name: http://mathies.com/mozilla/forms.html
description: 
Focus element flags: editable:0 focusable:1 readonly:1

Comment 3

[Jim Mathies [:jimm]]

Actually after testing a bit I think this was caused by bug 907410, and I can reproduce it on 8.0 as well. Accessibility updates after the gesture recognizer generates an ontapped event which we fire from HandleSingleTap. Looks like these events don't get delivered by the time Windows comes in looking for the focused element.

Comment 4

[Jim Mathies [:jimm]]

More detailed log output - 

mozilla::widget::winrt::MetroInput::OnPointerEntered
mozilla::widget::winrt::MetroInput::OnPointerPressed
dispatching 5200
mozilla::widget::winrt::MetroInput::OnPointerReleased
mozilla::widget::winrt::MetroInput::OnTapped
mozilla::widget::winrt::MetroInput::HandleSingleTap
mozilla::widget::winrt::MetroInput::DispatchAsyncEventIgnoreStatus
mozilla::widget::winrt::MetroInput::DispatchAsyncEventIgnoreStatus
mozilla::widget::winrt::MetroInput::DispatchAsyncEventIgnoreStatus
mozilla::widget::winrt::MetroInput::DispatchAsyncEventIgnoreStatus
dispatching 5201
WM_GETOBJECT
mozilla::widget::winrt::UIABridge::get_ProviderOptions
UIABridge::GetPropertyValue: idProp=UIA_NativeWindowHandlePropertyId
mozilla::widget::winrt::UIABridge::Navigate
UIABridge::Navigate NavigateDirection_Parent
mozilla::widget::winrt::UIABridge::get_ProviderOptions
mozilla::widget::winrt::UIABridge::get_ProviderOptions
mozilla::widget::winrt::UIABridge::Navigate
UIABridge::Navigate NavigateDirection_Parent
mozilla::widget::winrt::UIABridge::GetFocus
name: http://mathies.com/mozilla/forms.html
description: 
Focus element flags: editable:0 focusable:1 readonly:1
mozilla::widget::winrt::UIATextElement::ClearFocus
UIABridge::GetPropertyValue: idProp=UIA_IsControlElementPropertyId
UIABridge::GetPropertyValue: idProp=UIA_NativeWindowHandlePropertyId
mozilla::widget::winrt::UIABridge::get_BoundingRectangle
UIABridge::GetPropertyValue: idProp=UIA_ControlTypePropertyId
UIABridge::GetPropertyValue: idProp=49209
UIABridge: Unhandled property
UIABridge::GetPropertyValue: idProp=49213
UIABridge: Unhandled property
mozilla::widget::winrt::UIABridge::GetPatternProvider
UIABridge::GetPatternProvider=10014
mozilla::widget::winrt::UIABridge::GetPatternProvider
UIABridge::GetPatternProvider=10029
UIABridge::GetPropertyValue: idProp=UIA_IsPasswordPropertyId
dispatching 5202
mozilla::widget::winrt::MetroInput::OnPointerExited
dispatching 300
dispatching 302
dispatching 301
dispatching 300
mozilla::widget::winrt::UIABridge::FocusChangeEvent

Comment 5

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 6

[Marco Mucci [:MarcoM]]

Hey Jim, what point value would you like to assign to this defect?  I'll add it to Iteration #15.

Comment 7

[Jim Mathies [:jimm]]

Attachment: dispatch pending events v.1

Unfortunate that we have to do this since it'll cause problems if and when we try to completely split the threads. But we have to have an up to date dom by the time uia comes in looking for focus information.

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 802548 [details] [diff] [review]
dispatch pending events v.1

Review of attachment 802548 [details] [diff] [review]:
-----------------------------------------------------------------

It took me a little while to figure out how this is working. Hopefully we'll have some time after the v1 launch to reorganize our event processing

Comment 9

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #8)
> Comment on attachment 802548 [details] [diff] [review]
> dispatch pending events v.1
> 
> Review of attachment 802548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It took me a little while to figure out how this is working. Hopefully we'll
> have some time after the v1 launch to reorganize our event processing

Yeah. Once we are rolling out we just make the time.

This bug brought to light a big issue in the thread splitting work for me - accessibility. That module will be accessed via the winrt main thread. It is also heavily embedded in the dom and I doubt it is multi-thread safe. This may prevent us from splitting gecko / winrt input threads.

Comment 10

[Trevor Saunders (:tbsaunde)]

> This bug brought to light a big issue in the thread splitting work for me -
> accessibility. That module will be accessed via the winrt main thread. It is
> also heavily embedded in the dom and I doubt it is multi-thread safe. This
> may prevent us from splitting gecko / winrt input threads.

yeah, you can only use a11y on the same thread as the DOM / layout, and I don't see how we could reasonable change that requirement without making the dom / layout thread safe.  We could of course proxy things between threads if necessary and it isn't too slow for your application.

Comment 11

[Jim Mathies [:jimm]]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > This bug brought to light a big issue in the thread splitting work for me -
> > accessibility. That module will be accessed via the winrt main thread. It is
> > also heavily embedded in the dom and I doubt it is multi-thread safe. This
> > may prevent us from splitting gecko / winrt input threads.
> 
> yeah, you can only use a11y on the same thread as the DOM / layout, and I
> don't see how we could reasonable change that requirement without making the
> dom / layout thread safe.  We could of course proxy things between threads
> if necessary and it isn't too slow for your application.

Was wondering about that. I think it would be (more) messy proxying IAccessible, IAccessibleEx + the parts of UIA metro uses. Might be simpler to go for a full UIA implementation and just proxy that.

Comment 12

[alexander :surkov (:asurkov)]

I was thinking about IAccessibleEx as temporary solution, as a bridge between a11y core and UIA. Later I wanted to get a mapping from a11y core to full UIA.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out along with bug 914829 for intermittent mochitest-mc crashes.
https://hg.mozilla.org/integration/fx-team/rev/4be36d56d323

https://tbpl.mozilla.org/php/getParsedLog.php?id=27734694&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=27734168&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=27734386&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=27734018&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=27734103&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=27734434&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=27734211&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=27728411&tree=Fx-Team

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to Jim Mathies [:jimm] from comment #11)
> (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > > This bug brought to light a big issue in the thread splitting work for me -
> > > accessibility. That module will be accessed via the winrt main thread. It is
> > > also heavily embedded in the dom and I doubt it is multi-thread safe. This
> > > may prevent us from splitting gecko / winrt input threads.
> > 
> > yeah, you can only use a11y on the same thread as the DOM / layout, and I
> > don't see how we could reasonable change that requirement without making the
> > dom / layout thread safe.  We could of course proxy things between threads
> > if necessary and it isn't too slow for your application.
> 
> Was wondering about that. I think it would be (more) messy proxying
> IAccessible, IAccessibleEx + the parts of UIA metro uses. Might be simpler
> to go for a full UIA implementation and just proxy that.

Well, the other thing we could do is proxy our internal api, which may be useful for e10s, but I really haven't thought about it much.

Comment 15

[Jim Mathies [:jimm]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13)
> Backed out along with bug 914829 for intermittent mochitest-mc crashes.
> https://hg.mozilla.org/integration/fx-team/rev/4be36d56d323
> 

https://tbpl.mozilla.org/php/getParsedLog.php?id=27734386&full=1&branch=fx-team#error0

This work enabled the "crash on purpose" code for re-entrant calls to ProcessOneNativeEventIfPresent, which is what most of these are.

Also seems our crash stack walker can't go deep enough to see what's triggering this. :/

Comment 16

[Jim Mathies [:jimm]]

interesting - 

17:52:45     INFO -   8  xul.dll!XPCWrappedNative::CallMethod(XPCCallContext &,XPCWrappedNative::CallMode) [XPCWrappedNative.cpp:35a772328e24 : 2110 + 0x75d]
17:52:45     INFO -      eip = 0x73d1bfa1   esp = 0x0305c8a0   ebp = 0x0305c9e8
17:52:45     INFO -      Found by: call frame info
17:52:45     INFO -   9  xul.dll!xpc::JSMemoryMultiReporter::CollectReports(nsDataHashtable<nsUint64HashKey,nsCString> *,nsDataHashtable<nsUint64HashKey,nsCString> *,nsIMemoryMultiReporterCallback *,nsISupports *) [XPCJSRuntime.cpp:35a772328e24 : 2692 + 0x3]
17:52:45     INFO -      eip = 0x73d1010a   esp = 0x0305c900   ebp = 0x0305c9e8
17:52:45     INFO -      Found by: stack scanning
17:52:45     INFO -  10  xul.dll!XPC_WN_CallMethod(JSContext *,unsigned int,JS::Value *) [XPCWrappedNativeJSOps.cpp:35a772328e24 : 1307 + 0xc]

So my guess is we are processing a NativeCallback and drop into a js event dispatch loop.

Comment 17

[Jim Mathies [:jimm]]

Ted, curious if you can help me here, I'm having a hard time making sense of this stack - 

https://tbpl.mozilla.org/php/getParsedLog.php?id=27734168&full=1&branch=fx-team#error0

Specifically this frame - 

xul.dll!xpc::JSMemoryMultiReporter::CollectReports(nsDataHashtable<nsUint64HashKey,nsCString> *,nsDataHashtable<nsUint64HashKey,nsCString> *,nsIMemoryMultiReporterCallback *,nsISupports *) [XPCJSRuntime.cpp:a1a846de1a8a : 2692 + 0xc]

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSRuntime.cpp#2692

which points to no mans land. I also don't see where CollectReports might invoke nsThread::ProcessNextEvent?

Comment 18

[(not currently active) Ted Mielczarek]

Note the "Found by: stack scanning", which means that the stack walker is kind of guessing here. The rest of that stack looks pretty sane though, so I'd just ignore that one frame. I think you're just seeing JS spin the event loop.

Comment 19

[(not currently active) Ted Mielczarek]

If you can reproduce this on try, you can make your build upload PDB files:
https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_debug_symbols

and then ask someone in releng to grab the minidump from the slave for you (in the log you'll see "Saved dump as...")

Comment 20

[Jim Mathies [:jimm]]

Attachment: don't crash, log instead

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 803659 [details] [diff] [review]
don't crash, log instead

Longer term I think we need override more of nsBaseAppShell, but I'm not interested in doing that three days before aurora uplift. So lets land this work around which is what we've been running with for the last few weeks (prior to the landing here) and I'll file a follow up on more app shell event work.

Comment 22

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/15ba1cea7963

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/15ba1cea7963