Closed Bug 864582 Opened 11 years ago Closed 10 years ago

Clean up _sendMouseEvents

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: Margaret, Assigned: capella)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached patch WIPSplinter Review
This bug is split off from bug 667243.

We're not always getting the offset values that we want from caretPositionFromPoint, and this may be because we need to do a better job restricting the coordinates of the points we pass in.

The caret also becomes laggy after a bit, which may be because of IME events, but at the moment this patch isn't performant enough.
Assignee: nobody → margaret.leibovic
Depends on: 865654
Capella, this bug would be a nice win if we can find a way to finish it. I haven't been paying too much attention to the caretPositionFromPoint improvments, but if you're interested in working on this, you could try applying my old patch to see how well things work now.

Basically, after bug 667243, caret positioning is the only place we still use _sendMouseEvents, and I would love to see that function go away forever.
Unassigning myself because I haven't been working on this.
Assignee: margaret.leibovic → nobody
/me adds it to the list of interesting thingz
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch bug864582 (obsolete) — Splinter Review
The patch re-based into my queue ok ... I'm playing with sliding the caret around / back and forth quickly in nightly and in v24, and I think nightly on my GS3 does a slightly better job of keeping up.

In both versions the cursor and the caret position stay pretty tightly coupled. In both versions with practice I can get my touch position to lead or even seem to counter-balance the cursor/caret position.

However, in only the beta version can I observer cursor/caret movements that were queued and completed well after I released my touch.

This is all first glance on my part, maybe you can comment a little more on what you saw when you wrote and first tested, and what kind of warning signs I should be trying to observe ?
Attachment #795183 - Flags: feedback?(margaret.leibovic)
fyi- I ran some quick timing tests ...

Using the current routine _sendMouseEvents() I was seeing average times of:

TIME AVERAGE [41.4345703125]
TIME AVERAGE [26.93359375]
TIME AVERAGE [21.9375]
TIME AVERAGE [29.03125]
TIME AVERAGE [14.598838806152344]
TIME AVERAGE [14.68804931640625]
TIME AVERAGE [26.464599609375]
TIME AVERAGE [21.65087890625]
TIME AVERAGE [14.0546875]
TIME AVERAGE [20.0546875]
TIME AVERAGE [28.1004638671875]
TIME AVERAGE [16.79248046875]
TIME AVERAGE [18.484619140625]
TIME AVERAGE [24.258544921875]
TIME AVERAGE [29.751953125]
TIME AVERAGE [21.72945673018694]
TIME AVERAGE [19.750027298927307]
TIME AVERAGE [17.883434295654297]
TIME AVERAGE [16.61012077331543]
TIME AVERAGE [16.3565673828125]
TIME AVERAGE [22.61541748046875]
TIME AVERAGE [17.021339416503906]
TIME AVERAGE [24.832319259643555]
TIME AVERAGE [18.38307762145996]
TIME AVERAGE [18.11138916015625]
TIME AVERAGE [17.859130859375]
TIME AVERAGE [13.679358154498095]

Using the new routine _moveCaret() I was seeing average times of:

TIME AVERAGE [18.39453125]
TIME AVERAGE [9.515625]
TIME AVERAGE [13.99609375]
TIME AVERAGE [12.98931884765625]
TIME AVERAGE [9.401748657226562]
TIME AVERAGE [13.8466796875]
TIME AVERAGE [10.594734191894531]
TIME AVERAGE [11.029296875]
TIME AVERAGE [11.8096923828125]
TIME AVERAGE [11.639052960241695]
TIME AVERAGE [11.021484375]
TIME AVERAGE [10.483529718246778]
TIME AVERAGE [9.657065676066047]
TIME AVERAGE [8.457150292466707]
TIME AVERAGE [12.669921875]
TIME AVERAGE [9.162109375]
TIME AVERAGE [9.45908260345459]
TIME AVERAGE [10.210399150848389]
TIME AVERAGE [7.905381083488464]
TIME AVERAGE [7.991142272949219]
TIME AVERAGE [11.594528198242188]
TIME AVERAGE [8.192784958440576]
TIME AVERAGE [8.479339599609375]
TIME AVERAGE [7.27685546875]
TIME AVERAGE [11.028778076171875]
TIME AVERAGE [9.183982849121094]
TIME AVERAGE [13.27001953125]
TIME AVERAGE [9.976781227936463]
Comment on attachment 795183 [details] [diff] [review]
bug864582

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

Sorry for taking so long to get around to this... I still haven't had the chance to apply this and try it out (I'm on a plane right now, which makes getting wifi on two devices more annoying), but it sounds like it's doing better than it was when I worked with this patch before. I remember seeing really wildly wrong caret positions, which is why I didn't proceed further with my patch. You should make sure to test this out in both regular inputs and textareas, as well as those in frames. I made a test page that's handy for this:
http://people.mozilla.com/~mleibovic/test/text_select.html

If this patch works well in all the editable elements in that page, I'd say we should go ahead and land it!

Also flagging bnicholson for review, since I basically wrote this patch, so I'm a biased reviewer :)
Attachment #795183 - Flags: feedback?(margaret.leibovic)
Attachment #795183 - Flags: feedback?(bnicholson)
Attachment #795183 - Flags: feedback+
So your test page provides 4 interesting test cases where we can examine input-field cursor-caret positioning and movement.

fyi, I didn't do any actual code timings here like in testing re: comment #5 as performance issues were obvious.

In the simplest / first <input> case, both versions seem to behave correctly, but the new version performs significantly better in terms of tracking touch position while moving. The original version lags behind, as I noticed above, and can be easily observed moving / trying to process events after you've lifted your finger.

In the second <textarea> case, the newer version still performs better but does exhibit some odd positioning / flickering issues when the caret is dragged past the bottom right of the text. I'll look at that in some more detail.

For the third case, iframe input.html, both versions of the code have very poor ability to tap into the field. When zoomed in, the new version taps the caret into position and moving it around functions properly. The original version takes forever to respond into the tap if at all, and subsequent attempts at moving the caret seem to force it to stick to the beginning / left side of the field.

For the final case, iframe_text.html, each versions' results are consistent with their earlier behaviors. The newer version taps into position fast / correctly and moves around but gets confused / jumps / flickers when going to the bottom right of the text. The current version lags far behind the processing, and again seems to want to stick to the beginning / top of the field.

Overall they both have issues, but the new version is much more responsive, and if the bottom-right flicker can be fixed, will be clearly an improvement over what I'm seeing from release v23. Maybe you can try this out and verify my observations while I look in that direction (?).
Comment on attachment 795183 [details] [diff] [review]
bug864582

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

This looks good at first glance and it's nice to finally get rid of these mouse events. It's a bit difficult to see the changes the patch made though because of the whitespace changes you made. Do you think you could submit this as two separate patches, one just for your whitespace fixes, and the other with the actual bug fixes?
Attachment #795183 - Flags: feedback?(bnicholson) → feedback+
Assignee: markcapella → nobody
Looked back at this one, and thought we could sneak up on it a little ... this renames the _sendMouseEvents method and factors out some unused bits in the method.
Attachment #795183 - Attachment is obsolete: true
Attachment #8413685 - Flags: review?(margaret.leibovic)
This simply restyles the contents of the method for JS after previous patch removed an if() {} block.
Attachment #8413686 - Flags: review?(margaret.leibovic)
Comment on attachment 8413685 [details] [diff] [review]
bug864582p0.diff rebrand _sendMouseEvents

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

Thanks for breaking these into smaller changes. Much easier to review!
Attachment #8413685 - Flags: review?(margaret.leibovic) → review+
Attachment #8413686 - Flags: review?(margaret.leibovic) → review+
Assignee: nobody → markcapella
Since there isn't a patch here yet to get rid of the fake mouse events, I think we should re-summarize this bug and file a follow-up for the actual caretPositionFromPoint change. I find it easier to follow bug activity and history if there's just one bug per patch landing.
Ok, I can do that. (I forgot to mark this [leave open] in any case ;)

I think the only remaining piece was to replace the sendMouseEventToWindow() events with the two lines from the originaly patch(s)

  this._targetElement.selectionStart = this._targetElement.selectionEnd = caretPos.offset;
  this._positionHandles();

But I haven't re-done that yet ... maybe tomorrow
Filed Bug 1002882 for the final piece.
https://hg.mozilla.org/mozilla-central/rev/6815cc4eea57
https://hg.mozilla.org/mozilla-central/rev/cf75dbf2383d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
(In reply to Mark Capella [:capella] from comment #16)
> Filed Bug 1002882 for the final piece.

Thanks. I just remember last time the reason this patch didn't land was because there were problems just swaping in caretPositionFromPoint, so it may take a bit of work before we can land that.
Yep :( for example, I'm currently tracking an issue causing return of wrong offset when moving cursor to right/bottom in textarea = returns 0 ...

I've got a hack I can use but *ick*

Gonna keep playing for a bit
Summary: Use caretPositionFromPoint to move caret → Clean up _sendMouseEvents
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.