Closed Bug 1224604 Opened 4 years ago Closed 4 years ago

Focusing on any element in B2GDroid when Talkback enabled crashes and reboots B2GDroid

Categories

(B2GDroid Graveyard :: General, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: yzen, Assigned: sgiles)

References

Details

(Keywords: access)

Attachments

(1 file, 3 obsolete files)

When talkback is enabled, trying to focus on any element, app icon on homescreen for example, will trigger a crash+reboot of the B2GDroid.
Assignee: nobody → sgiles
Assignee: sgiles → nobody
Priority: -- → P1
Assignee: nobody → sgiles
Attached patch part1-talkback-crash.patch (obsolete) — Splinter Review
Hey, this is what I found after the crash I mentioned in IRC the yesterday.  

It seems that TalkBack sends hover events, these were then being sent to Gecko and not being handled in the multi touch event handler.  

I'm not sure of the flow of control or why these hover events are being handled in multi-touch. 

This patch stops the crash.  But I'm not sure if it's the right patch.
Attachment #8691420 - Flags: feedback?(bugmail.mozilla)
Attached patch part2-talkback-crash.patch (obsolete) — Splinter Review
The accessibility code makes a lot of assumptions about the product it's running.  Here I'm looking for some cross between b2g and mobile/android for b2gdroid, which seems quite messy right now.  What's the best course of action here? Refactor to be more generic - we could just add the ability for the chrome to configure the presenters it needs for example, leaving the existing default configurations for b2g et.al. intact.

Thoughts?
Attachment #8691426 - Flags: feedback?(surkov.alexander)
Comment on attachment 8691420 [details] [diff] [review]
part1-talkback-crash.patch

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

So generally this looks ok from my side, although I don't know if the accessibility code is set up to handle hover events as touch events. The one change I would suggest is wrapping the LayerView.java change inside an if (AppConstants.MOZ_ANDROID_APZ) guard, because we probably don't want to be doing that unless the C++ APZ code is enabled. That will preserve the existing Fennec behaviour.
Attachment #8691420 - Flags: feedback?(bugmail.mozilla) → feedback+
Attachment #8691426 - Flags: feedback?(surkov.alexander) → feedback?(yzenevich)
Attached patch part1-talkback-crash.patch (obsolete) — Splinter Review
Added `AppConstants.MOZ_ANDROID_APZ` guard.
Attachment #8691420 - Attachment is obsolete: true
Attachment #8692046 - Flags: review?(bugmail.mozilla)
Attachment #8692046 - Flags: review?(bugmail.mozilla) → review+
Added r=
Check in please!

I'm going to take part2 into another bug.
Attachment #8692046 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to sgiles from comment #5)
> Created attachment 8692061 [details] [diff] [review]
> part1-talkback-crash.patch
> 
> Added r=
> Check in please!
> 
> I'm going to take part2 into another bug.

I'm trying a build with part 1+2 right now. Imidiate suggestion I have is to separate mobile/android and mobile/android/b2gdroid so we would only load correct presenters and resolve things in utils separately.
(In reply to Yura Zenevich [:yzen] from comment #6)
> 
> I'm trying a build with part 1+2 right now. Imidiate suggestion I have is to
> separate mobile/android and mobile/android/b2gdroid so we would only load
> correct presenters and resolve things in utils separately.

Yes, I was thinking grander, what if the b2gdroid chrome was able to register with AccessFu the presenters it required?  This would make new consumers of ./accessible much easier moving forward?
Comment on attachment 8691426 [details] [diff] [review]
part2-talkback-crash.patch

Moved this over to bug 1228039 for discussion.
Attachment #8691426 - Attachment is obsolete: true
Attachment #8691426 - Flags: feedback?(yzenevich)
https://hg.mozilla.org/mozilla-central/rev/414d106b2e20
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.