854605 - Use selectAtPoint to start a selection

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[:Margaret Leibovic]

Attachment: (Part 1) Rename SelectionHandler properties to better match metro

I'm going to try to work on bug 667243 in smaller bugs. This one just gets rid of the mouse event hack in startSelection.

I started by doing some more refactoring to get this to look more like metro's SelectionHandler, so that we can follow the same logic (and hopefully eventually share some utility functions).

This first patch does some renaming, but it also keeps _targetElement and _contentWindow as separate things, instead of the way we were just setting _target = _view in the selection case, which avoids some confusion.

Comment 1

[:Margaret Leibovic]

Attachment: (Part 2) Make private SelectionHanlder functions look private

Some more renaming to match metro (and make me less confused).

Comment 2

[:Margaret Leibovic]

Attachment: (Part 3) Use selectAtPoint to start a selection

Here's the real patch. I suppose there was a bit of scope creep to refactor showThumb here, but I just went for it while I was creating _initTargetInfo.

Text selection still works with our mouse event hacks, but this removes the first hack to start the selection!

Modeled after _onSelectionStart and _onCaretAttach:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/contenthandlers/SelectionHandler.js#108
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/contenthandlers/SelectionHandler.js#260

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 729251 [details] [diff] [review]
(Part 2) Make private SelectionHanlder functions look private

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

nit: s/SelectionHanlder/SelectionHandler/ in commit message

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 729256 [details] [diff] [review]
(Part 3) Use selectAtPoint to start a selection

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

Looks good to me with the event type fixed.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +236,5 @@
> +   */
> +  attachCaret: function sh_attachCaret(aElement) {
> +    this._initTargetInfo(aElement);
> +
> +    this._contentWindow.addEventListener("pagehide", this, false);

A "pagehide" listener is already added in _initTargetInfo -- I assume this should be "blur"?

Comment 5

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Comment on attachment 729256 [details] [diff] [review]
> (Part 3) Use selectAtPoint to start a selection
> 
> Review of attachment 729256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me with the event type fixed.
> 
> ::: mobile/android/chrome/content/SelectionHandler.js
> @@ +236,5 @@
> > +   */
> > +  attachCaret: function sh_attachCaret(aElement) {
> > +    this._initTargetInfo(aElement);
> > +
> > +    this._contentWindow.addEventListener("pagehide", this, false);
> 
> A "pagehide" listener is already added in _initTargetInfo -- I assume this
> should be "blur"?

Nice catch, thanks.

Comment 6

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4a2753c2a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/16adf42b1eee
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2ec9e008ee9

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e4a2753c2a2
https://hg.mozilla.org/mozilla-central/rev/16adf42b1eee
https://hg.mozilla.org/mozilla-central/rev/e2ec9e008ee9