832957 - Work - Touch selection in content regressed

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect)

Description

[Jim Mathies [:jimm]]

This cropped up over the last month or so, likely due to all our input handling changes. Need to go over selection code for content again and work through any regressions. Also some tests that hopefully catch future regressions would be good.

Comment 1

[Jim Mathies [:jimm]]

Attachment: wip

Comment 2

[Jim Mathies [:jimm]]

Attachment: [checked-in] fix download test line endings

might as well land this now.

Comment 3

[Jim Mathies [:jimm]]

Attachment: wip

Comment 4

[Jim Mathies [:jimm]]

Attachment: text area test case

Comment 5

[Jim Mathies [:jimm]]

str:

1) apply both patches
2) load test case
3) press-hold a work in the text area
4) expand out selection if you want to include more text
5) tap anywhere in the selected text

results:
selection is cleared, and the cursor moves to the tap location

Note, in the patch, _onTap in SelectionHelperUI.js calls stopPropagation and preventDefault. But the event is still delivered. I also added mousedown and mouseup captures and did the same, same result.

disable ui.mouse.radius.enabled, ui.touch.radius.enabled

do test again. selection doesn't get cleared.

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 711801 [details] [diff] [review]
wip

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

Some random drive-by comments.

::: browser/metro/base/tests/browser_selection_tests.js
@@ +48,5 @@
> +
> +function waitForSelectionChange(aTarget, aCallback) {
> +  let deferred = Promise.defer();
> +  gSelWaitIdx = 0;
> +  let selText = getSelection(aTarget).toString();

I think you might be able to implement this in terms of waitForCondition.

@@ +91,5 @@
> +
> +    info(chromeRoot + "browser_selection_tests_01.html\n");
> +    yield addTab(chromeRoot + "browser_selection_tests_01.html");
> +
> +    yield waitForMs(500);

Really hoping we can avoid hard-coded timeouts.  If you're waiting for the ContextUI to disappear, you could waitForEvent(Elements.tray, "transitionend").  Or we could make ContextUI.forceDismiss disable the transition.

::: browser/metro/base/tests/head.js
@@ +274,5 @@
> +  } catch (ex) {
> +    info("sendContextMenuClick: " + ex.message);
> +  }
> +}
> +

It's probably better to let the exception propagate so that it will cause a test failure (unless the caller chooses to catch it).

@@ +303,5 @@
> +/*
> + * sendTouchEvent - sends a single touch event. See sendTouchDrag
> + * for use.
> + */
> +function sendTouchEvent(aWinUtils, aType, aEvent, aModifiers) {

Can we use EventUtils.synthesizeTouch and friends, instead?

Comment 7

[Jim Mathies [:jimm]]

Attachment: wip v.2

Comment 8

[Jim Mathies [:jimm]]

> ::: browser/metro/base/tests/browser_selection_tests.js
> @@ +48,5 @@
> > +
> > +function waitForSelectionChange(aTarget, aCallback) {
> > +  let deferred = Promise.defer();
> > +  gSelWaitIdx = 0;
> > +  let selText = getSelection(aTarget).toString();
> 
> I think you might be able to implement this in terms of waitForCondition.

Updated to use that. 

> 
> @@ +91,5 @@
> > +
> > +    info(chromeRoot + "browser_selection_tests_01.html\n");
> > +    yield addTab(chromeRoot + "browser_selection_tests_01.html");
> > +
> > +    yield waitForMs(500);
> 
> Really hoping we can avoid hard-coded timeouts.  If you're waiting for the
> ContextUI to disappear, you could waitForEvent(Elements.tray,
> "transitionend").  Or we could make ContextUI.forceDismiss disable the
> transition.

Got rid of most of these with some better helpers.

> @@ +303,5 @@
> > +/*
> > + * sendTouchEvent - sends a single touch event. See sendTouchDrag
> > + * for use.
> > + */
> > +function sendTouchEvent(aWinUtils, aType, aEvent, aModifiers) {
> 
> Can we use EventUtils.synthesizeTouch and friends, instead?

I'm still using my own as they do some special things that help make the test more stable. I'll look more at these but if they don't fit in well I'd prefer to stick with my own.

Comment 9

[Jim Mathies [:jimm]]

Attachment: selection overlay binding v.1

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 718022 [details] [diff] [review]
selection overlay binding v.1

This connects the contextmenu event on the selection overlay to our contextmenu processing code down in content. 

Note, I'm not passing events through this overlay, it still acts as a trap. I would like to get away from that, but it creates a number of problems. I might try to do it, or simply wait until we can get the logic down into dom (which I'd like to work on this summer.)

Comment 11

[Jim Mathies [:jimm]]

Attachment: remove old copy ContextCommand code

This originally went over to the ContextMenuHandler where we coped active selection. However the current ContextMenuHandler logic always puts the current selection in the message it sends over first, so this is no longer needed. (and there's no receiver for it anyway, so it would just disappear into the message manager ether.)

Comment 12

[Jim Mathies [:jimm]]

Attachment: dom bug 835175 fix

Dom bug 835175 fix for the selection overlay.

Comment 13

[Jim Mathies [:jimm]]

Attachment: the big patch of little fixes v.1

This accomplishes a few things:

- it standardizes on client coordinates for messages sent between SelectionHelperUI and SelectionHandler. This makes coding much simpler.

- adds a word snap feature for when you are dragging monocles. Currently snap is always on, but once we get zoom working, I want to flip it between snap and precise depending on the zoom level. Snap mode just makes it simpler to select a sentence, since you don't have to drag all the way to a specific character.

- cleans up a few random exception in selection code that can occur (found by running tests.)

- cleans up some confusion between content / client coords in comments and method names.

- fixes a bug _setContinuousSelection where case 0 was using the wrong range end points. 

- fixes a bug in _shrinkSelectionFromPoint where it was ignoring all but the base index range in a range set.

- streamlines sending messages in SelectionHelperUI through a single helper method.

I can try to break this up more if you like. basically I was just twiddling with the code while I wrote tests to get everything working right.

Comment 14

[Jim Mathies [:jimm]]

Attachment: double click patch

This traps double clicks in content. Comments explain why, this is a temporary fix until double-tap to zoom is working.

Comment 15

[Jim Mathies [:jimm]]

Attachment: tests v.1

lots and lots of tests!

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 718031 [details] [diff] [review]
the big patch of little fixes v.1

wrong patch.

Comment 17

[Jim Mathies [:jimm]]

Attachment: the big patch of little fixes v.1

Comment 18

[Matt Brubeck (:mbrubeck)]

Comment on attachment 718022 [details] [diff] [review]
selection overlay binding v.1

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

::: browser/metro/base/content/contenthandlers/Content.js
@@ +405,5 @@
>     * message manager or sent directly via dispatch.
>     */
>  
>    _genericMouseDown: function _genericMouseDown(x, y) {
> +    let { element: element } = elementFromPoint(x, y);

"let { element } = ..." works as a nice little short-hand for this.

::: browser/metro/base/content/contenthandlers/ContextMenuHandler.js
@@ +90,5 @@
>     */
>    _onContextAtPoint: function _onContextCommand(aMessage) {
>      // we need to find popupNode as if the context menu were
>      // invoked on underlying content.
> +    let { element: element, frameX: frameX, frameY: frameY } =

and here, "let { element, frameX, frameY } = ..."

@@ +175,5 @@
> +    let win = null;
> +    if (element == aPopupNode)
> +      win = content;
> +    else
> +      win = element.contentDocument.defaultView;

Can you just use "let win = element.contentDocument.defaultView;" all the time?

@@ +309,5 @@
>        // If this is text and has a selection, we want to bring
>        // up the copy option on the context menu.
> +      if (targetWindow.getSelection() &&
> +          targetWindow.getSelection().toString().length > 0) {
> +        state.string = targetWindow.getSelection().toString();

Could you put a "let selection = targetWindow.getSelection();" at the start of this, just for better readability?

Comment 19

[Matt Brubeck (:mbrubeck)]

Comment on attachment 718033 [details] [diff] [review]
double click patch

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

::: browser/metro/base/content/input.js
@@ +150,5 @@
> +              let contextInfo = { name: "",
> +                                  json: { xPos: aEvent.clientX, yPos: aEvent.clientY },
> +                                  target: Browser.selectedTab.browser };
> +              SelectionHelperUI.attachEditSession(contextInfo);
> +            }, 50);

It would be good if we can avoid the arbitrary timeout here.  It looks like the alternative would be to use addEventListener("select") and nsISelectionPrivate.addSelectionListener in the content script, and send a message back to chrome when the selection happens.

However, since we are planning to get rid of double-tap for selection anyway, we can skip that for now (or do it in a separate bug if it's still useful when we add more Metro-style selection gestures).

Comment 20

[Matt Brubeck (:mbrubeck)]

Comment on attachment 718049 [details] [diff] [review]
the big patch of little fixes v.1

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

Looks good with some minor changes (mostly stylistic).

::: browser/metro/base/content/contenthandlers/SelectionHandler.js
@@ +96,5 @@
> +  },
> +
> +  set snap(aVal) {
> +    this._snap = aVal;
> +  },

If this setter is unused, could you remove it for now?

@@ +548,5 @@
>  
>      // Tests to see if the user is trying to shrink the selection, and if so
>      // collapses it down to the appropriate side such that our calls below
>      // will reset the selection to the proper range.
> +    let shrunk = this._shrinkSelectionFromPoint(aMarker, framePoint);

Do we actually use this return value?

@@ +555,5 @@
>      try {
> +      // If we're at the end of a selection (touchend) snap to the word.
> +      let type = ((aEndOfSelection && !this._snap) ?
> +        Ci.nsIDOMWindowUtils.SELECT_CHARACTER :
> +        Ci.nsIDOMWindowUtils.SELECT_WORD);

If aEndOfSelection is false, this always returns SELECT_WORD regardless of the "snap" setting, which seems wrong.  I think it should be ((aEndOfSelection && this.snap) ? WORD : CHARACTER).

@@ +1036,5 @@
>     * such that:
>     * sub frame coords + offset = root frame position
>     */
>    getCurrentWindowAndOffset: function(x, y) {
> +    // If the element at the given point belongs to anothe document (such

Typo: "anothe"

@@ +1059,1 @@
>        // subtract frame and scroll offset and from elementFromPoint coordinates

You can remove this now-redundant (and ungrammatical) comment.

@@ +1064,3 @@
>        // add frame client offset to our total offset result
>        offset.x += rect.left;
> +      // add the frame's y offset to our total offset result

and let's get rid of this one too.

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +201,5 @@
>    get startMark() {
>      if (this._startMark == null) {
>        this._startMark = new Marker();
>        this._startMark.tag = "start";
> +      this._startMark.init(this, "selectionhandle-start", 0, 0);

It looks like Marker.prototype.init just ignores the last two arguments.  Unless I'm missing something, we should get rid of them.  We could also get rid of the "init" method and just pass these arguments to the constructor...

@@ -305,4 @@
>     *
>     * Determines if an edit session is currently active.
>     */
> -  isActive: function isActive() {

I think you'll need to update a call to isActive() in Browser.receiveMessage in browser.js (unless one of the other patches here already removed that call and I've forgotten).

@@ +536,5 @@
>    _onResize: function _onResize() {
> +    this._sendAsyncMessage("Browser:SelectionUpdate", {});
> +  },
> +
> +  _onConextUIVisibilityEvent: function _onConextUIVisibilityEvent(aType) {

Typo ("Conext")

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 718045 [details] [diff] [review]
tests v.1

moving this over to the tests bug.

Comment 22

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #18)
> Comment on attachment 718022 [details] [diff] [review]
> selection overlay binding v.1
> 
> @@ +175,5 @@
> > +    let win = null;
> > +    if (element == aPopupNode)
> > +      win = content;
> > +    else
> > +      win = element.contentDocument.defaultView;
> 
> Can you just use "let win = element.contentDocument.defaultView;" all the
> time?

Apparently not, aPopupNode.contentDocument is undefined. contentDocument is only present when you're dealing with a frame element. elements have ownerDocument.

Comment 23

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a2c6dda9543e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/33dd2de9984f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6b2a53188ca1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad8a9bf306a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7d3714833b58

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a2c6dda9543e
https://hg.mozilla.org/mozilla-central/rev/33dd2de9984f
https://hg.mozilla.org/mozilla-central/rev/6b2a53188ca1
https://hg.mozilla.org/mozilla-central/rev/0ad8a9bf306a
https://hg.mozilla.org/mozilla-central/rev/7d3714833b58