862054 - Defect - caret selection via touch not working, no grippers appear (elementFromPoint doesn't take clientX/clientY from sub frame events.)

Status: VERIFIED FIXED

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

Description

[juan becerra [:juanb]]

Tested on 2013-04-15 using latest nightly built from http://hg.mozilla.org/mozilla-central/rev/261d6997d1d1

While regression testing bug 851388 I noticed that text selection using touch isn't working. The grippers don't show.

Steps:
1. In Firefox Metro go to https://developer.mozilla.org/en-US/docs/HTML/HTML_Elements/textarea#HTML_Content
2. Tap on a word in the text area

Expected: You should see a monocle you can drag  left or fright to select thext

Actual: Caret blinking but no monocle.

You can select text using mouse and also with hardware arrows.

Comment 1

[Jim Mathies [:jimm]]

Appears to be a problem with elementFromPoint - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/contenthandlers/Content.js#421

I placed a dump statement here, it returns random elements, a div, a head, and sometimes the test area. The textarea in this sample is in an iframe, so looks like elementFrompoint is having a hard time with that.

Comment 2

[Jim Mathies [:jimm]]

Attachment: part 1 - move translateToTopLevelWindow to Utils

Comment 3

[Jim Mathies [:jimm]]

Attachment: part 2 - get rid of ElementTouchHelper and elementFromPoint

I don't see why we need this anymore. We have fluffing through the dom so this just fluffs things out more while adding complexity to Content.js. FWIW, I've been surfing around with this and having no issues.

mbrubeck, curious what your thoughts are on this?

Comment 4

[Jim Mathies [:jimm]]

Attachment: part 3 - misc

A few issues addressed here - 

1) the Content:SelectionCaret message needs top level browser coordinates. I was sending subframe coordinates.

2) _restrictSelectionRectToEditBounds needs to restrict to browser vs. subframe as well.

3) minor tweak to the deck offset code that popped up here, this._targetElement.ownerDocument.defaultView.innerHeight is the subframe height, not the browser height.

Generally SelectionHandler isn't well tested with input in subframes so there are probably other bugs. When I get back to updating my selection tests I'll put together a big set of sub frame element related tests and address any additional issues in follow ups.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 738108 [details] [diff] [review]
part 1 - move translateToTopLevelWindow to Utils

simple code move.

Comment 6

[Brian R. Bondy [:bbondy]]

Comment on attachment 738108 [details] [diff] [review]
part 1 - move translateToTopLevelWindow to Utils

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

::: browser/metro/base/content/contenthandlers/ContextMenuHandler.js
@@ +175,5 @@
>  
>      let { targetWindow: targetWindow,
>            offsetX: offsetX,
>            offsetY: offsetY } =
> +      Utils.translateToTopLevelWindow(aPopupNode);

Shouldn't this be Util.translateToTopLevelWindow(aPopupNode);?

Comment 7

[Jim Mathies [:jimm]]

Attachment: part 1 - move translateToTopLevelWindow to Utils

Nice catch, that would have broken something I'm sure. :)

Comment 8

[Jim Mathies [:jimm]]

mbrubeck - ping.

I'm currently working up some tests for this in bug 863748. Thus far I haven't seen any issues. Curious if you know of anything elementFromPoint is helping us with warranting it's use. If not I'd like to go ahead and seek reviews on these changes.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 738110 [details] [diff] [review]
part 2 - get rid of ElementTouchHelper and elementFromPoint

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

Yes, I'm in favor of this.  ElementTouchHelper was originally designed for tapping on clickable items like links and buttons; we're not using it for that since Gecko now handles it for us (bug 780847).  I don't think it's really necessary or suitable for text selection.

Comment 10

[Jim Mathies [:jimm]]

Attachment: get rid of ElementTouchHelper and elementFromPoint

No changes to part 2 compared to the original patch. 99% code removal.

Comment 11

[Jim Mathies [:jimm]]

Attachment: part 3 - SelectionHandler should use browser relative coords

This is pretty straight forward. In three places in the selection code, I use the bounds of the element in calculations. The original routine returned coords that were relative to the frame the element was in. If there was a sub frame things broke, if not coords were relative to the top level browser, which is why this all worked outside of sub frames.

I've written a number of tests for the sub frame case in the dependent bug. Top level coords are tested by existing selection tests that have already landed. Everything passes with these changes.

Comment 12

[Brian R. Bondy [:bbondy]]

Comment on attachment 740483 [details] [diff] [review]
part 3 - SelectionHandler should use browser relative coords

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

::: browser/metro/base/content/contenthandlers/Content.js
@@ +281,5 @@
>  
>      // A tap on a form input triggers touch input caret selection
>      if (Util.isTextInput(element) &&
>          aEvent.mozInputSource == Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH) {
> +      let { targetWindow, offsetX, offsetY } = Util.translateToTopLevelWindow(element);

I think you ca just do:
let { offsetX, offsetY } = Util.translateToTopLevelWindow(element);

here.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31d99490ea46
https://hg.mozilla.org/mozilla-central/rev/29100949293d
https://hg.mozilla.org/mozilla-central/rev/e220c750ec35

Comment 14

[Marco Mucci [:MarcoM]]

For Virgil to test and verify.

Comment 15

[Virgil Dicu [:virgil] [QA]]

I won't be able to verify this, as I don't have access to a Win8 tablet. Adding Juan.

Comment 16

[juan becerra [:juanb]]

Tested on 2013-04-24 using a nightly built from http://hg.mozilla.org/mozilla-central/rev/fef5f202b2dc
- You can now select text via touch using the grippers, but Firefox for Metro isn't following the guidelines described in http://msdn.microsoft.com/en-us/library/windows/apps/hh465334.aspx which make text selection in IE more natural.
- Filed bug 865369 for the follow-up issue

Comment 17

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

We really don't need to be verifying this bug anymore since there are a bunch of other bugs whose verification steps involve selecting text using touch input, but... verified for it8