Closed Bug 832957 Opened 8 years ago Closed 8 years ago

Work - Touch selection in content regressed

Categories

(Firefox for Metro Graveyard :: Input, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: feature=work)

Attachments

(6 files, 6 obsolete files)

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.
Summary: Touch selection in content regressed → Work - Touch selection in content regressed
Whiteboard: feature=work
Blocks: 831985
Attached patch wip (obsolete) — Splinter Review
might as well land this now.
Attachment #710857 - Attachment is obsolete: true
Attachment #711799 - Flags: review?(mbrubeck)
Attached patch wip (obsolete) — Splinter Review
Attached file text area test case (obsolete) —
Attachment #711799 - Flags: review?(mbrubeck) → review+
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.
Attachment #711799 - Attachment description: fix download test line endings → [checked-in] fix download test line endings
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?
Attached patch wip v.2 (obsolete) — Splinter Review
Attachment #711801 - Attachment is obsolete: true
> ::: 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.
Attachment #711802 - Attachment is obsolete: true
Attachment #713544 - Attachment is obsolete: true
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.)
Attachment #718022 - Flags: review?(mbrubeck)
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.)
Attachment #718026 - Flags: review?(mbrubeck)
Dom bug 835175 fix for the selection overlay.
Attachment #718027 - Flags: review?(mbrubeck)
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.
Attachment #718031 - Flags: review?(mbrubeck)
This traps double clicks in content. Comments explain why, this is a temporary fix until double-tap to zoom is working.
Attachment #718033 - Flags: review?(mbrubeck)
Attached patch tests v.1 (obsolete) — Splinter Review
lots and lots of tests!
Attachment #718045 - Flags: review?(sfoster)
Attachment #718045 - Attachment is patch: true
Comment on attachment 718031 [details] [diff] [review]
the big patch of little fixes v.1

wrong patch.
Attachment #718031 - Flags: review?(mbrubeck)
Attachment #718031 - Attachment is obsolete: true
Attachment #718049 - Flags: review?(mbrubeck)
Blocks: 845122
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?
Attachment #718022 - Flags: review?(mbrubeck) → review+
Attachment #718026 - Flags: review?(mbrubeck) → review+
Attachment #718027 - Flags: review?(mbrubeck) → review+
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).
Attachment #718033 - Flags: review?(mbrubeck) → review+
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")
Attachment #718049 - Flags: review?(mbrubeck) → review+
Comment on attachment 718045 [details] [diff] [review]
tests v.1

moving this over to the tests bug.
Attachment #718045 - Attachment is obsolete: true
Attachment #718045 - Flags: review?(sfoster)
(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.
No longer blocks: 845122
Depends on: 846412
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.