Closed
Bug 864582
Opened 11 years ago
Closed 10 years ago
Clean up _sendMouseEvents
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: Margaret, Assigned: capella)
References
Details
Attachments
(3 files, 1 obsolete file)
7.45 KB,
patch
|
Details | Diff | Splinter Review | |
4.59 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
4.99 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter 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.
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → margaret.leibovic
Reporter | ||
Comment 1•11 years ago
|
||
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.
Reporter | ||
Comment 2•11 years ago
|
||
Unassigning myself because I haven't been working on this.
Assignee: margaret.leibovic → nobody
Assignee | ||
Comment 3•11 years ago
|
||
/me adds it to the list of interesting thingz
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
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]
Reporter | ||
Comment 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: markcapella → nobody
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
This simply restyles the contents of the method for JS after previous patch removed an if() {} block.
Attachment #8413686 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 11•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Attachment #8413686 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 12•10 years ago
|
||
habitual push to TRY: https://tbpl.mozilla.org/?tree=Try&rev=4e781d232c6d
Assignee | ||
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/cf75dbf2383d https://hg.mozilla.org/integration/fx-team/rev/6815cc4eea57
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → markcapella
Reporter | ||
Comment 14•10 years ago
|
||
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.
Assignee | ||
Comment 15•10 years ago
|
||
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
Assignee | ||
Comment 16•10 years ago
|
||
Filed Bug 1002882 for the final piece.
Comment 17•10 years ago
|
||
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
Reporter | ||
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Comment 19•10 years ago
|
||
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
Reporter | ||
Updated•10 years ago
|
Summary: Use caretPositionFromPoint to move caret → Clean up _sendMouseEvents
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
•