Closed
Bug 1061944
Opened 10 years ago
Closed 10 years ago
Clicking link loses text selection on Android before content script click listener runs
Categories
(Firefox for Android Graveyard :: Text Selection, defect)
Firefox for Android Graveyard
Text Selection
Tracking
(firefox35 verified, firefox36 verified, firefox37 verified)
VERIFIED
FIXED
Firefox 35
People
(Reporter: anaran, Assigned: manu.jain13, Mentored)
References
()
Details
(Whiteboard: [lang=js][good first bug])
Attachments
(1 file, 5 obsolete files)
4.02 KB,
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:34.0) Gecko/20100101 Firefox/34.0 Build ID: 20140902030202 Steps to reproduce: See instructions in https://github.com/anaran/IssuePigeonFirefox/issues/3 I'm eager to get to the bottom of this.
Reporter | ||
Comment 1•10 years ago
|
||
Apparently there's more than 40 text select bugs on Android: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=product%3Afirefox%20for%20android%20summary%3Atext%20select
Reporter | ||
Updated•10 years ago
|
Product: Add-on SDK → Firefox for Android
Reporter | ||
Comment 2•10 years ago
|
||
Here's a sweet little test case: data:text/html;charset=utf-8,<div>selection lost on android on button click</div><input value="Use Selection" type="button"></input> Selection remains on Windows (good) but is lost on button click on Android.
Comment 3•10 years ago
|
||
Mark, is this mentorable?
Comment 4•10 years ago
|
||
That's an interesting question. The bit of code that's responsible for the issue is: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=070491691b4e&mark=111-122#90 This I believe was in place even before :margaret ported it over from browser, and I subsequently got involved. iirc, it was in place early on when grcko-to-Android SelectionHandler code was rough, and we used it as a catch all for clearing the new left/right handles (Java side) off the screen when the gecko selection was lost, or we became unsure of the actual UI state. We've made a lot of improvements since then, notably the addition of our SelectionListener logic, and we may not actually need this anymore. Mentoring someone to remove a block of code is easy of course. We'd need to look at regression bugs in that area flagged by |blame|, bug 970054, bug 906499, and bug 907271 to see if they complain after we remove the code, and we'll most likely need to update our testSelectionHandler robocop test code.
Flags: needinfo?(markcapella)
Updated•10 years ago
|
Component: General → Text Selection
Updated•10 years ago
|
Mentor: markcapella
Whiteboard: [lang=js][good first bug]
Hi, I'm interested in patching this bug. Can you assign this to me. I've recently solved "https://bugzilla.mozilla.org/show_bug.cgi?id=1060423" under your guidance.
Comment 6•10 years ago
|
||
Good show! This patch will be a little less cut and dried than the last one, where we had a provably wrong issue, with no possible side effects as a result of the change. To fix this reported problem, we're going to remove old code that I see no current use for. So we need to: a) prove that removing the code fixes this specific bug. b) prove that removing the code doesn't regress/break anything else. The last one will be a little harder, and will involve re-creating / re-testing the bugs I mentioned in comment #4 above, and dog-fooding the changes on your (and my) test devices for a while looking for problems. I don't think there will be any, but proving a negative takes thought. Finally, when we do push your code change to TRY as we did in the final stages of your first bug, I believe you'll see us fail a test. http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/testSelectionHandler.html?rev=c474691b9c0f&force=1&mark=180-185#158 Fixing this is easy, we just remove that test, and add it's changes to your patch before final approval. Basically, this is the problem we're trying to solve as I understand it: https://www.dropbox.com/s/sh9wvat854osube/test_bug1061944.xhtml?dl=0 If you launch that page on Android and longtap some text to "select" it, then click the button, the text that you selected should appear between the brackets. (This works on Firefox desktop, but *not* here in Android land.) With me so far ? :-D
Assignee: nobody → manu.jain13
Status: NEW → ASSIGNED
I think this patch is working fine. I've tested this on my firefox. Thanks!!
Attachment #8485896 -
Flags: review?(markcapella)
Comment 8•10 years ago
|
||
Comment on attachment 8485896 [details] [diff] [review] bug1061944.patch Review of attachment 8485896 [details] [diff] [review]: ----------------------------------------------------------------- This looks right on first glance ... make sure to read and test the situations in comment #4 And the testcase: https://www.dropbox.com/s/im1q3x0yhj6kd3k/test_bug1061944.html?dl=0 I'll try this out myself and give it a TRY-server push. Take a look here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/testSelectionHandler.html?rev=c474691b9c0f&force=1&mark=180-185#158 I fully expect that test to break, and require you to add a correction to your patch. btw - you can perform robocop tests on your device to check this ahead of time. See: https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop This is a little tricky, but worth learning.
Attachment #8485896 -
Flags: review?(markcapella) → feedback+
Comment 9•10 years ago
|
||
push to TRYserver https://tbpl.mozilla.org/?tree=Try&rev=8e35821d65f1 Note the orange (failed) rc5 test for example ... this is the expected failure I mentioned ... clicking through into there shows the error https://tbpl.mozilla.org/php/getParsedLog.php?id=47651255&tree=Try&full=1#error0 So now, you'll need to review this, find the appropriate test code and fix it (remove the Gesture:SingleTap specific stuff), and re-post your patch with all the changes.
Assignee | ||
Comment 10•10 years ago
|
||
I have removed the test case.
Attachment #8485896 -
Attachment is obsolete: true
Attachment #8486193 -
Flags: review?(markcapella)
Comment 11•10 years ago
|
||
Comment on attachment 8486193 [details] [diff] [review] bug_1061944.patch Review of attachment 8486193 [details] [diff] [review]: ----------------------------------------------------------------- That looks pretty close but I'd like to clean it up a little more. See the below comments, and please briefly answer: 1) Have you managed to run this Robocop test locally to be sure it works? 2) Have you tried any of the tests from the other bugs I mentioned? 3) Have you at least run the testcase I've included in comment #8? I'm assuming you looked at these things, but you haven't really described your results yet. ::: mobile/android/base/tests/roboextender/testSelectionHandler.html @@ +170,4 @@ > // Check the initial state of the selection handler. > return Promise.all([ > ok(!sh.isSelectionActive(), "Selection should not be active at start of testCloseSelection"), > The next four lines don't really seem needed anymore. Let's remove them and have the first test start after // Various other ways to close an active selection. @@ +179,1 @@ > // Various other ways to close an active selection. And fix-up the comment to reflect how the test works now, maybe change // Various other ways to close an active selection. to be // Various ways to close an active selection. @@ +179,2 @@ > // Various other ways to close an active selection. > }).then(function() { Note that after you remove the four lines above that start with ]).then(function() { then this line right before here will need to be updated ...
Attachment #8486193 -
Flags: review?(markcapella) → feedback+
Assignee | ||
Comment 12•10 years ago
|
||
Sorry for the late reply mark. Actually I was trying to install robocop but was not successful. It was saying "unknown mach command: robocop". But I've tried from the other bugs you mentioned in comment #4 and also I've run the testcase you've given in comment #8. They all are working normally i.e there is no failure in those tests. Also I've made the changes as you told in comment #11. I'll surely try to install robocop though. Please review the new patch. Thanks!!
Attachment #8486193 -
Attachment is obsolete: true
Attachment #8488829 -
Flags: review?(markcapella)
Comment 13•10 years ago
|
||
Comment on attachment 8488829 [details] [diff] [review] bug_1061944_.patch Review of attachment 8488829 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to see you able to run the robocop tests, It's unfortunate that the |mach| command is changing as we speak, but we're in no hurry with this patch. Can you take some time to learn this? You can ask in #mobile for help if I'm not around or wait until you can catch me. fwiw, We're almost done :-) Your patch looks correct. ::: mobile/android/base/tests/roboextender/testSelectionHandler.html @@ +173,1 @@ > nit: The line above here (#173) has a trailing white-space that needs to be removed. @@ +175,1 @@ > }).then(function() { I'd like you to be able to run robocop tests to catch javascript errors locally, without having to push to the TRY-server. In this case, I think you'll find a syntax error on the line above here.
Attachment #8488829 -
Flags: review?(markcapella) → feedback+
Assignee | ||
Comment 14•10 years ago
|
||
Here's the new patch after robocop testing.
Attachment #8488829 -
Attachment is obsolete: true
Attachment #8490540 -
Flags: review?(markcapella)
Attachment #8490540 -
Flags: review?(markcapella) → review?(mark.finkle)
Comment 15•10 years ago
|
||
try-server push https://tbpl.mozilla.org/?tree=Try&rev=d8cc0bf6cc66
Assignee | ||
Comment 16•10 years ago
|
||
It looks like all the tests passed successfully. :)
Comment 17•10 years ago
|
||
Comment on attachment 8490540 [details] [diff] [review] bug1061944_v4.patch I need to pass this review. I am too out of touch with the selection code to provide good feedback.
Attachment #8490540 -
Flags: review?(mark.finkle)
Attachment #8490540 -
Flags: review?(margaret.leibovic)
Attachment #8490540 -
Flags: feedback?(markcapella)
Comment 18•10 years ago
|
||
Comment on attachment 8490540 [details] [diff] [review] bug1061944_v4.patch Review of attachment 8490540 [details] [diff] [review]: ----------------------------------------------------------------- This patch brings us in compliance with desktop behaviour, and better conforms to spec http://w3c.github.io/selection-api/#user-interactions starting here: "The user agent must not make a selection empty if it was not already empty in response to any user actions (e.g. clicking on a non-editable region)." ... The Java "Gesture:SingleTap" message does not directly interact with the DOM and modifying selection logic in response I believe is incorrect. UI interactions that previously depended on this clean-up code have been tested and still work properly. The provided testcase also works properly now.
Attachment #8490540 -
Flags: feedback?(markcapella) → feedback+
Comment 19•10 years ago
|
||
Comment on attachment 8490540 [details] [diff] [review] bug1061944_v4.patch Review of attachment 8490540 [details] [diff] [review]: ----------------------------------------------------------------- It looks like capella did a good job reviewing this (thanks!). Given the discussion in this bug, this sounds like a reasonable fix to me. ::: mobile/android/chrome/content/SelectionHandler.js @@ -110,5 @@ > > case "Gesture:SingleTap": { > - if (this._activeType == this.TYPE_SELECTION) { > - let data = JSON.parse(aData); > - if (!this._pointInSelection(data.x, data.y)) You can also remove the _pointInSelection function, since this is the only place it's used.
Attachment #8490540 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 20•10 years ago
|
||
I've removed the function _pointInSelection.
Attachment #8490540 -
Attachment is obsolete: true
Attachment #8493368 -
Flags: review?(markcapella)
Comment 21•10 years ago
|
||
Comment on attachment 8493368 [details] [diff] [review] bug_1061944_v5.patch Review of attachment 8493368 [details] [diff] [review]: ----------------------------------------------------------------- nice catch and correction ! manu, can you fix your commit message from |r=markcapella| to |r=margaret| and re-post this? We'll carry over her approval so you don't need to flag me for r? this last time. (If you'd prefer, I can fix that bit up and get you pushed)
Attachment #8493368 -
Flags: review?(markcapella)
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8493368 [details] [diff] [review] bug_1061944_v5.patch ># HG changeset patch ># Parent f4037194394efcad03d5076860387bcf32ff98a0 ># User Manu Jain <manu.jain13@gmail.com> >Bug 1061944 - Clicking link loses text selection on Android before content script click listener runs; r=margaret > >diff --git a/mobile/android/base/tests/roboextender/testSelectionHandler.html b/mobile/android/base/tests/roboextender/testSelectionHandler.html >--- a/mobile/android/base/tests/roboextender/testSelectionHandler.html >+++ b/mobile/android/base/tests/roboextender/testSelectionHandler.html >@@ -189,31 +189,18 @@ function testCloseSelection() { > var sh = getSelectionHandler(); > var inputNode = document.getElementById("inputNode"); > inputNode.value = INPUT_TEXT; > > // Check the initial state of the selection handler. > return Promise.all([ > ok(!sh.isSelectionActive(), "Selection should not be active at start of testCloseSelection"), > >+ // Various ways to close an active selection. > ]).then(function() { >- // Start by selecting all in an <input>. >- sh.startSelection(inputNode); >- return is(sh._activeType, sh.TYPE_SELECTION, "Selection should be active in <input> before Gesture:SingleTap"); >- >- }).then(function() { >- // Tap outside <input> to close active selection. >- sh.observe(null, "Gesture:SingleTap", JSON.stringify({ >- x: 1, >- y: 1 >- })); >- return ok(!sh.isSelectionActive(), "Gesture:SingleTap outside <input> should close active selection"); >- >- // Various other ways to close an active selection. >- }).then(function() { > sh.startSelection(inputNode); > sh.observe(null, "TextSelection:End", {}); > return ok(!sh.isSelectionActive(), "TextSelection:End should close active selection"); > > }).then(function() { > sh.startSelection(inputNode); > sh.observe(null, "Tab:Selected", {}); > return ok(!sh.isSelectionActive(), "Tab:Selected should close active selection"); >diff --git a/mobile/android/chrome/content/SelectionHandler.js b/mobile/android/chrome/content/SelectionHandler.js >--- a/mobile/android/chrome/content/SelectionHandler.js >+++ b/mobile/android/chrome/content/SelectionHandler.js >@@ -104,21 +104,17 @@ var SelectionHandler = { > > // Update caret position on keyboard activity > case "TextSelection:UpdateCaretPos": > // Generated by IME close, autoCorrection / styling > this._positionHandles(); > break; > > case "Gesture:SingleTap": { >- if (this._activeType == this.TYPE_SELECTION) { >- let data = JSON.parse(aData); >- if (!this._pointInSelection(data.x, data.y)) >- this._closeSelection(); >- } else if (this._activeType == this.TYPE_CURSOR) { >+ if (this._activeType == this.TYPE_CURSOR) { > // attachCaret() is called in the "Gesture:SingleTap" handler in BrowserEventHandler > // We're guaranteed to call this first, because this observer was added last > this._deactivate(); > } > break; > } > case "Tab:Selected": > case "TextSelection:End": >@@ -1038,26 +1034,16 @@ var SelectionHandler = { > offset.y += rect.top; > > win = win.parent; > } > > return offset; > }, > >- _pointInSelection: function sh_pointInSelection(aX, aY) { >- let offset = this._getViewOffset(); >- let rangeRect = this._getSelection().getRangeAt(0).getBoundingClientRect(); >- let radius = ElementTouchHelper.getTouchRadius(); >- return (aX - offset.x > rangeRect.left - radius.left && >- aX - offset.x < rangeRect.right + radius.right && >- aY - offset.y > rangeRect.top - radius.top && >- aY - offset.y < rangeRect.bottom + radius.bottom); >- }, >- > // Returns true if the selection has been reversed. Takes optional aIsStartHandle > // param to decide whether the selection has been reversed. > _updateCacheForSelection: function sh_updateCacheForSelection(aIsStartHandle) { > let rects = this._getSelection().getRangeAt(0).getClientRects(); > if (!rects[0]) { > // nsISelection object exists, but there's nothing actually selected > throw "Failed to update cache for invalid selection"; > }
Attachment #8493368 -
Flags: review?(margaret.leibovic)
Comment 23•10 years ago
|
||
manu, just attach the patch like you did previously.
Assignee | ||
Comment 24•10 years ago
|
||
Review changed to margaret.
Attachment #8493368 -
Attachment is obsolete: true
Attachment #8493368 -
Flags: review?(margaret.leibovic)
Attachment #8493376 -
Flags: review?(margaret.leibovic)
Comment 25•10 years ago
|
||
Comment on attachment 8493376 [details] [diff] [review] bug_1061944_v5.patch Review of attachment 8493376 [details] [diff] [review]: ----------------------------------------------------------------- We'll carry over her r+ from before, and I'll get this moved for you.
Attachment #8493376 -
Flags: review?(margaret.leibovic)
Updated•10 years ago
|
Keywords: testcase → checkin-needed
Comment 26•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/654e7e288711
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/654e7e288711
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → Firefox 35
Reporter | ||
Comment 28•10 years ago
|
||
(In reply to adrian.aichner from comment #2) > Here's a sweet little test case: > > data:text/html;charset=utf-8,<div>selection lost on android on button > click</div><input value="Use Selection" type="button"></input> > > Selection remains on Windows (good) but is lost on button click on Android. I can confirm this issue fixed in "35.0a1 (2014-09-24)", great! But there is major new breakage: Bug 1072276
Comment 29•10 years ago
|
||
Selection remains on button click, verified as fixed on latest builds: Firefox for Android 37.0a1 (2014-12-09) Firefox for Android 36.0a2 (2014-12-09) Firefox for Android 35.b1 Device: Nexus 4 (Android 4.4.4)
Status: RESOLVED → VERIFIED
status-firefox35:
--- → verified
status-firefox36:
--- → verified
status-firefox37:
--- → verified
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
•