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)
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)
1.41 KB,
text/html
|
Details | |
1.63 KB,
patch
|
cpeterson
:
review+
cpeterson
:
feedback+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
OS: Linux → Android
Hardware: x86_64 → ARM
Updated•12 years ago
|
Severity: normal → major
Whiteboard: [Q2 a11y dependency]
Assignee | ||
Updated•12 years ago
|
Severity: major → normal
Comment 1•12 years ago
|
||
Eitan says he tested Nexus S (running CM9) and that the text NS_QUERY_TEXT_CONTENT returns seems wrong and arbitrary.
Assignee | ||
Comment 2•12 years ago
|
||
(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.
Assignee | ||
Comment 3•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
(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.
Assignee | ||
Comment 7•12 years ago
|
||
(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.
Assignee | ||
Updated•12 years ago
|
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
Assignee | ||
Comment 8•12 years ago
|
||
(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 :)
Updated•12 years ago
|
Assignee: nobody → cpeterson
Assignee | ||
Comment 9•12 years ago
|
||
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
Comment 10•12 years ago
|
||
(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?
Assignee | ||
Comment 11•12 years ago
|
||
(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).
Assignee | ||
Comment 12•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Attachment #630670 -
Flags: feedback?(cpeterson)
Comment 13•12 years ago
|
||
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+
Assignee | ||
Comment 14•12 years ago
|
||
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]
Comment 15•12 years ago
|
||
Eitan, did the try build complete? If so, what's the link?
Assignee | ||
Comment 16•12 years ago
|
||
(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•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [autoland-try:630670:-b do -p android,android-xul -u all -t none] [Q2 a11y dependency] → [Q2 a11y dependency]
Assignee | ||
Comment 18•12 years ago
|
||
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 19•12 years ago
|
||
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+
Assignee | ||
Comment 20•12 years ago
|
||
(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.
Assignee | ||
Comment 21•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/e9276a59e4a2
Assignee: cpeterson → eitan
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e9276a59e4a2 (Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 23•12 years ago
|
||
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?
Updated•12 years ago
|
Attachment #630670 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 26•12 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/f7e34a0e107d
Assignee | ||
Updated•12 years ago
|
status-firefox15:
--- → fixed
Comment 27•12 years ago
|
||
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.
Assignee | ||
Comment 28•12 years ago
|
||
(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.
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•