Panning ends selection in progress

RESOLVED WONTFIX

Status

P1
normal
RESOLVED WONTFIX
7 years ago
6 years ago

People

(Reporter: aaronmt, Unassigned)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 9
ARM
Android
Dependency tree / graph

Details

(Whiteboard: [testday-2011-06-24])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 666915
(Reporter)

Comment 2

7 years ago
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
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Reporter)

Updated

7 years ago
Status: REOPENED → NEW
(Reporter)

Updated

7 years ago
Priority: -- → P1
Whiteboard: [testday-20110624] → [testday-2011-06-24]
(Reporter)

Updated

7 years ago
Summary: Panning or scrolling breaks text selection in progress → Panning/scrolling or rotation breaks text selection in progress
(Reporter)

Updated

7 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → +
Created attachment 552938 [details] [diff] [review]
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.
Assignee: nobody → blassey.bugs
Attachment #552938 - Flags: review?(mark.finkle)
Summary: Panning/scrolling or rotation breaks text selection in progress → Panning ends selection in progress
Created attachment 553111 [details] [diff] [review]
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.
Attachment #552938 - Attachment is obsolete: true
Attachment #553111 - Flags: review?(mbrubeck)
Attachment #552938 - Flags: review?(mark.finkle)
what happens when the handles move offscreen?
the handles pan off screen with the selection
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 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.)
Attachment #553111 - Flags: review?(mbrubeck) → review-
Created attachment 553200 [details] [diff] [review]
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
Attachment #553200 - Flags: review?(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.)
Attachment #553200 - Flags: review?(mbrubeck) → review+
status-firefox7: --- → affected
status-firefox8: --- → affected
http://hg.mozilla.org/integration/mozilla-inbound/rev/00f9b3304811
Whiteboard: [testday-2011-06-24] → [testday-2011-06-24][inbound]
http://hg.mozilla.org/mozilla-central/rev/00f9b3304811
Status: NEW → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
(Reporter)

Updated

7 years ago
Depends on: 680419
(Reporter)

Comment 13

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

Filed a followup: bug 680419
Status: RESOLVED → VERIFIED
Flags: in-litmus?(martijn.martijn)
(Reporter)

Updated

7 years ago
Depends on: 680425
No longer depends on: 680425
Depends on: 680425
Created attachment 558520 [details] [diff] [review]
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.
Attachment #558520 - Flags: review?(mbrubeck)
Attachment #558520 - Flags: review?(mbrubeck) → review+
Backout: http://hg.mozilla.org/integration/mozilla-inbound/rev/e3ce5dcd2483

Reopening
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 16

7 years ago
Backout on central: http://hg.mozilla.org/mozilla-central/rev/e3ce5dcd2483
Whiteboard: [testday-2011-06-24][inbound] → [testday-2011-06-24]
Assignee: blassey.bugs → nobody
tracking-fennec: + → -
(Reporter)

Updated

6 years ago
Flags: in-litmus?(martijn.martijn)
(Reporter)

Updated

6 years ago
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.