858526 - domWinUtils sendQueryContentEvent w/QUERY_CARET_RECT wildly inaccurate in form inputs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jim Mathies [:jimm]]

Attachment: test

Metro relies on this for some form menu positioning functions and android relies on this api in its selection related code. 

I was using this recently to adjust browser offsets when the soft keyboard is displayed and found that in some form elements, particularly textareas, the results can be wildly inaccurate the from the real caret position.

Attached is a mochitest-metro-chrome test that positions the caret at the bottom left side of a textarea. The caret position should be something like 10x395. sendQueryContentEvent returns a rect with coordinates of left:584 top:506!

Matt, any chance you could take this test for a spin when you have a free moment and see if you get the same results? The test is browser_caretrect.js.

Comment 1

[Jim Mathies [:jimm]]

Things seem to go wrong here - 

http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsContentEventHandler.cpp#704

these offsets don't match, so we fall through to a "guess"(?) mode, which does a horrible job.

Comment 2

[Jim Mathies [:jimm]]

This appears to be related to new lines in text. In OnQueryCaretRect, we query GetFlatTextOffsetOfRange passing the root text node. GetFlatTextOffsetOfRange queries the native text length, which it returns as the offset.

http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsContentEventHandler.cpp#928

The native offset is different from the offsets in selection objects, the node returned by caretPositionFromPoint, and selection offsets on the element itself. In my test case, the offset I get from these methods is 983, while the native offset is 1002, a difference of 19, which just happens to match the number of new lines in the text area prior to the caret rect.

I think we need to be calling ConvertToXPOffset on the offset handed back.

Comment 3

[Jim Mathies [:jimm]]

Attachment: metro test

Comment 4

[Jim Mathies [:jimm]]

Attachment: patch

If this looks ok I'll also put together a more general mochitest test that can run in platform.

Comment 5

[Jim Mathies [:jimm]]

cc'ing masayuki since these routines are heavily used by the Windows IME code.

Comment 6

[Jim Mathies [:jimm]]

passes try:

https://tbpl.mozilla.org/?tree=Try&rev=114b8b1f1644

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 733938 [details] [diff] [review]
metro test

Need to work up a test for this.

Comment 8

[Jim Mathies [:jimm]]

Attachment: tests

Comment 9

[(no longer active)]

Comment on attachment 735347 [details] [diff] [review]
tests

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

::: dom/tests/mochitest/chrome/queryCaretRectWin.html
@@ +10,5 @@
> +  # WARNING: this file has Windows line endings. If you
> +  # update this file please maintain them. Line endings are
> +  # taken into account when calculating query caret rect
> +  # values.
> +  #########################################################

I assume that if I ignore this comment and change the test file to have Unix line endings, there's a check here which reliably fails?  If not, please add one (by measuring the length of a text node containing a newline or something.)

Comment 10

[Jim Mathies [:jimm]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)
> I assume that if I ignore this comment and change the test file to have Unix
> line endings, there's a check here which reliably fails?  If not, please add
> one (by measuring the length of a text node containing a newline or
> something.)

I haven't been able to find an interface that provides a non-normalized text length that I can check. I looked at various interfaces on elements like divs and inputs. If you have any suggestions please post them. I'm going to ahead and land this later today, if I find something after that I can land a follow up.

Comment 11

[(no longer active)]

(In reply to comment #10)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)
> > I assume that if I ignore this comment and change the test file to have Unix
> > line endings, there's a check here which reliably fails?  If not, please add
> > one (by measuring the length of a text node containing a newline or
> > something.)
> 
> I haven't been able to find an interface that provides a non-normalized text
> length that I can check. I looked at various interfaces on elements like divs
> and inputs. If you have any suggestions please post them. I'm going to ahead
> and land this later today, if I find something after that I can land a follow
> up.

Hmm, yeah I can't seem to find a good way to test this either. :(

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7dc94dc6cd22
https://hg.mozilla.org/mozilla-central/rev/4d115df0d920

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The approach must be wrong. NS_QUERY_CARET_RECT is used by both nsIMM32Handler and nsTextStore with native text offset. The patches didn't change the current users but changed the offset meaning.

I think that nsIDOMWindowUtils::SendQueryContentEvent() should have additional argument which can chose if input and output offsets are in XP offset or native offset.

I'll work in bug998188.