Closed Bug 755976 Opened 12 years ago Closed 12 years ago

Virtual D-Pad does not send key press events when caret browsing is enabled

Categories

(Firefox for Android Graveyard :: Keyboards and IME, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox15 fixed)

VERIFIED FIXED
Firefox 15
Tracking Status
firefox15 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

(Whiteboard: [Q2 a11y dependency])

Attachments

(2 files)

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.
OS: Linux → Android
Hardware: x86_64 → ARM
Blocks: 755573
Severity: normal → major
Whiteboard: [Q2 a11y dependency]
Severity: major → normal
Eitan says he tested Nexus S (running CM9) and that the text NS_QUERY_TEXT_CONTENT returns seems wrong and arbitrary.
(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.
Attached file 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).
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.
(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.
(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.
(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.
Summary: Virtual D-Pad changes behaviour when selection is set in the document → Virtual D-Pad does not send key press events when caret browsing is enabled
(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 :)
Assignee: nobody → cpeterson
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.
Target Milestone: --- → Firefox 15
(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?
(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).
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.
Attachment #630670 - Flags: feedback?(cpeterson)
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. :)
Attachment #630670 - Flags: feedback?(cpeterson) → feedback+
Putting this in try before asking for review. I would love to see this approved for aurora too.
Whiteboard: [Q2 a11y dependency] → [autoland-try:630670:-b do -p android,android-xul -u all -t none] [Q2 a11y dependency]
Eitan, did the try build complete? If so, what's the link?
(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.
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.
Whiteboard: [autoland-try:630670:-b do -p android,android-xul -u all -t none] [Q2 a11y dependency] → [Q2 a11y dependency]
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.
Attachment #630670 - Flags: review?(cpeterson)
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.
Attachment #630670 - Flags: review?(cpeterson) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/e9276a59e4a2

(Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Attachment #630670 - Flags: approval-mozilla-aurora?
Verified fixed in Nightly/Android 2012-06-09.
Status: RESOLVED → VERIFIED
Attachment #630670 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
(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.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.