Last Comment Bug 755976 - Virtual D-Pad does not send key press events when caret browsing is enabled
: Virtual D-Pad does not send key press events when caret browsing is enabled
Status: VERIFIED FIXED
[Q2 a11y dependency]
:
Product: Firefox for Android
Classification: Client Software
Component: Keyboards and IME (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 15
Assigned To: Eitan Isaacson [:eeejay]
:
Mentors:
: 755573 (view as bug list)
Depends on:
Blocks: 755573
  Show dependency treegraph
 
Reported: 2012-05-16 17:48 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-10-24 13:07 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
testcase (1.41 KB, text/html)
2012-05-17 12:07 PDT, Eitan Isaacson [:eeejay]
no flags Details
Send empty text and selection when IME is disabled. (1.63 KB, patch)
2012-06-06 12:31 PDT, Eitan Isaacson [:eeejay]
cpeterson: review+
cpeterson: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Eitan Isaacson [:eeejay] 2012-05-16 17:48:01 PDT
Steps to reproduce:
1. In about:config set accessibility.accessfu to 0
2. Use eyes-free keyboard, switch to "navigation mode", this is the translucent rectangle that takes up the bottom part of the screen.
3. Navigate to http://monotonous.org/imebug.html
4. Swipe right a few times, watch the counter go up.
5. Press the button.
6. Swipe right, watch the counter not move.

Looks like the input method is controlling the caret instead of sending hw key events. I wonder why setting the selection does that.
Comment 1 Chris Peterson [:cpeterson] 2012-05-17 10:48:04 PDT
Eitan says he tested Nexus S (running CM9) and that the text NS_QUERY_TEXT_CONTENT returns seems wrong and arbitrary.
Comment 2 Eitan Isaacson [:eeejay] 2012-05-17 10:59:26 PDT
(In reply to Chris Peterson (:cpeterson) from comment #1)
> Eitan says he tested Nexus S (running CM9) and that the text
> NS_QUERY_TEXT_CONTENT returns seems wrong and arbitrary.

Yup,

By wrong I mean that the entire text contents of the page are returned, including any javascript or css content and title in the header.

By arbitrary I mean that if a selection is set in the page (e.g. win.getSelection().collapse(element, 0)), NS_QUERY_TEXT_CONTENT returns the page's contents, and if not, the event dispatch fails because of:
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsContentEventHandler.cpp#100

To be clear, I prefer OnIMETextChange always notify with empty text when the caret is in a r/o place. Otherwise, the virtual d-pad directly controls the cursor (badly) instead of sending key events.
Comment 3 Eitan Isaacson [:eeejay] 2012-05-17 12:07:14 PDT
Created attachment 624826 [details]
testcase

Sorry, I guess the reproduction steps earlier were wrong. Here are new ones:
1. In about:config set
  - accessibility.accessfu to 0
  - enable accessibility.browsewithcaret
2. Use eyes-free keyboard, switch to "navigation mode", this is the translucent rectangle that takes up the bottom part of the screen.
3. Navigate to http://monotonous.org/imebug.html
4. Swipe down a few times watch the counter go up.
6. Swipe right, watch the counter not move.

You could set the caret by tapping on the text, but swiping left or right will only move the caret intermittently, and no keypress event will happen (in other words the ime is doing the caret moving, and a bad job at it).
Comment 4 Chris Peterson [:cpeterson] 2012-05-17 13:32:09 PDT
Eitan, I have a Sony Xperia with a real D-pad. I see similar, but intermittent, results when trying to browsewithcaret using the real D-pad.
Comment 5 Eitan Isaacson [:eeejay] 2012-05-17 13:37:23 PDT
(In reply to Chris Peterson (:cpeterson) from comment #4)
> Eitan, I have a Sony Xperia with a real D-pad. I see similar, but
> intermittent, results when trying to browsewithcaret using the real D-pad.

Do you see the count go up? I do with the hw keyboard on a droid 2.
Comment 6 Chris Peterson [:cpeterson] 2012-05-17 13:58:57 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #5)
> Do you see the count go up? I do with the hw keyboard on a droid 2.

I tried the Sony Xperia again and it seems to work. I can move the caret around (though sometimes it is inserted at a different position than where I tapped) and the counter goes up.
Comment 7 Eitan Isaacson [:eeejay] 2012-05-17 14:09:32 PDT
(In reply to Chris Peterson (:cpeterson) from comment #6)
> (In reply to Eitan Isaacson [:eeejay] from comment #5)
> > Do you see the count go up? I do with the hw keyboard on a droid 2.
> 
> I tried the Sony Xperia again and it seems to work. I can move the caret
> around (though sometimes it is inserted at a different position than where I
> tapped) and the counter goes up.

Right. Broken caret navigation is another issue. The main issue here is that the virtual d-pad does not send key press events at all.
Comment 8 Eitan Isaacson [:eeejay] 2012-05-17 14:18:05 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #7)
> (In reply to Chris Peterson (:cpeterson) from comment #6)
> > (In reply to Eitan Isaacson [:eeejay] from comment #5)
> > > Do you see the count go up? I do with the hw keyboard on a droid 2.
> > 
> > I tried the Sony Xperia again and it seems to work. I can move the caret
> > around (though sometimes it is inserted at a different position than where I
> > tapped) and the counter goes up.
> 
> Right. Broken caret navigation is another issue. The main issue here is that
> the virtual d-pad does not send key press events at all.

Hardware keyboard, bluetooth and slideout,  seem to unconditionally send keypress events. The virtual d-pad does not send left/right events when caret navigation is on.

I guess that sums up the issue, sorry for the muddled descriptions earlier. I'm trying to keep it simple and reproducible :)
Comment 9 Eitan Isaacson [:eeejay] 2012-05-30 18:49:41 PDT
I tried backing out some things we do with the caret so we don't run in to this issue. But it seems like once you have an item with keyboard focus in the document, the IME starts controlling the caret instead of sending hardware events to the content.
Comment 10 Chris Peterson [:cpeterson] 2012-05-30 19:16:15 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #9)
> I tried backing out some things we do with the caret so we don't run in to
> this issue. But it seems like once you have an item with keyboard focus in
> the document, the IME starts controlling the caret instead of sending
> hardware events to the content.

Is the IME "stealing" the caret a bug?
Comment 11 Eitan Isaacson [:eeejay] 2012-05-30 20:29:38 PDT
(In reply to Chris Peterson (:cpeterson) from comment #10)
> (In reply to Eitan Isaacson [:eeejay] from comment #9)
> > I tried backing out some things we do with the caret so we don't run in to
> > this issue. But it seems like once you have an item with keyboard focus in
> > the document, the IME starts controlling the caret instead of sending
> > hardware events to the content.
> 
> Is the IME "stealing" the caret a bug?

It is stealing the read-only page caret (the one you get when you press f7 on desktop).

I think it is a bug, or at least it is problematic because:
1. It is not consistent with hardware keyboard behavior.
2. It conflicts with content by not supporting DOM events.
3. It does it wrong (last time I checked), the IME gets updated with all the text nodes in a document, including javascript and css code (not to mention invisible content). So the caret navigation that it tries to do is off.
4. It only happens in hard to reproduce edge cases (our a11y scripts being one of them).
Comment 12 Eitan Isaacson [:eeejay] 2012-06-06 12:31:35 PDT
Created attachment 630670 [details] [diff] [review]
Send empty text and selection when IME is disabled.

I still don't have a deep understanding of the IME codebase, but this patch seems to work. It ensures we don't send to Android text or selection info when the input context for the widget shows it is not editable (right?).

I could put this through try, but it would be nice to see what Chris thinks.
Comment 13 Chris Peterson [:cpeterson] 2012-06-06 12:39:25 PDT
Comment on attachment 630670 [details] [diff] [review]
Send empty text and selection when IME is disabled.

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

The idea seems reasonable, though I wonder if resetting the selection for non-editable content might interfere with selecting page text. Conveniently, that is not a feature we currently support. :)
Comment 14 Eitan Isaacson [:eeejay] 2012-06-06 12:47:20 PDT
Putting this in try before asking for review. I would love to see this approved for aurora too.
Comment 15 Marco Zehe (:MarcoZ) 2012-06-06 22:30:06 PDT
Eitan, did the try build complete? If so, what's the link?
Comment 16 Eitan Isaacson [:eeejay] 2012-06-06 23:18:50 PDT
(In reply to Marco Zehe (:MarcoZ) from comment #15)
> Eitan, did the try build complete? If so, what's the link?

https://tbpl.mozilla.org/?tree=Try&rev=c3873f06b4fc

Relatively green with some orange zest :) I'll look into it tomorrow.
Comment 17 Marco Zehe (:MarcoZ) 2012-06-07 01:38:02 PDT
I just tested this try-server build and find that it works extremely well on the Galaxy Nexus using the Eyes-Free keyboard in navigation mode. Swiping left and right through website content is extremely fast and reliable.
Comment 18 Eitan Isaacson [:eeejay] 2012-06-07 10:19:56 PDT
Comment on attachment 630670 [details] [diff] [review]
Send empty text and selection when IME is disabled.

Ok, the try tbpl URL above shows some orange, but I can't see how it is related to this patch. Requesting review.
Comment 19 Chris Peterson [:cpeterson] 2012-06-07 11:00:21 PDT
Comment on attachment 630670 [details] [diff] [review]
Send empty text and selection when IME is disabled.

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

LGTM, r+

Eitan, when you say you would like this bug approved for Aurora, do you mean Fennec 15 (Aurora on the trains) or a post-release update to Fennec 14 (the current beta in the Google Play market)? This sounds like a good fix for Fennec 15 (maybe after baking on Nightly 16 for a couple days), but may be to risky for Fennec 14.
Comment 20 Eitan Isaacson [:eeejay] 2012-06-07 11:05:14 PDT
(In reply to Chris Peterson (:cpeterson) from comment #19)
> Comment on attachment 630670 [details] [diff] [review]
> Send empty text and selection when IME is disabled.
> 
> Review of attachment 630670 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, r+
> 
> Eitan, when you say you would like this bug approved for Aurora, do you mean
> Fennec 15 (Aurora on the trains) or a post-release update to Fennec 14 (the
> current beta in the Google Play market)? This sounds like a good fix for
> Fennec 15 (maybe after baking on Nightly 16 for a couple days), but may be
> to risky for Fennec 14.

Thanks! I mean Fennec 15. I am still confused about the mobile release schedule. Fennec 14 does not have accessibility support.
Comment 22 Graeme McCutcheon [:graememcc] 2012-06-08 04:20:36 PDT
https://hg.mozilla.org/mozilla-central/rev/e9276a59e4a2

(Merged by Ed Morley)
Comment 23 Marco Zehe (:MarcoZ) 2012-06-09 07:50:14 PDT
Comment on attachment 630670 [details] [diff] [review]
Send empty text and selection when IME is disabled.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None.
User impact if declined: Blind users using the first accessible release of Fennec and using a virtual d-pad, like the one provided by the Eyes-Free keyboard, would experience extremely sluggish navigation through web content, or on many pages like the provided test case, no navigation capabilities at all. All touchscreen only devices without a directional controller are affectecd in both Gingerbread and Ice Cream Sandwich.
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): No risk, no regressions found, I tested a try server build for over a day before this landed on inbound and m-c.
String or UUID changes made by this patch: None.
Comment 24 Marco Zehe (:MarcoZ) 2012-06-09 07:54:26 PDT
Verified fixed in Nightly/Android 2012-06-09.
Comment 25 Eitan Isaacson [:eeejay] 2012-06-09 13:25:04 PDT
*** Bug 755573 has been marked as a duplicate of this bug. ***
Comment 26 Eitan Isaacson [:eeejay] 2012-06-11 15:00:26 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/f7e34a0e107d
Comment 27 Jim Chen [:jchen] [:darchons] 2012-10-24 13:00:42 PDT
Eitan: Can you update me on the steps to test this? I'm working on some keyboard changes on Android and I want to make sure I don't regress this. But I can't seem to turn on caret browsing at the moment.
Comment 28 Eitan Isaacson [:eeejay] 2012-10-24 13:07:23 PDT
(In reply to Jim Chen [:jchen :nchen] from comment #27)
> Eitan: Can you update me on the steps to test this? I'm working on some
> keyboard changes on Android and I want to make sure I don't regress this.
> But I can't seem to turn on caret browsing at the moment.

This is a tough bug to isolate. The best way to know that things are not regressing is testing Firefox accessibility with the eyes-free keyboard. If swiping left and right in the "navigation" mode moves the orange rectangle to the next page item, then it is working correctly.

Note You need to log in before you can comment on or make changes to this bug.