domWinUtils sendQueryContentEvent w/QUERY_CARET_RECT wildly inaccurate in form inputs

RESOLVED FIXED in mozilla23

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
mozilla23
x86_64
Windows 8.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 733820 [details] [diff] [review]
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.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 733938 [details] [diff] [review]
metro test
Attachment #733820 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 733939 [details] [diff] [review]
patch

If this looks ok I'll also put together a more general mochitest test that can run in platform.
Assignee: nobody → jmathies
Attachment #733939 - Flags: feedback?(ehsan)
(Assignee)

Comment 5

5 years ago
cc'ing masayuki since these routines are heavily used by the Windows IME code.
(Assignee)

Updated

5 years ago
Blocks: 850413
(Assignee)

Updated

5 years ago
Attachment #733939 - Flags: feedback?(ehsan) → review?(ehsan)

Updated

5 years ago
Attachment #733939 - Flags: review?(ehsan) → review+
(Assignee)

Comment 7

5 years ago
Comment on attachment 733938 [details] [diff] [review]
metro test

Need to work up a test for this.
Attachment #733938 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 735347 [details] [diff] [review]
tests
Attachment #735347 - Flags: review?(ehsan)

Comment 9

5 years ago
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.)
Attachment #735347 - Flags: review?(ehsan) → review+
(Assignee)

Comment 10

5 years ago
(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

5 years ago
(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. :(
https://hg.mozilla.org/mozilla-central/rev/7dc94dc6cd22
https://hg.mozilla.org/mozilla-central/rev/4d115df0d920
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.