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

RESOLVED FIXED in mozilla26

Status

()

Core
Widget: WinRT
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
mozilla26
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.

Updated

5 years ago
Blocks: 838081
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
(Assignee)

Updated

5 years ago
No longer blocks: 838081
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]
(Assignee)

Updated

5 years ago
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
(Assignee)

Updated

5 years ago
Blocks: 838081
(Assignee)

Comment 1

5 years ago
Appears to be some sort of a timing issue with accessibility.
(Assignee)

Updated

5 years ago
Whiteboard: [preview-triage][8.1]feature=defect c=tbd u=tbd p=0 → [preview][8.1]feature=defect c=tbd u=tbd p=0

Updated

5 years ago
Blocks: 899390
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
(Assignee)

Comment 2

5 years ago
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
(Assignee)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Updated

5 years ago
Blocks: 903866
(Assignee)

Comment 5

5 years ago
Created attachment 802354 [details] [diff] [review]
patch v.1
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)
(Assignee)

Updated

5 years ago
Flags: needinfo?(jmathies)
Whiteboard: [preview] feature=defect c=tbd u=tbd p=0 → [preview] feature=defect c=tbd u=tbd p=2

Updated

5 years ago
Blocks: 909637
No longer blocks: 838081
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
(Assignee)

Updated

5 years ago
Component: Input → Widget: WinRT
Product: Firefox for Metro → Core
(Assignee)

Comment 7

5 years ago
Created attachment 802548 [details] [diff] [review]
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.
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+
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
(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

5 years ago
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.
(Assignee)

Comment 15

5 years ago
(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. :/
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
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...")
(Assignee)

Comment 20

5 years ago
Created attachment 803659 [details] [diff] [review]
don't crash, log instead
Attachment #803659 - Flags: feedback?(netzen)
Attachment #803659 - Flags: feedback?(netzen) → feedback+
(Assignee)

Comment 21

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Updated

5 years ago
Whiteboard: [preview] feature=defect c=tbd u=tbd p=2 → [preview] feature=defect c=content_features u=metro_firefox_user p=2

Updated

5 years ago
Blocks: 855297
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.