Regression in Find-In-Page logic due to SelectionHandler change in bug 1011059

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: capella, Assigned: capella)

Tracking

unspecified
Firefox 33
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

I bit of innocuous code removed from SelectionHandler.js around bug 1011059 comment 18 causes an issue in combination with the find-in-page function ... noticeable on devices with a menu button. It seems there's an edge-case requiring us to pre-clear selection ranges in startSelection().

STR:

   Open any news article
      ex: http://www.politico.com/story/2014/06/2014-primary-night-takeaways-108274.html?hp=t1

   Long-tap a word such as "Richard" ...
      observe selection changes to orange, selection handles appear, actionbar opens

   Tap the menu button, tap Find-in-Page ...
      Observe find-bar opens with selected word prefilled
      Selection changes from orange to green, handles disappear.

   Long-tap a different word such as "scenario" below the green found text and observe:
      https://www.dropbox.com/s/us9egv3s35me8e7/regress1011059.png


We wind up with two selection ranges. SelectionHandler.js wraps handles around the first one, as the entire module assumes we either have selection.rangeCount == 0 or 1.
Seems like we should set the selection range if there is an existing range. But if we have to clear selection ranges, we should do that only when we don't have an editor. Editor doesn't like not having a selection.
I don't understand |Editor doesn't like not having a selection.|

Are you suggesting we're somehow losing the nsISelection object? All we're doing is clearing what ever ranges associated with that object have been set outside of SelectionHandler (which we control and track) by other code such as Find-In-Page.
Actually, even with replacing the original code and solving the regression in comment #0, I can see a further issue with similar STR as above, but where the initial find-in-page string is in an editable, and then we long-press a second word further down in the same editable (I used a textarea in bugzilla for my test).

In that case, the code |this._contentWindow.getSelection().removeAllRanges();| removes selected text ranges in the document (solving the original bug in comment #0) but not in the editable.

SelectionHandler wants to clear all selections on the displayed page at start. It tries to close anything it was responsible for selecting previously by the call to closeSelection().

The subsequent call to .removeAllRanges() is a catch-all for text selected by external processing, but it doesn't know if the selected text ranges were in an editor or not.
(In reply to Mark Capella [:capella] from comment #2)
> I don't understand |Editor doesn't like not having a selection.|
> 
> Are you suggesting we're somehow losing the nsISelection object? All we're
> doing is clearing what ever ranges associated with that object have been set
> outside of SelectionHandler (which we control and track) by other code such
> as Find-In-Page.

There are some code in editor that assumes a selection range always exists, i.e. the number of ranges is always > 0.
Do you mean like when there's no selection string, there's @ least 1 range where startNode == endNode and selection.collapsed == true ... (ie: blinking cursor in focused field) ...

I've also seen situations especially during construction where the nsISelection has 0 ranges (query an input field before it's focused).

My point is, this seems to be a surprising assumption, and I wonder what code you're referring to (core side? our/fennec side?) ... 

This isn't entirely academic ... we could fix the immediate issue reported in comment #0 by adding back the line that you removed but qualify it with
|if (!(aElement instanceof Ci.nsIDOMNSEditableElement)) { ... }|

But that leaves the case I mention in comment #3, where find-in-page has found text / created a selection range in an editable field, and the user tries to long-tap select a different word in the same editable.
Ping?
Flags: needinfo?(nchen)
So when an editor is focused, some editor code expect there to always be a selection (even if it's a collapsed selection in the case of a blinking cursor). e.g. nsHTMLEditor::SelectAll() expects this behavior [1].

I think this is a reasonable assumption to make. You wouldn't expect a focused editor to *not* have any seleciton or cursor present.

[1] dxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#3565
Flags: needinfo?(nchen)
Ok, we're talking past each other. This line was removed from TextSelection code. It's broken as a result in a way that I can understand. Putting it back fixes it.

I don't see how this was an issue with your original test failure. Can you please a) explain it further so I can fix it or, b) submit a correction so I can go away.
Sorry for being unclear. The patch in bug 1011059 comment 17 switches the "select all" code in text selection from using nsFrameSelection::SelectAll to using nsHTMLEditor::SelectAll, when focus is in an editor. This was necessary to fix the crash in bug 1011059.

However, nsHTMLEditor::SelectAll did not work out of the box because nsHTMLEditor::SelectAll depends on the selection having at least one range before the call. Because we cleared all ranges in text selection code, nsHTMLEditor::SelectAll failed. So the followup patch in bug 1011059 comment 18 removed the line to clear all ranges, to get nsHTMLEditor::SelectAll to work and to fix the crash.
That helped, thank you. This corrects the regression, and preserves your patch.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #8455597 - Flags: review?(margaret.leibovic)
Attachment #8455597 - Flags: feedback?(nchen)
Comment on attachment 8455597 [details] [diff] [review]
bug1030060.diff (v0)

Thanks! LGTM.
Attachment #8455597 - Flags: feedback?(nchen) → feedback+
Comment on attachment 8455597 [details] [diff] [review]
bug1030060.diff (v0)

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

Sounds reasonable to me as well!

Could you do me a favor and update the bug summary and commit message to be more specific? That would make it easier to tell what issue this bug is actually addressing without reading all the comments.
Attachment #8455597 - Flags: review?(margaret.leibovic) → review+
Summary: Regression in text SelectionHandler ... → Regression in Find-In-Page logic due to SelectionHandler change in bug 1011059
random rant ... Dang TRY IS SLOW !!!!

https://hg.mozilla.org/integration/fx-team/rev/451ee12473f7
https://hg.mozilla.org/mozilla-central/rev/451ee12473f7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
You need to log in before you can comment on or make changes to this bug.