Closed Bug 914331 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [preview] feature=defect c=content_features u=metro_firefox_user p=2)

Attachments

(2 files, 1 obsolete file)

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.
Summary: Soft keyboard requires two taps on a form inputs to display → Defect - Soft keyboard requires two taps on a form inputs to display
Whiteboard: [preview-triage] → [preview-triage] feature=defect c=tbd u=tbd p=0
No longer blocks: metrov1backlog
Summary: Defect - Soft keyboard requires two taps on a form inputs to display → Soft keyboard requires two taps on a form inputs to display
Whiteboard: [preview-triage] feature=defect c=tbd u=tbd p=0 → [preview-triage][8.1]
Summary: Soft keyboard requires two taps on a form inputs to display → Defect - Soft keyboard requires two taps on a form inputs to display
Whiteboard: [preview-triage][8.1] → [preview-triage][8.1]feature=defect c=tbd u=tbd p=0
Appears to be some sort of a timing issue with accessibility.
Whiteboard: [preview-triage][8.1]feature=defect c=tbd u=tbd p=0 → [preview][8.1]feature=defect c=tbd u=tbd p=0
Summary: Defect - Soft keyboard requires two taps on a form inputs to display → [MP] Defect - Soft keyboard requires two taps on a form inputs to display
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
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.
Blocks: 907410
Whiteboard: [preview][8.1]feature=defect c=tbd u=tbd p=0 → [preview] feature=defect c=tbd u=tbd p=0
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
Blocks: 903866
Attached patch patch v.1 (obsolete) — Splinter Review
Assignee: nobody → jmathies
Hey Jim, what point value would you like to assign to this defect?  I'll add it to Iteration #15.
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
Whiteboard: [preview] feature=defect c=tbd u=tbd p=0 → [preview] feature=defect c=tbd u=tbd p=2
Blocks: metrov1it15
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Component: Input → Widget: WinRT
Product: Firefox for Metro → Core
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.
Attachment #802354 - Attachment is obsolete: true
Attachment #802548 - Flags: review?(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
Attachment #802548 - Flags: review?(tabraldes) → review+
(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.
> 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.
(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.
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.
(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.
(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. :/
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.
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?
Flags: needinfo?(ted)
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.
Flags: needinfo?(ted)
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...")
Attachment #803659 - Flags: feedback?(netzen)
Attachment #803659 - Flags: feedback?(netzen) → feedback+
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.
Attachment #803659 - Flags: review?(netzen)
Attachment #803659 - Flags: review?(netzen) → review+
https://hg.mozilla.org/mozilla-central/rev/15ba1cea7963
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Whiteboard: [preview] feature=defect c=tbd u=tbd p=2 → [preview] feature=defect c=content_features u=metro_firefox_user p=2
Blocks: 855297
OS: Windows 8 Metro → Windows 8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.