940952 - Fixup selection with apzc pan/zoom

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Pan and Zoom, defect)

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 3

[Jim Mathies [:jimm]]

I'm going to let this sit for a bit until we get buffered zoomtorect sorted out for double tap.

Comment 4

[Jim Mathies [:jimm]]

Attachment: part 1

Comment 5

[Jim Mathies [:jimm]]

Attachment: part 2

Comment 6

[Jim Mathies [:jimm]]

Attachment: part 3

Comment 7

[Jim Mathies [:jimm]]

Attachment: part 1 - better, general purpose tracking of apzc activity for front end v.1

This patch adds better apzc tracking for front end script. I'm using these to hide selection handles in the front end during pan, zoom and fling operations.

Comment 8

[Jim Mathies [:jimm]]

Attachment: part 2 - remove some event cruft from SelectionHelperUI.js

Comment 9

[Jim Mathies [:jimm]]

Attachment: part 3 - fixup selection code to handle pan/zoom v.1

This allows selection to stay active after apzc operations. This also fixes a missing return value warning in widget left over from the zoom to rect changes.

Comment 10

[Jim Mathies [:jimm]]

There's also one failing selection test I have to look at.

Comment 11

[Jim Mathies [:jimm]]

Attachment: disable "pan disables selection" test

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8337710 [details] [diff] [review]
part 3 - fixup selection code to handle pan/zoom v.1

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

I'd like to try an alternate approach for the new browser property:

::: browser/metro/base/content/bindings/browser.xml
@@ +611,5 @@
> +                        .defaultView
> +                        .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                        .getInterface(Components.interfaces.nsIDOMWindowUtils);
> +          let resX = {}, resY = {};
> +          cwu.getResolution(resX, resY);

You can use "this.scale" to get the resolution here.

@@ +614,5 @@
> +          let resX = {}, resY = {};
> +          cwu.getResolution(resX, resY);
> +          return {
> +            x: scroll.x  / resX.value,
> +            y: scroll.y / resY.value,

It might be nice to return a new Rect object here, so callers have some built-in methods for convenience.

@@ +616,5 @@
> +          return {
> +            x: scroll.x  / resX.value,
> +            y: scroll.y / resY.value,
> +            width: this.contentWindowWidth / resX.value,
> +            height: this.contentWindowHeight / resY.value

I think you should use this.getBoundingClientRect().width and .height here.  Currently this will usually be the same as contentWindowWidth/Height, but I think that will change when we allow pages to change their viewport dimensions.

If we want to get more general, you could write a rectClientToBrowser method (in the style of rectBrowserToClient) and then this property could be implemented as:

  return this.rectClientToBrowser(this.getBoundingClientRect());

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8337707 [details] [diff] [review]
part 2 - remove some event cruft from SelectionHelperUI.js

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

::: browser/metro/base/content/input.js
@@ -32,5 @@
> -// subtle event sequencing differences in this feature when running on
> -// the desktop using the win32 widget backend and the winrt widget backend
> -// in metro. Fixing something in this mode does not insure the bug is
> -// in metro.
> -const kDebugMouseInputPref = "metro.debug.treatmouseastouch";

If you remove this, make sure to also remove TouchModule._treatMouseAsTouch and all references to it.

Comment 15

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #14)
> Comment on attachment 8337707 [details] [diff] [review]
> part 2 - remove some event cruft from SelectionHelperUI.js
> 
> Review of attachment 8337707 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/content/input.js
> @@ -32,5 @@
> > -// subtle event sequencing differences in this feature when running on
> > -// the desktop using the win32 widget backend and the winrt widget backend
> > -// in metro. Fixing something in this mode does not insure the bug is
> > -// in metro.
> > -const kDebugMouseInputPref = "metro.debug.treatmouseastouch";
> 
> If you remove this, make sure to also remove TouchModule._treatMouseAsTouch
> and all references to it.

awesome! more cruft, nice catch.

Comment 16

[Jim Mathies [:jimm]]

Attachment: part 3 - fixup selection code to handle pan/zoom v.1

Implemented all your suggestions.

One of these days I think we need to get into browser.xml and do some cleanup. I'd also like to see the local and remote browsers split up into two separate files.

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8337703 [details] [diff] [review]
part 1 - better, general purpose tracking of apzc activity for front end v.1

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

It looks like Metro's is the only GeckoContentController implementation that ever used HandlePanBegin() and HandlePanEnd(). Since it's no longer using them, let's get rid of them.

r=me with that change

Comment 18

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/7915cbaa245c
remote:   https://hg.mozilla.org/integration/fx-team/rev/9dc6d9732176
remote:   https://hg.mozilla.org/integration/fx-team/rev/4825747a322c
remote:   https://hg.mozilla.org/integration/fx-team/rev/354c0301057d

Thanks!

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7915cbaa245c
https://hg.mozilla.org/mozilla-central/rev/9dc6d9732176
https://hg.mozilla.org/mozilla-central/rev/4825747a322c
https://hg.mozilla.org/mozilla-central/rev/354c0301057d