848594 - Story - Dragging the starting selection monocle to the left in a scrollable text input with selection doesn't work

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Input, defect, P2)

Description

[Jim Mathies [:jimm]]

Attachment: testcase

STR:

1) press hold a word in the text input in the test case.
2) drag right hand selection monocle out to select text and scroll the input some way to the right.
3) try to drag the left hand monocle to the left to expand the start selection node.

result: selection doesn't scroll properly and selected text isn't expanded.

we'll likely need new apis to do this right.

Comment 1

[Jim Mathies [:jimm]]

Attachment: simplify sel advance

This simplifies character advancement which was using content oriented selection routines instead of the easier to work with text input selection indexes. However this doesn't solve the edit scrolling problem.

There are a couple problems here:

1) anchor selection advancement - this simply doesn't work. The selection controller has apis in it that request "scroll to anchor region" but the text input seems to be hardwired to keep the focus node visible. So the controller calls don't work. The visual results caused by this conflict are less than stellar.

2) drag focus node to anchor node - strange things currently happen. This may just be a bug in the way I'm using the controller here, not sure. The entire edit resets such that the focus node ends up all the way over on the right boundary. But the monocle is still over on the left, creating a gap between the two.

I'm wondering if we could create some sort of a new ui mode to solve this that doesn't involve changing selection in the text input at all. Android has a nice hover magnifier window for their address bar, maybe something like that for restricted text inputs in content.

Comment 2

[Jim Mathies [:jimm]]

I'm going to split this patch out to a new bug so I can land it independent of this issue.

Comment 3

[Marco Mucci [:MarcoM]]

Separating this out from Bug 851388 to its own Story for inclusion in Iteration #5.

Comment 4

[Jim Mathies [:jimm]]

Attachment: code reorg

Comment 5

[Jim Mathies [:jimm]]

Attachment: fix

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 728644 [details] [diff] [review]
code reorg

This is purely a code re-org, no logic changes.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 728666 [details] [diff] [review]
fix

This update uses the new html5 interface setSelectionRange to rearrange the anchor and focus selection nodes depending on the direction of the drag. Prior to this patch, I was manipulating the anchor when dragging to the left in ltr languages. However text edits are designed to keep the focus node visible, which conflicted with my requests to scroll to the anchor. With this patch I'm always working on the focus node which avoids these issues.

Note, no rush here since we don't want to close this out until it5 has begun.

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 728644 [details] [diff] [review]
code reorg

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

::: browser/metro/base/content/contenthandlers/SelectionHandler.js
@@ +625,5 @@
> +   *
> +   * @param the marker currently being manipulated
> +   * @param aClientPoint the point designating the new start or end
> +   * position for the selection.
> +   * @param aAdjustedClientPoint client point adjusted for line height.

This aClientPoint is unused.  Can you remove it and just pass aAdjustedClientPoint?

@@ +650,5 @@
> +      // recurring timer to keep the expected selection behavior going as
> +      // long as the user keeps the monocle out of bounds.
> +      if (!this._scrollTimer)
> +        this._scrollTimer = new Util.Timeout();
> +      this._setTextEditUpdateInterval(result.speed);

While you are here, please move the "if (!this._scrollTimer)" block inside of _setTextEditUpdateInterval -- or into a separate "get scrollTimer" method if you prefer.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 728666 [details] [diff] [review]
fix

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

Just some trivial nits below.

::: browser/metro/base/content/contenthandlers/SelectionHandler.js
@@ +877,5 @@
>      let selCtrl = this._getSelectController();
>      try {
>        if (aLocation == kSelectionNodeAnchor) {
> +        let start = this._targetElement.selectionStart == 0 ?
> +          0 : this._targetElement.selectionStart - 1;

Optional suggestion: I think this might be a little easier to read as:

let start = Math.max(this._targetElement.selectionStart - 1, 0);

@@ +884,3 @@
>        } else {
> +        let end = this._targetElement.selectionEnd == this._targetElement.textLength ?
> +          this._targetElement.textLength : this._targetElement.selectionEnd + 1;

let end = Math.min(this._targetElement.selectionEnd + 1, this.targetELement.textLength);

@@ +895,5 @@
> +  },
> +
> +  _updateInputFocus: function _updateInputFocus(aMarker) {
> +    try {
> +      let selCtrl = this._getSelectController();

Nit: Please move this line down below the next statement.

Comment 10

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/12a599069db2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6d94c44aa4d2

Thanks for the quick reviews!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12a599069db2
https://hg.mozilla.org/mozilla-central/rev/6d94c44aa4d2

Comment 12

[juan becerra [:juanb]]

Tested on 2013-03-29 using a Nightly built from http://hg.mozilla.org/mozilla-central/rev/8693d1d4c86d
- Tested using the STR in comment #0 and took into account the information in bug 851388. Dragging the monocle to the left in a scrollable text input with selection works now.
- Dragging to the left or right, or up and down, selects the text. If there is text past the left or right, it scrolls as it selects.
- Monocles are dismissed when touching outside that text area.

Comment 13

[juan becerra [:juanb]]

While verifying bug 858556, I found out this no longer works.

Comment 14

[Jim Mathies [:jimm]]

Please file a fresh regression bug.

Comment 15

[Marco Mucci [:MarcoM]]

Hey Juan, see Comment #14.

Comment 16

[Jim Mathies [:jimm]]

I went ahead and filed bug 859490. There are a couple of odd things going on here I'd like to look at before it5 closes out.

Comment 17

[Marco Mucci [:MarcoM]]

Thanks Jim.  I've cancelled the request to Juan.

Comment 18

[Brian R. Bondy [:bbondy]]

Retested this and found the same issue as Juan which is already posted in bug 867657.

Comment 19

[juan becerra [:juanb]]

While testing this for iteration #10 I haven't been able to reproduce the problem described in bug 867657.

Comment 20

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130812030209
Built from http://hg.mozilla.org/mozilla-central/rev/87c1796bc46c

WFM
Tested on windows 8 using latest nightly for iteration-11. Followed steps provided in comment0 and got expected result.

Comment 21

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130825030201
Built from http://hg.mozilla.org/mozilla-central/rev/01576441bdc6

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and got expected result.