Closed Bug 921964 Opened 6 years ago Closed 5 years ago

[Keyboard][User Story] Cursor movement

Categories

(Core :: Selection, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bhuang, Assigned: pchang)

References

Details

(Whiteboard: [ucid:SystemPlatform25, 2.1, ft:system-platform])

Attachments

(2 files, 2 obsolete files)

User story:
As a user, I want to easily move the input cursor so that I can insert or delete text at any point.
blocking-b2g: --- → 1.3+
Whiteboard: [ucid:SystemPlatform25, ft:system-platform, 1.3:p1][NPOTB]
Depends on: 931495, 924692
No longer depends on: 931495
If we need a volunteer for this I can help.
This feature is moved to v1.4 due to the late technical decision to leverage layout engine.
blocking-b2g: 1.3+ → ---
Jan,

I appreciate your help and welcome to join the development on this cursor movement.
Assignee: nobody → janjongboom
please note that this patch also has bug 944681 (so the keyboard would not redraw all the time) and bug 943795 (to make it work in FF nightly) included.
Attached patch WIPSplinter Review
Here's a patch, it's maybe a bit messy but it works in apps and on websites. Here is the strategy:

1. I use SelectionHandler.js from FF for Android. We want to swap this out and put it in platform as discussed in bug 924692 so I think that's OK. I changed some minor things regarding positioning that I'll make separate bugs for in FF for Android section.
2. SelectionHandler communicates over 2 channels, first from browser.js to get events from the browser (tap etc.), and then talks to Java to tell where the caret should be positioned. Both these roles are done by SelectionHandler_glue.js, a frame script.
3. SelectionHandler & glue are loaded as frame scripts by Keyboard.jsm. Perhaps we need to move this into a separate module but I think it fits inputmethod.
4. Everything is implemented in gecko, works against latest gaia.

Here is a video of it in action: https://www.youtube.com/watch?v=-lPF2Hr_KWo

Some caveats:

* Reposition on scrolling only works in browser, not in apps (if you click again then it will show in right position though).
* If you have a textbox that scrolls that is fine but you need to wiggle your finger a bit to generate touchmove events to keep it scrolling.
* I create an element in the page that is visible in the DOM tree. It shouldn't.
* Only does cursor positioning. The android code has also different scenario's so it's just event routing for now.
* It looks ugly, but hey. :-)
Attachment #8340351 - Flags: feedback?(xyuan)
Another caveat:
* Sometimes the caret gets attached before the OS moves the caret actually and we end up being positioned in the wrong place. Maybe listen to inputcontext for changes rather than doing it ourselves.

Tested it on Keon and Peak by the way.
Update whiteboard tag to follow format [ucid:{id}, {release}:p{1,2}, ft:{team-id}]
Whiteboard: [ucid:SystemPlatform25, ft:system-platform, 1.3:p1][NPOTB] → [ucid:SystemPlatform25, 1.3:p1, ft:system-platform][NPOTB]
Comment on attachment 8340351 [details] [diff] [review]
WIP

I don't know where :yxl is these days, so asking kanru for some feedback.
Attachment #8340351 - Flags: feedback?(kchen)
Comment on attachment 8340351 [details] [diff] [review]
WIP

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

This approach looks good to me. About the injected DOM you probably could use anonymous DOM (XBL) to do that. SC has better understanding of this problem.
Attachment #8340351 - Flags: feedback?(schien)
Attachment #8340351 - Flags: feedback?(kchen)
Attachment #8340351 - Flags: feedback+
Update whiteboard to p2 since this one has been changed to targeted in v1.3
Whiteboard: [ucid:SystemPlatform25, 1.3:p1, ft:system-platform][NPOTB] → [ucid:SystemPlatform25, 1.3:p2, ft:system-platform][NPOTB]
Comment on attachment 8340351 [details] [diff] [review]
WIP

Attaboy! What a cool demo.
Let's ask Mark(markcapella@twcny.rr.com) to review the code. He knows quite a lot about SelectionHandler.js.

I'm learning SelectionHandler.js from scratch now and I expect to give any useful feedback later.
Attachment #8340351 - Flags: feedback?(markcapella)
Ah, FF OS, I knew we'd meet one day ;-)

I'm going to copy margaret since she implemented the original SelectionHandler... I'm just pulling slack in this area to make her life easier so she can focus on more pressing issues :)

I wish we didn't have to clone the object involved, we've done a lot of work in this area, most recently working through some <textarea> related issues, but more strikingly, wesj has changed the cut / copy / paste / etc. options from being contextmenu based, to actionbar style.

(Synching/sharing changes will have to be kept in mind.)

That's just my quick-glance ... more to come, as I work through, and become more familiar with Gaia.
It looks like you did a lot of work here already, but you may want to look into using metro's SelectionHandler, rather than Android's. It's already designed with e10s in mind, and depending on what you're doing for the UI for the handles, it may be easier to port over.

At one point I looked at combining the metro/Android text selection code into a utility in toolkit, but our architectures were just too different to make that work nicely. However, maybe if metro and b2g are similar enough, that could be a worthwhile strategy. It makes me worried to see so much code being forked, especially since this code is known for being buggy and high-maintenance.

I'm cc'ing Jim Mathies, since he worked on the metro text selection code.
Attachment #8340351 - Flags: feedback?(markcapella)
Whiteboard: [ucid:SystemPlatform25, 1.3:p2, ft:system-platform][NPOTB] → [ucid:SystemPlatform25, 1.4:p1, ft:system-platform][NPOTB]
Mark, Margaret, from what I understood from https://bugzilla.mozilla.org/show_bug.cgi?id=924692 is that the idea is that we would get rid of all SelectionHandlers and move it into platform. Do you think that makes sense?

The code in SelectionHandler.js is almost the same as Android now (some small things changed regarding positioning that I don't think would break Android). I really don't want to fork away. If bug 924692 is not going to get resolved in the next release we might need to think of moving this into toolkit so we have one codebase (whether it's Android & FxOS that use the same lib, or Metro & FxOS).
Flags: needinfo?(margaret.leibovic)
Alright, in that case I'll look into porting the code to Metro codebase until we land in platform.
I looked into the Metro code today but compared to Android code it's a lot less self contained. In Android there is some gesture forwarding from browser.js, but after that it's one unit of work. In Metro the SelectionHelperUI is referenced from 14 different files (from metro/base/content) and SelectionHandler in three files.

Are we sure that is the way we want to move forward? I'd figure a self contained plugin would make more sense because we can put it under a simple pref...
Flags: needinfo?(markcapella)
(In reply to Jan Jongboom [:janjongboom] from comment #17)
> I looked into the Metro code today but compared to Android code it's a lot
> less self contained. In Android there is some gesture forwarding from
> browser.js, but after that it's one unit of work. In Metro the
> SelectionHelperUI is referenced from 14 different files (from
> metro/base/content) and SelectionHandler in three files.
> 
> Are we sure that is the way we want to move forward? I'd figure a self
> contained plugin would make more sense because we can put it under a simple
> pref...

If you think that the Android SelectionHandler suits your needs better, then I think it's fine to use that. I just wanted to point out that there was also a Metro implementation, in case that was better.

I hadn't seen bug 924692 before, that's exciting! Hopefully all our products can move towards using that platform solution, rather than maintaining our own front-end text selection code.
Flags: needinfo?(margaret.leibovic)
Jan, sorry I just saw this bug.  What has changed about our plans to fix bug 924692 to make the platform capable of doing selections using touch?  I don't think that we want to inherit the Android implemnetation here.
Flags: needinfo?(janjongboom)
(Neither the metro implementation for that matter...)
Attachment #8340351 - Flags: feedback?(xyuan)
Nothing, but I want to know what the API is going to look like (more or less). In this patch I have some UI and the SelectionHandler from Android (this is *not* the way forward; nor did I change anything on that, I don't want to fork or use this), so we can have that ready before this code lands in platform.

I figured based on comment 15 that we would use a model similar to the Metro implementation when we implement this in platform, so I looked into that, and found that (at least from my point of view) a model more similar to Android seems more sane.

When bug 924692 lands, strip out SelectionHandler, hook up the glue to platform and done.
Flags: needinfo?(janjongboom) → needinfo?(ehsan)
OK, thanks, that sounds good!
Flags: needinfo?(ehsan)
Flags: needinfo?(markcapella)
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Comment on attachment 8340351 [details] [diff] [review]
WIP

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

The overall design should be using frame script to listen/update selection state and the position of caret handle is updated by selection state, so we can have a synchronized display for caret and caret handle.

::: dom/inputmethod/SelectionHandler_glue.js
@@ +308,5 @@
> +    this.init = function() {
> +      var e = self._el = doc.createElement('div');
> +      e.classList.add('caret');
> +      e.classList.add(handleType.toLowerCase());
> +      doc.body.appendChild(e);

I'm not sure if the div for caret handle should be seen by client side javascript/css. I think the "Handle" object implementation can be move into c++ and using the approach that @phoebe is trying to do in bug 924692 (i.e. anonymous content for caret handle).
Attachment #8340351 - Flags: feedback?(schien) → feedback+
No longer blocks: 1.3-system-platform
Drive-by comment: For Android, we originally implemented text selection with anonymous XBL handles in content, and we ran into a lot of performance issues related to reflow. In the old XUL version of Fennec (and Metro, which is based off that), I believe the handles are created in a separate process. This creates annoying implementation issues like keeping the handles aligned with the selection, but I think we had to do that for performance reasons. In any case, you should be sure to test on some big pages, and collect some profiles :)

For some history about Android text selection, you can check out bug 695173 and bug 767070.
Comment on attachment 8340351 [details] [diff] [review]
WIP

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

::: dom/inputmethod/SelectionHandler_glue.js
@@ +142,5 @@
> +let glue = new SelectionHandlerGlue();
> +glue.init();
> +
> +/**
> + * SelectionHandler.js requires this, I dont know what it does

Another drive-by: In Firefox for Android, BrowserApp is a JS object that controls most of the core browser object:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#247

BrowserApp.deck is the XUL <deck> that holds the XUL <browser> objects that correspond to the browsers in the tabs. So, this code adds/removes event listeners that will listen for all events that bubble up to that deck.
(In reply to :Margaret Leibovic from comment #24)
> For some history about Android text selection, you can check out bug 695173
> and bug 767070.

Thanks, that's helpful. It puts the reasons for implementing the handlers in C++ as in bug 924692 in perspective.
Jan,

Want to clarify if the WIP in this bug the same as it is in bug 924692. This is originally a user story bug for the feature of cursor movement. Instead of having implementation in this bug, I expect to have other corresponding engineering bugs linked to this.
Flags: needinfo?(janjongboom)
(In reply to Ivan Tsay (:ITsay) from comment #27)
> Jan,
> 
> Want to clarify if the WIP in this bug the same as it is in bug 924692. This
> is originally a user story bug for the feature of cursor movement. Instead
> of having implementation in this bug, I expect to have other corresponding
> engineering bugs linked to this.
Not really, this would be FxOS specific, the other bug would be platform-specific. We're just going back and forth to determine what goes where.
Flags: needinfo?(janjongboom) → needinfo?(itsay)
(In reply to Jan Jongboom [:janjongboom] from comment #28)
> (In reply to Ivan Tsay (:ITsay) from comment #27)
> > Jan,
> > 
> > Want to clarify if the WIP in this bug the same as it is in bug 924692. This
> > is originally a user story bug for the feature of cursor movement. Instead
> > of having implementation in this bug, I expect to have other corresponding
> > engineering bugs linked to this.
> Not really, this would be FxOS specific, the other bug would be
> platform-specific. We're just going back and forth to determine what goes
> where.

Ok, so it means eventually there will be the patch for this bug. We can still keep the way as it is now to have the implementation in this bug. But need your help to verify if the dependency bugs here are all correctly set for the cursor movement works.
Flags: needinfo?(itsay) → needinfo?(janjongboom)
I think bug 908493 could be removed as it's a UX requirement for this bug rather than a separate unit of work. Other than that 924692 + this bug would be enough. This bug in itself blocks all other bugs regarding text selections.
Flags: needinfo?(janjongboom)
Flags: needinfo?(itsay)
(In reply to Jan Jongboom [:janjongboom] from comment #30)
> I think bug 908493 could be removed as it's a UX requirement for this bug
> rather than a separate unit of work. Other than that 924692 + this bug would
> be enough. This bug in itself blocks all other bugs regarding text
> selections.

I will keep UX bug for the keyboard spec. and check with PM and UX to see if we want to move the spec. to keyboard meta bug. 

Plus this one as the committed feature in v1.4. Jan will use this bug as the engineering bug for the implementation.
blocking-b2g: --- → 1.4+
Flags: needinfo?(itsay)
Whiteboard: [ucid:SystemPlatform25, 1.4:p1, ft:system-platform][NPOTB] → [ucid:SystemPlatform25, 1.4:p1, ft:system-platform]
Unlink 908493 and need info Carrie Wang for the up-to-date spec. on cursor movement.
No longer depends on: 908493
Flags: needinfo?(cawang)
Depends on: 955987
Comment on attachment 8340351 [details] [diff] [review]
WIP

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

::: dom/inputmethod/SelectionHandler_glue.js
@@ +93,5 @@
> +		 * Make sure we only communicate the new position of the caret every tick
> +		 */
> +		function renderLoop() {
> +			var last;
> +			// Only process the last event, probably its accurate

I just tried it on B2G and found that there's no chance to call in here.
renderQueue.length is always 0.

@@ +107,5 @@
> +				renderQueue = [];
> +			}
> +
> +			contentWindow.requestAnimationFrame(function() {
> +				// on next frame, make sure position is correct

last is always empty here in my test.

@@ +130,5 @@
> +  
> +  /**
> +   * Caret handler calls this. It's still called content event because I did
> +   * this in system first
> +   */

Never being called.

@@ +193,5 @@
> +      
> +      // We are going to show something, so let's attach the window
> +      glue.attachContentWindow(element.ownerDocument.defaultView);
> +      
> +      element.ownerDocument.defaultView.setTimeout(function() {

I can see a green triangle being drawn with caret. It seems being done here.
Comment on attachment 8340351 [details] [diff] [review]
WIP

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

::: dom/inputmethod/SelectionHandler_glue.js
@@ +96,5 @@
> +			var last;
> +			// Only process the last event, probably its accurate
> +			if (renderQueue.length > 0) {
> +				last = renderQueue.pop();
> +				

I just tried it on B2G and found that there's no chance to call in here.
renderQueue.length is always 0.

@@ +108,5 @@
> +			}
> +
> +			contentWindow.requestAnimationFrame(function() {
> +				// on next frame, make sure position is correct
> +				if (last && last.name === 'TextSelection:Move') {

last is always empty here in my test.

@@ +135,5 @@
> +  this.receiveContentEvent = function(name, data) {
> +		dump('receiveContentEvent ' + JSON.stringify({ name: name, data: data }) + '\n');
> +
> +		renderQueue.push({ name: name, data: data });
> +  };

Never being called.

@@ +194,5 @@
> +      // We are going to show something, so let's attach the window
> +      glue.attachContentWindow(element.ownerDocument.defaultView);
> +      
> +      element.ownerDocument.defaultView.setTimeout(function() {
> +        SelectionHandler.attachCaret(element);

I can see a green triangle being drawn with caret. It seems being done here.
Forget Comment 33.
There's alignment issue.
Cool. When you drag it events should come in in the animation frame handler.
I was wondering how you decided to handle software synching thoughts between the Android Version and yours in |. In this latest patch I don't see changes we've released lately, and I know of at least another in the pipeline.

Also/fyi, I've spent some time reviewing Metro's SelectionHandler approach while working an a loosly-related Metro patch, and and will paying attention here as well   :-D
Sorry, (spelling), I was referencing yours in |dom/inputmethod/|.
The idea would be (see the depending bugs and the discussion in this issue) that we would move to one codebase for selection handling in 1.4.
Ok, I re-read the bug and reviewed the patch again. (Trying to catch up to the release schedule across FF/OS, Metro and Android projects.

I'm just wondering if there is a rough order of estimate for this specific patch to get into m-c. It looks currently like: |+/- weeks| is the factor, with FF/OS 1.4 as your bounding date.
I don't really know about the current schedule. Don't see much updates on this area. It'd be nice to land this stuff if we can't make the schedule. I have some copy/paste/selection experiments based on the Android code somewhere in a patch as well, but there was no UX.
Jan,

The target milestone of cursor movement work has been set Jan 31 since beginning and what we actually target is to check in the code in the Mozilla 29 m-c.

Regarding the text selection and copy/paste, we originally target FxOS v1.5 but it would be great if we can start it much earlier. I've attached the UX spec for the cursor movement, text selection, and copy paste in the bug. Please take a look and see if you have any questions. Also, let me know if you have any further question on the schedule.
Flags: needinfo?(cawang) → needinfo?(janjongboom)
Since the FxOS v1.4 development cycle will be very likely re-adjusted, what I mentioned in the comments 43 is based on the schedule we have originally. I don't think it makes much difference on the gecko work of cursor movement since we target it in gecko 29. Feel free to let me know if there is any question on the schedule.
Well, yeah, the original plan for this feature was that we needed bug 924692 first. If that doesn't happen in 1.4 we could land this patch without that changes.
Flags: needinfo?(janjongboom)
(In reply to Jan Jongboom [:janjongboom] from comment #45)
> Well, yeah, the original plan for this feature was that we needed bug 924692
> first. If that doesn't happen in 1.4 we could land this patch without that
> changes.

Jan,

Would you clarify it more for me that why this feature could still be completed without 924692? Per sync-up with Phoebe last week, it seems that the cursor won't be painted in the correct position after its movement without bug 924692.
Flags: needinfo?(janjongboom)
Because the patch in this state (in this bug) doesn't use the work from 924692.
Flags: needinfo?(janjongboom)
Comment on attachment 8360505 [details]
[SPEC] keyboard text selection v2.2.pdf

New spec. has been provided, this one is no longer valid
Attachment #8360505 - Attachment is obsolete: true
Duplicate of this bug: 968485
Feature is not the blocker for the release. Reset blocking-b2g flag.
blocking-b2g: 1.4+ → ---
Plan to land code in v1.4 sprint 2 and start QA test in v1.4 sprint 3. This feature work is on bugs 924692 and 955987. Will not use the WIP attached in this bug.
Whiteboard: [ucid:SystemPlatform25, 1.4:p1, ft:system-platform] → [ucid:SystemPlatform25, 1.4, ft:system-platform]
Target Milestone: 1.3 C3/1.4 S3(31jan) → 1.4 S2 (28feb)
Assign user story to Steven Lee (cursor movement feature engineer lead) for tracking on any engineering bug update/dependency
Assignee: janjongboom → slee
William,

Set in-moztrap flag as the reminder for you to create the test cases for this feature. Please check with Steven Lee if you have any question on testing this feature.
Flags: in-moztrap?(whsu)
Thanks Ivan.
No problem. :)
The spec is updated to v0.2. Please check the attachment. Thanks!
Hi, Ivan and Carrie,

Thanks for your help!
Here are the test cases I created.
- http://goo.gl/1KY04d

I will host a meeting to review test cases later.
Flags: in-moztrap?(whsu) → in-moztrap+
Target Milestone: 1.4 S2 (28feb) → 1.4 S4 (28mar)
Blocks: 2.0-system-platform
No longer blocks: 1.4-system-platform
Whiteboard: [ucid:SystemPlatform25, 1.4, ft:system-platform] → [ucid:SystemPlatform25, 1.5, ft:system-platform]
Whiteboard: [ucid:SystemPlatform25, 1.5, ft:system-platform] → [ucid:SystemPlatform25, 2.0, ft:system-platform]
Target Milestone: 1.4 S4 (28mar) → 2.0 S2 (23may)
feature-b2g: --- → 2.0
Need functional test, move to 2.1
feature-b2g: 2.0 → 2.1
Target Milestone: 2.0 S2 (23may) → ---
Depends on: 1021527, 1021499
Depends on: 1021530
Depends on: 1016184
Blocks: 1029465
No longer blocks: 2.0-system-platform
Whiteboard: [ucid:SystemPlatform25, 2.0, ft:system-platform] → [ucid:SystemPlatform25, 2.1, ft:system-platform]
QA Contact: edchen
Assignee: slee → pchang
feature-b2g: 2.1 → ---
Blocks: 1056355
Duplicate of this bug: 1074796
Change the component as most of the dependent work is in Core::Selection.
Component: Gaia::Keyboard → Selection
Product: Firefox OS → Core
The task is done, please refer to the meta bug 1023688
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
No longer depends on: 1021499
You need to log in before you can comment on or make changes to this bug.