Closed Bug 858526 Opened 7 years ago Closed 7 years ago

domWinUtils sendQueryContentEvent w/QUERY_CARET_RECT wildly inaccurate in form inputs

Categories

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

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch test (obsolete) — Splinter Review
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.
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.
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.
Attached patch metro test (obsolete) — Splinter Review
Attachment #733820 - Attachment is obsolete: true
Attached patch patchSplinter Review
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)
cc'ing masayuki since these routines are heavily used by the Windows IME code.
Blocks: skb
Attachment #733939 - Flags: feedback?(ehsan) → review?(ehsan)
Attachment #733939 - Flags: review?(ehsan) → review+
Comment on attachment 733938 [details] [diff] [review]
metro test

Need to work up a test for this.
Attachment #733938 - Attachment is obsolete: true
Attached patch testsSplinter Review
Attachment #735347 - Flags: review?(ehsan)
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+
(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.
(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
Closed: 7 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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.