Closed Bug 1064071 Opened 5 years ago Closed 5 years ago

Remove obsolete Text Selection caret positioning logic

Categories

(Firefox for Android :: Text Selection, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: capella, Assigned: cvielma, Mentored)

Details

(Whiteboard: [lang=js][lang=c++][good first bug])

Attachments

(1 file, 1 obsolete file)

In bug 927882 we introduced some code into nsWindow.cpp to maintain the UI text selection caret position in keyboard-specific situations (Google keyboard for example behaving differently than Swift keyboard).

Since then, we've completed bug 895463 which I think solves the problem in a more general way and obviates the need for the c++ code to exist.

This is a simple fix to remove code involved in generating and processing "TextSelection:UpdateCaretPos" messages, and requires basic knowledge of Javascript, and C++.

You should have an Android device, and be able to build the mobile version of Firefox to test your solution.
Hi! I'd like to git it a try to this bug. Is it ok to start working on it? 

I was checking SelectionHandler.js (http://hg.mozilla.org/mozilla-central/file/2255d7d187b2/mobile/android/chrome/content/SelectionHandler.js). This seems to be the only place where it's used. 

What I understand needs to be done is to remove the code in lines 106-109 (case statement), 711 (adding the observer), and 1009 (remove observer). Am I right or does it require additional changes?

Finally, how can I test this code change? What should be the expected behavior before and after? Do you usually create unit tests for these changes?

Thanks!
Flags: needinfo?(markcapella)
That's the gist of it, yes. Also, in addition to not listening for and responding to theses messages in SelectionHandler, we no longer need the code to originate them.

You can check MXR to find those bits. Or, you can cross-ref the original patch that implemented it. (We're not trying to undo that whole patch, just the code concerned with these messages.)

We don't have unit-tests specifically for this code, so you'll have to build and test locally on a device, and become familiar with the TextSelection process in general to be able to spot issues / incorrect UI display, etc.

I'm not anticipating any, but we'll need to dogfood for awhile.
Flags: needinfo?(markcapella)
Assignee: nobody → cvielma
Status: NEW → ASSIGNED
Thanks for the quick feedback. I'll get it into it and let you know if I get stuck.
ping status?
Hi Mark, I'll have an update by end of week. Sorry about the delay.
Hi Mark, I was expecting to work on this bug during this weekend, but didn't get the time. I'll try to find some time this week to get to work on this. I think the change is simple, but I'd like to have the time to understand the code better and learn how the current logic is implemented.
That's fine. We have an unfortunate abandon rate with new contributors, so I checked in. If you decide you can't help with this one let me know so I can free it back up? :-)
Hi Mark, I finally put some time to work on this. I removed the code from SelectionHandler and nsWindow.cpp (according to lxr). 

I also tested a few websites selecting text, copy/pasting and watching logcat. I didn't see any weird behavior in the UI or errors in the logs.

I still have to do some more study on how the whole text selection feature works through the code, but I think this fix correctly removes the old code.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8500042 - Flags: review?(markcapella)
Comment on attachment 8500042 [details] [diff] [review]
Patch

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

That's looking good. I tried to apply the patch but it fails with rejects. You'll have to update your local m-c repo and rebase this patch so it applies clean.

Repost the corrected patch with feedback? to me. I'll push it to our TRY-server for integration testing, and on success there, I'll flag it to margaret for final approval.

::: mobile/android/chrome/content/SelectionHandler.js
@@ -962,5 @@
>      this._removeObservers();
>  
>      // Only observed for caret positioning
>      if (this._activeType == this.TYPE_CURSOR) {
> -      Services.obs.removeObserver(this, "TextSelection:UpdateCaretPos");

I don't see where you removed the addObserver() counterpart to this line. Please do so.

::: widget/android/nsWindow.cpp
@@ +1822,1 @@
>          }

nit: Please also remove the blank line above between the DispatchEvent() and the following final `}`.

@@ +1905,1 @@
>          }

nit: Please also remove the blank line above between the DispatchEvent() and the following final `}`.
Attachment #8500042 - Flags: review?(markcapella) → feedback+
I'm also cc:ing jchen since he was involved with the nsWindow.pp changes when I put them in originally.
Hi Mark, I'll try to address the above comments this weekend. Just confirming with you: I'm currently syncing my code from Aurora (in a previous bug someone told me it was more stable to do the changes there). Should I sync instead from other branch?
You should be synching from mozilla-central. see bug 1064068 comment #13 through and where margaret agrees in bug 1064068 comment #15.
Attachment #8500042 - Attachment is obsolete: true
Attachment #8507409 - Flags: review?(markcapella)
Hi Mark, I did a hg pull from central and a rebase, and addressed the comments above. Let me know if there's anything else that needs to be done.
Comment on attachment 8507409 [details] [diff] [review]
bug-1064071-fix.patch

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

lgtm ! Let's see if the try-server has any complaints ...
https://tbpl.mozilla.org/?tree=Try&rev=241d32bebed9
Attachment #8507409 - Flags: review?(markcapella) → feedback+
Comment on attachment 8507409 [details] [diff] [review]
bug-1064071-fix.patch

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

Good try ... now r? to margaret for final approval
Attachment #8507409 - Flags: review?(margaret.leibovic)
Comment on attachment 8507409 [details] [diff] [review]
bug-1064071-fix.patch

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

Looks fine to me. I haven't tested this myself, but I trust capella's review :)
Attachment #8507409 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/1ab57d5a8e6a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Awesome! :)
You need to log in before you can comment on or make changes to this bug.