891175 - Orange caret indicator is off by a couple lines

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[alex_mayorga]

Attachment: Screen capture of the issue.

The caret indicator is off by a couple lines, please se attached.

Comment 1

[Aaron Train [:aaronmt]]

Which build/device/OS? Reproducible on all forms, or only that Persona login? Steps to reproduce?

Comment 2

[alex_mayorga]

This is happening everywhere, even as I type this bugzilla comment it also happens on m.facebook.com

The screenshots are obtained with Nightly updated as of today running on Free Xperia Project CyanogenMod 10 on a Sony Ericsson Xperia pro.

Please let me know if there's anything needed from the phone.

Comment 3

[alex_mayorga]

Attachment: Screen capture of the issue in Facebook horizontal.

Comment 4

[alex_mayorga]

Attachment: Screen capture of the issue in Facebook vertical.

Comment 5

[alex_mayorga]

Attachment: Screen capture of the issue in bugzilla comment.

Comment 6

[:Margaret Leibovic]

Is this a regression? Does it happen on Aurora/Beta/Release?

Comment 7

[alex_mayorga]

Seems it is a regression as it doesn't reproduce on Release/Beta/Aurora on this phone.

Comment 8

[:Margaret Leibovic]

Let's see if we can get a regression window.

Comment 9

[Brian Nicholson (:bnicholson)]

I see this too in Nightly. It makes the cursor caret pretty much unusable.

Comment 10

[Teodora Vermesan (:TeoVermesan)]

The regression window is:

1.mozilla-central
good build: 04.07.2013 
bad build: 05.07.2013 
-pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dcbbfcdf7bb4&tochange=17fe59f6c54a

2.inbound
good build:1372942018
bad build: 1372943362
-pushlog: 
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0dbc3d79971d&tochange=efa23d6bd3fa

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Looks like DOMWindowUtils.sendQueryContentEvent returns a thing in LayoutDevice pixels, not CSS pixels. We'll have to divide cursor.* by window.devicePixelRatio before using it at [1]. The code at [2] may also be affected, although I'm not entirely sure how that will manifest. We should also update the documentation for sendQueryContentEvent to indicate that it returns things in LayoutDevicePixel space rather than CSSPixel space.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#500
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#334

Comment 12

[Kartikaya Gupta (email:[e-mail])]

CC'ing mbrubeck in case this affects metro as well. Looks like metro also uses these functions.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Haven't tried this locally yet (in the middle of other stuff) but I pushed it to try and will test out the build when that's done. If that works I'll flag for review.

https://tbpl.mozilla.org/?tree=Try&rev=95b3ab7b2b7c

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 774000 [details] [diff] [review]
Patch

Checked the try build, this seems to work as expected.

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 774000 [details] [diff] [review]
Patch

Passing review to jimm -- he knows the Metro version of this best, though neither of us really know this Android code.  (Maybe margaret would like to review it too?)

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 774000 [details] [diff] [review]
Patch

I understand what you're doing here and it looks correct, but I have no way to test it.

Comment 17

[:Margaret Leibovic]

Comment on attachment 774000 [details] [diff] [review]
Patch

This looks good to me. Maybe add some comments where we call sendQueryContentEvent explaining why we need to do this conversion (this code is always kinda confusing, so I like to err on the side of more comments).

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Added the comments and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0de26cfc54bd

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0de26cfc54bd

Comment 20

[alex_mayorga]

The caret is now on the right place in Nightly.

Thank you!

Comment 21

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 25.0a1(2013-07-14)
Device: Samsung Galaxy Tab
OS: Android 4.0.4