703241 - scrollIntoView should take transforms into account

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Martin Prebio]

Attachment: example.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111104165243

Steps to reproduce:

Open the attachment.
Resize your browser and scroll down and right to the input box.
Click into the input box and type something.


Actual results:

The browser scrolls to the left upper edge where the input box should be without the -moz-transform.


Expected results:

No scrolling

Comment 1

[Martin Prebio]

This may be related to Bug #455166 (https://bugzilla.mozilla.org/show_bug.cgi?id=455166) - The Select Box in the testcase.html doesn't work right too.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  Should scrollIntoView take transforms into account?  Seems like it...

Comment 3

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

Yes!

Comment 5

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

Attachment: fix

Comment 6

[Mats Palmgren (inactive)]

Attachment: Testcase, relative inline

Chrome and Opera scrolls this to the relative position showing
the text "HERE", which seems correct to me.  I'm guessing our
scroll-to-the-top-of-the-line quirk kicks in here.  Seems like
we shouldn't do that if aFrame or any of its ancestors up to
the first block is positioned/transformed.

STR: load URL#t in a window with narrow height

Comment 7

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Same STR, works OK (scrolls to top of the line)

Comment 8

[Mats Palmgren (inactive)]

Attachment: Same as Testcase #3, with an added :first-line style

Same STR, unlike Testcase #3 this scrolls to the top of the frame.
I'm guessing we should also check for nsGkAtoms::lineFrame in
the ancestor loop?

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 633950 [details] [diff] [review]
fix

Looks fine, but I'd like to see a follow-up bug for the additional
cases mentioned above (unless you want to address them here).
r=mats

Comment 10

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

Attachment: extra fix

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 635245 [details] [diff] [review]
extra fix

Looks great, thanks.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/243d341c4edd
https://hg.mozilla.org/integration/mozilla-inbound/rev/215cc8b416a2

Comment 13

[Mounir Lamouri (:mounir)]

Backed out due to perma orange on Moth, see:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=8661c74deeb5

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef1c6a327b0d

Comment 14

[Mathias Panzenböck]

So do I understand this right: There was a fix but it wasn't good for some reason and so it was removed again. When can we expect this to be fixed? Is there a workaround?

Comment 15

[Bart OL-NL]

Additional case to test: content editable where scale transform is present.
In current situation scroll position will be all over the place once moving out (either typing or navigating) of the 'bottom' of the screen (have a content that is larger then the viewport). Manually moving the cursor into view using mouse doesnt help (when typing scrollpos is erratically moved again, so cant even see where typing occurs).
Underlying cause is this bug as far as I can tell.

Comment 16

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

Still has a couple of test failures on Mac:
https://tbpl.mozilla.org/?tree=Try&rev=a155d3fffd3c

ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_focus.xul | scroll position before focus - got 35, expected 0

ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug791616.html | Target should not have scrolled - got 114.60000610351562, expected 115.39999389648438

I'll try to debug these and get this landed.

Comment 17

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

Attachment: Part 3: handle aFrame == aAncestor

Getting the transform between a frame and itself completely fails. We should fix that. (We could also require aAncestor to be a "proper" ancestor, but I think it's better to handle this case.)

Comment 18

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

Attachment: Part 4: don't allow frame == container

The previous patch fixed a test failure where DoScrollContentIntoView called on a scrollable frame used that frame as 'container', and then later on we called nsLayoutUtils::TransformFrameRectToAncestor and it blew up because GetTransformToAncestor couldn't handle the frame and ancestor being the same.

Really, though, DoScrollContentIntoView should not allow 'container' to be the frame itself. Note that this doesn't affect the actual scrolling of content into view, which works up through all scrollable ancestors anyway. 'container' is more like a reference frame into which we transform the union of all the rects for the frames of the element, so in some ways it doesn't matter what it is. I think this is more logical than other choices though.

Comment 19

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

https://tbpl.mozilla.org/?tree=Try&rev=7400ade37e84

Comment 20

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

It's green!

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 672167 [details] [diff] [review]
Part 3: handle aFrame == aAncestor

It does seem somewhat sloppy of a caller to call GetTransformToAncestor
with aFrame == aAncestor and then presumably do a bunch of unnecessary work.
Possibly it's also not what the caller intended to do, like in Part 4?
So asserting that aFrame != aAncestor seems like the slightly better
alternative to me.

r=mats either way

Comment 22

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

(In reply to Mats Palmgren [:mats] from comment #21)
> It does seem somewhat sloppy of a caller to call GetTransformToAncestor
> with aFrame == aAncestor and then presumably do a bunch of unnecessary work.
> Possibly it's also not what the caller intended to do, like in Part 4?
> So asserting that aFrame != aAncestor seems like the slightly better
> alternative to me.

I lean the other way. In callers that usually pass an ancestor that's different to the frame, but sometimes (correctly) pass the frame as the ancestor, it's a pain to have to write special-case code for that.

We could create a separate GetTransformToProperAncestor function for some callers, like this one I guess, but I don't see that as particularly useful.

Comment 23

[Mats Palmgren (inactive)]

Yeah, we should rename it to GetTransformToProperAncestor in that case.
My assumption was that we currently have no callers that needs
GetTransformToAncestor, since as you said it fails when aFrame ==
aAncestor, but maybe I misunderstood?

Comment 24

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

I don't know, I'd have to check all the callers and run some tests.

I think I only need part 4 to finish this bug. I've filed bug 802925 to deal with fixing GetTransformToAncestor.

Comment 25

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf63fe2c1b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f9518910e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4033a060e7af

Comment 26

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

There are actually some cases where GetTransformToAncestor could be called with aFrame == aAncestor, so I wontfixsed bug 802925 and landed part 3.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d5f464dc1c0

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fbf63fe2c1b0
https://hg.mozilla.org/mozilla-central/rev/f1f9518910e8
https://hg.mozilla.org/mozilla-central/rev/4033a060e7af
https://hg.mozilla.org/mozilla-central/rev/9d5f464dc1c0