666977 - Panning ends selection in progress

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:7.0a1) Gecko/20110624 Firefox/7.0a1 Fennec/7.0a1

Currently when one pans or scrolls to expose more text for selection it will break the text selection in progress.

Comment 2

[Aaron Train [:aaronmt]]

Reopening, it seems that bug 666915 is requesting a design for an automatic scroll, where in this bug I'm referring to a manual scroll from the user one leaving the selection up

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

this patch takes care of maintaining the selection during a pan. Zooming and rotation are probably fairly different, we should get a new bug for that.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Looks like I misspoke, this patch should take care of maintaining the selection during panning, zooming and rotation. 

Matt, shifting the review to you since mfinkle is on PTO.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

what happens when the handles move offscreen?

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

the handles pan off screen with the selection

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 553111 [details] [diff] [review]
patch

drive-by

What's the purpose of the timer? I mean, what affect are you using it to acheive?

>diff --git a/mobile/chrome/content/common-ui.js b/mobile/chrome/content/common-ui.js

>+  startPt: {x:0, y:0},

{ x:0, y:0 }

>diff --git a/mobile/chrome/content/content.js b/mobile/chrome/content/content.js

>         this.cache = this._extractFromRange(range, offset);
>         this.contentWindow = contentWindow;
>+        this.offset = offset;

I thought this.cache.offset was used for holding the offset

>+      case "Browser:SelectionMeasure":
>+      {

nit: { up on the case line is the normal style here

>+        let currentDocShell = this.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebNavigation).QueryInterface(Ci.nsIDocShell);
>+        let selcon = currentDocShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsISelectionDisplay).QueryInterface(Ci.nsISelectionController);
>+        let selection = selcon.getSelection(1);
>+        let range = selection.getRangeAt(0).QueryInterface(Ci.nsIDOMNSRange);

Why do you use the selection controller to get the selection? why not | this.contentWindow.getSelection() |  ?

>+        // If the range didn't have any text, let's bail
>+        if (!this.selectedText.length) {
>+          selection.collapseToStart();
>+          return;
>+        }

I think we started to use | selection.removeAllRanges() | because it does what we really want.

>+        this.cache = this._extractFromRange(range, this.offset);

          this.cache = this._extractFromRange(range, this.cache.offset);

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 553111 [details] [diff] [review]
patch

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

The overall patch looks good.  I'll review it again with all of our picky nits addressed.

::: mobile/chrome/content/common-ui.js
@@ +1346,5 @@
>    handleEvent: function handleEvent(aEvent) {
>      switch (aEvent.type) {
> +      case "PanBegin":
> +        window.removeEventListener("PanBegin", this, true);
> +        if (this.panTimer != 0) {

if (this.panTimer)

@@ +1371,5 @@
>          } else {
> +          window.addEventListener("PanBegin", this, true);
> +          this._start.hidden = true;
> +          this._end.hidden = true;
> +          this.panTimer = window.setTimeout(function(self, aEvent) {

You don't need to pass aEvent here; it will still be in scope from the outer function.

@@ +1380,4 @@
>          }
>          break;
>        case "TapUp":
> +        if (this.target != null) {

if (this.target)

@@ +1407,5 @@
>        case "ZoomChanged":
> +      {
> +        let json = { };
> +        try {
> +          this.popupState.target.messageManager.sendAsyncMessage("Browser:SelectionMeasure", json);

No need for the "json" local -- just sendAsyncMessage("...", {})

::: mobile/chrome/content/content.js
@@ +1507,5 @@
> +      case "Browser:SelectionMeasure":
> +      {
> +        let currentDocShell = this.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebNavigation).QueryInterface(Ci.nsIDocShell);
> +        let selcon = currentDocShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsISelectionDisplay).QueryInterface(Ci.nsISelectionController);
> +        let selection = selcon.getSelection(1);

Can you use Ci.nsISelectionController.SELECTION_NORMAL here, instead of "1"?  (Update: Or see mfinkle's comment.)

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 553111 [details] [diff] [review]
> patch
> 
> drive-by
> 
> What's the purpose of the timer? I mean, what affect are you using it to
> acheive?
> 
> >diff --git a/mobile/chrome/content/common-ui.js b/mobile/chrome/content/common-ui.js
> 
> >+  startPt: {x:0, y:0},
actually not using this anymore, just got rid of it
> 
> >diff --git a/mobile/chrome/content/content.js b/mobile/chrome/content/content.js
> 
> >         this.cache = this._extractFromRange(range, offset);
> >         this.contentWindow = contentWindow;
> >+        this.offset = offset;
> 
> I thought this.cache.offset was used for holding the offset
its not clear to me that this.offset is the same as this.cache.offset

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 553200 [details] [diff] [review]
patch

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

::: mobile/chrome/content/content.js
@@ +1430,4 @@
>  
>          this.cache = this._extractFromRange(range, offset);
>          this.contentWindow = contentWindow;
> +        this.offset = offset;

I think mfinkle was correct that this.offset will always be equal to this.cache.offset, so you can just remove the former and use the latter.  (_extractFromRange always sets the .offset property of its result to the same value you pass in.)

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/00f9b3304811

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/00f9b3304811

Comment 13

[Aaron Train [:aaronmt]]

Verified Fixed
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110819 Firefox/9.0a1 Fennec/9.0a1

Filed a followup: bug 680419

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: backout

It's very easy to get the selection handles showing up in the wrong position. Just select some text and pan up and down a bit. The handles do no track the selection.

This problem, coupled with the API issues (bug 681387) make me want to revert back to the state we have in Aurora.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Backout: http://hg.mozilla.org/integration/mozilla-inbound/rev/e3ce5dcd2483

Reopening

Comment 16

[(no longer active)]

Backout on central: http://hg.mozilla.org/mozilla-central/rev/e3ce5dcd2483