Closed Bug 848594 Opened 8 years ago Closed 8 years ago

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

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

()

Details

(Whiteboard: feature=story u=metro_firefox_user c=content_features p=8 [selection][leave-open] status=verified)

Attachments

(3 files, 1 obsolete file)

Attached file 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.
Blocks: caret-sel
Attached patch simplify sel advance (obsolete) — Splinter Review
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.
I'm going to split this patch out to a new bug so I can land it independent of this issue.
Blocks: metrov1it5
Priority: -- → P2
Blocks: 831671
Separating this out from Bug 851388 to its own Story for inclusion in Iteration #5.
Assignee: nobody → jmathies
No longer blocks: caret-sel
Summary: Dragging the starting selection monocle to the left in a scrollable text input with selection doesn't work → Story - Dragging the starting selection monocle to the left in a scrollable text input with selection doesn't work
Whiteboard: [selection] → feature=story c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=13 [selection]
Attachment #727153 - Attachment is obsolete: true
Whiteboard: feature=story c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=13 [selection] → feature=story c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=8 [selection]
Attached patch code reorgSplinter Review
Attached patch fixSplinter Review
Blocks: 831565
No longer blocks: 831671
Comment on attachment 728644 [details] [diff] [review]
code reorg

This is purely a code re-org, no logic changes.
Attachment #728644 - Flags: review?(mbrubeck)
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.
Attachment #728666 - Flags: review?(mbrubeck)
Whiteboard: feature=story c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=8 [selection] → feature=story u=metro_firefox_user c=content_features p=8 [selection]
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.
Attachment #728644 - Flags: review?(mbrubeck) → review+
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.
Attachment #728666 - Flags: review?(mbrubeck) → review+
Whiteboard: feature=story u=metro_firefox_user c=content_features p=8 [selection] → feature=story u=metro_firefox_user c=content_features p=8 [selection][leave-open]
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Whiteboard: feature=story u=metro_firefox_user c=content_features p=8 [selection][leave-open] → feature=story u=metro_firefox_user c=content_features p=8 [selection][leave-open] status=verified
While verifying bug 858556, I found out this no longer works.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Please file a fresh regression bug.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
QA Contact: jbecerra
Hey Juan, see Comment #14.
Flags: needinfo?(jbecerra)
Depends on: 859490
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.
No longer depends on: 859490
Thanks Jim.  I've cancelled the request to Juan.
Flags: needinfo?(jbecerra)
Depends on: 867657
Retested this and found the same issue as Juan which is already posted in bug 867657.
While testing this for iteration #10 I haven't been able to reproduce the problem described in bug 867657.
No longer depends on: 867657
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.
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.
Status: RESOLVED → VERIFIED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.