1025815 - Incorrect result of document.caretPositionFromPoint after CSS transform

Status: RESOLVED FIXED

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

Description

[inferrna]

Attachment: test.html based on https://developer.mozilla.org/en-US/docs/Web/API/document.caretPositionFromPoint

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140608211622

Steps to reproduce:

Trying to get caret position offset inside element after css transform.scale() was applied.
example is in attachment. In chromium it works fine.


Actual results:

Returning value is incorrect and depends on scaling factor.


Expected results:

Returning value must be the same as without transform.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Probably related to bug 863618.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This bug needs its own fix in nsIDocument::CaretPositionFromPoint.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Additional test case

Comment 4

[Kestrel]

Attachment: Bug 1025815 - caretPositionFromPoint needs to account for CSS transforms.

Review commit: https://reviewboard.mozilla.org/r/238370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238370/

Comment 5

[Andrew Overholt [:overholt]]

Comment on attachment 8969602 [details]
Bug 1025815 - caretPositionFromPoint needs to account for CSS transforms.

Boris is a better reviewer here.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8969602 [details]
Bug 1025815 - caretPositionFromPoint needs to account for CSS transforms.

https://reviewboard.mozilla.org/r/238370/#review245186

r=me with the minor test nit below.

Please let me know if you won't be able to update the patch to that nit and I can do it.

::: dom/base/test/test_caretPositionFromPoint.html:113
(Diff revision 1)
>  
> +    // Check the first and last characters of the transformed div.
> +    var test7Element = document.getElementById('test7');
> +    var test7Rect = test7Element.getBoundingClientRect();
> +    checkOffsetsFromPoint(Math.round(test7Rect.left + 1), Math.round(test7Rect.top + 1), 0, 'test7');
> +    checkOffsetsFromPoint(Math.round(test7Rect.left + test7Rect.width - 1), Math.round(test7Rect.top + 1), 13, 'test7');

How about "test7Rect.right - 1" instead of manually adding up left+width?

Comment 7

[Kestrel]

Comment on attachment 8969602 [details]
Bug 1025815 - caretPositionFromPoint needs to account for CSS transforms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238370/diff/1-2/

Comment 8

[Kestrel]

Can you please trigger a try build for me?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yep, triggered https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d8bfd7ebe67cb49c00a913facc1aed6cfef5130

If that succeeds (and I expect it will), I'll make sure this lands.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK, there are definitely some test failures.  Mostly in test_caretPositionFromPoint.html but also in test_bug864595.html.

Comment 11

[Kestrel]

Comment on attachment 8969602 [details]
Bug 1025815 - caretPositionFromPoint needs to account for CSS transforms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238370/diff/2-3/

Comment 12

[Kestrel]

Thanks for running the try for me.

TransformRootPointToFrame uses _global_ coords so it does not work correctly in iframes (I was running the tests directly unframed), should be using GetEventCoordinatesRelativeTo. I also had to move test7 so it wasn't clipped by the frame. The failing tests now pass locally when run in iframes using --verify.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Kicked off another try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ca71aaff219e39e27e2d3f4b073a87cef63322e&selectedJob=176508048

I need to think a bit about the actual patch (and whether I'm even the right reviewer for it).  Having to go through widgets here is a bit odd to me.

Comment 14

[Kestrel]

Thanks! I couldn't find any layout helper functions that do this transformation without using widgets. The approach here is the same as nsDOMWindowUtils::SelectAtPoint which already works with CSS transform.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8969602 [details]
Bug 1025815 - caretPositionFromPoint needs to account for CSS transforms.

https://reviewboard.mozilla.org/r/238370/#review247668

Sorry for the lag here.... This looks good, modulo the nits below.

::: dom/base/nsDocument.cpp:9998
(Diff revision 3)
>        nsLayoutUtils::IGNORE_PAINT_SUPPRESSION | nsLayoutUtils::IGNORE_CROSS_DOC);
>    if (!ptFrame) {
>      return nullptr;
>    }
>  
> -  // GetContentOffsetsFromPoint requires frame-relative coordinates, so we need
> +  // Require frame-relative coordinates for GetContentOffsetsFromPoint.

How about "We need frame-relative...."

::: dom/base/nsDocument.cpp:10001
(Diff revision 3)
>    }
>  
> -  // GetContentOffsetsFromPoint requires frame-relative coordinates, so we need
> -  // to adjust to frame-relative coordinates before we can perform this call.
> -  // It should also not take into account the padding of the frame.
> -  nsPoint adjustedPoint = pt - ptFrame->GetOffsetTo(rootFrame);
> +  // Require frame-relative coordinates for GetContentOffsetsFromPoint.
> +  nsPoint aOffset;
> +  nsCOMPtr<nsIWidget> widget = nsContentUtils::GetWidget(ps, &aOffset);
> +  LayoutDeviceIntPoint aPoint =

This should probably have a better name than aPoint.  Maybe pointRelativeToWidget?

::: dom/base/test/test_caretPositionFromPoint.html:125
(Diff revision 3)
>  <body onload="doTesting();">
>  <div id="a" contenteditable><span id="test1">abc, abc, abc</span><br>
>  <span id="test2" style="color: blue;">abc, abc, abc</span><br>
>  <textarea id="test3">abc</textarea><input id="test4" value="abcdef"><br><br>
>  <marquee>marquee</marquee>
> +<div id="test7" style="transform: translate(140px, -20px); display: inline-block;">abc, abc, abc</div>

Please document why those specific translate() values are used.

Comment 16

[Kestrel]

Comment on attachment 8969602 [details]
Bug 1025815 - caretPositionFromPoint needs to account for CSS transforms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238370/diff/3-4/

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8969602 [details]
Bug 1025815 - caretPositionFromPoint needs to account for CSS transforms.

https://reviewboard.mozilla.org/r/238370/#review249056

Comment 18

[Boris Zbarsky [:bzbarsky]]

Hmm.  I somehow missed the mail for comment 16....   I don't know how that happened.

Triggered another try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9bdf625075fef728146bfc441ad7b49a6b7c11b

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f365bf91369c
caretPositionFromPoint needs to account for CSS transforms. r=bz

Comment 20

[Boris Zbarsky [:bzbarsky]]

Thank you for fixing this!

Comment 21

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/f365bf91369c

Comment 22

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Test case showing change in result

Thank you very much for fixing this.

Interestingly I notice this affects the result even when transforms are not in use. For example, in the attached test case, if we call caretPositionFromPoint at a point exactly half way across a character, it will now return the *next* character whereas previously it returned the character under the cursor.

That is, before the patch it would report 0 but now it will report 1.

I don't think that's a problem but I'm adding this test case in case it breaks anyone's content (it broke some automated tests I have for this).

Comment 23

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Further test case showing change in result for SVG text

There is a similar change in result for SVG text. In this case taking the point exactly between two characters (according to Range.getBoundingClientRect) returns the *previous* character after this change, where it used to return the *next* character.

Comment 24

[Kestrel]

Thanks for sharing details about your test breakage Brian. Non-transform cases are expected to be affected by this change and it now behaves consistently with caret placement using the mouse cursor.

Here's what I get with your tests:

> Ubuntu 18.04
> FF60  HTML: 0  SVG: 1
> FF62  HTML: 1  SVG: 1

> Windows 10 VirtualBox
> FF60  HTML: 0  SVG: 1
> FF62  HTML: 0  SVG: 1

I cannot reproduce your results with the SVG case or with Windows 10 VirtualBox. In the Linux HTML case it varies with the character, eg 'A' gives offset 0 while 'Z' gives offset 1. The 'A' middle position is 14px while 'Z' is 13.5px, so it appears subpixel positions are being rounded up causing the offset to go to the next character. Rounding is expected with this change due to the conversion from app units to device pixels which is necessary to utilize functions that account for CSS transform.