Closed
Bug 703241
Opened 13 years ago
Closed 12 years ago
scrollIntoView should take transforms into account
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: mp, Assigned: roc)
References
Details
Attachments
(8 files)
363 bytes,
text/html
|
Details | |
7.95 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
495 bytes,
text/html
|
Details | |
445 bytes,
text/html
|
Details | |
497 bytes,
text/html
|
Details | |
1.61 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
1.97 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
1.33 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•13 years ago
|
||
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•13 years ago
|
||
Hmm. Should scrollIntoView take transforms into account? Seems like it...
Assignee | ||
Comment 3•13 years ago
|
||
Yes!
![]() |
||
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Component: Layout: Form Controls → Layout
Ever confirmed: true
QA Contact: layout.form-controls → layout
Summary: Site scrolling wrongly if focusing a form element in a -moz-transform: translate() container → scrollIntoView should take transforms into account
Updated•13 years ago
|
Attachment #575143 -
Attachment mime type: text/plain → text/html
Updated•13 years ago
|
Status: NEW → UNCONFIRMED
Component: Layout → Layout: Form Controls
Ever confirmed: false
OS: Mac OS X → All
Hardware: x86 → All
Version: 8 Branch → Trunk
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Component: Layout: Form Controls → Layout
Ever confirmed: true
Assignee | ||
Comment 5•12 years ago
|
||
Assignee: nobody → roc
Attachment #633950 -
Flags: review?(matspal)
Comment 6•12 years ago
|
||
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•12 years ago
|
||
Same STR, works OK (scrolls to top of the line)
Comment 8•12 years ago
|
||
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•12 years ago
|
||
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
Attachment #633950 -
Flags: review?(matspal) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #635245 -
Flags: review?(matspal)
Comment 11•12 years ago
|
||
Comment on attachment 635245 [details] [diff] [review] extra fix Looks great, thanks.
Attachment #635245 -
Flags: review?(matspal) → review+
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/243d341c4edd https://hg.mozilla.org/integration/mozilla-inbound/rev/215cc8b416a2
Comment 13•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 16•12 years ago
|
||
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.
Assignee | ||
Comment 17•12 years ago
|
||
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.)
Attachment #672167 -
Flags: review?(matspal)
Assignee | ||
Comment 18•12 years ago
|
||
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.
Attachment #672172 -
Flags: review?(matspal)
Assignee | ||
Comment 19•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=7400ade37e84
Assignee | ||
Comment 20•12 years ago
|
||
It's green!
Comment 21•12 years ago
|
||
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
Attachment #672167 -
Flags: review?(matspal) → review+
Updated•12 years ago
|
Attachment #672172 -
Flags: review?(matspal) → review+
Assignee | ||
Comment 22•12 years ago
|
||
(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•12 years ago
|
||
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?
Assignee | ||
Comment 24•12 years ago
|
||
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.
Assignee | ||
Comment 25•12 years ago
|
||
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
Assignee | ||
Comment 26•12 years ago
|
||
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•12 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•