539949 - Regression : Clicking some links with anchors doesn't scroll downward their destinations [allocine.com]

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect, P2)

Description

[Yannick Lavoquer]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.3a1pre) Gecko/20100115 Minefield/3.7a1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.3a1pre) Gecko/20100115 Minefield/3.7a1pre

Verified with a clean profile.
Platform : Linux & windows XP

Reproducible: Always

Steps to Reproduce:
1.Load http://www.allocine.fr/seance/salle_gen_csalle=C0179.html
2.Click any link in section "Films de la semaine" 
Actual Results:  
Updated URL in location bar. Nothing else happens.

Expected Results:  
Should have jumped downward the right part of the document

It works fine when styles are disabled [View > Pages Style > No Style]

Regression Window :
Good : Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20100111 Minefield/3.7a1pre
Built from http://hg.mozilla.org/mozilla-central/rev/120667a01fd2
 
Broken : Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20100112 Minefield/3.7a1pre
Built from http://hg.mozilla.org/mozilla-central/rev/a43e2f7eda8f

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

range based of comment 0:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=120667a01fd2&tochange=a43e2f7eda8f

Comment 2

[Nickolay_Ponomarev]

Based on the range, it's probably bug 526394. Confirming, since I can reproduce this on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a5pre) Gecko/20100425 Minefield/3.7a5pre and can't find a similar-sounding issue in bug 526394's duplicates.

Anyone wants to try creating a simple testcase for this? https://developer.mozilla.org/en/Reducing_testcases

Comment 3

[Brian Polidoro]

From my poking around with this it seems to involve positioning.  With the dom inspector it seems like the anchor is on the page with 3.6 but with trunk it doesn't seem like it's on the page. With DOMi in 3.6 you select the anchor div and it will scroll into view.  On the trunk no scrolling occurs.

One key CSS rule seems to be this one:
.anchor { position:absolute; position:relative\0; top:-22px; }  
The computed style for an anchor that I looked at in both versions in absolute.

My first attempt at reducing to a testcase resulted in something that doesn't work in either trunk or 3.6.  So I guess I'm missing something.

Comment 4

[Brian Polidoro]

Attachment: minimzed testcase

Ok now I got the missing piece: overflow:hidden.

Comment 5

[Timothy Nikkel (:tnikkel)]

So the problem here is in that in PresShell::ScrollFrameRectIntoView if we weren't able to scroll to make any of the target rect visible we bail out, and the target rect in this case comes from an abspos child of an overflow hidden container that is positioned outside of the visible area of the parent.

Changeset 5d8894904869 changed the behaviour here. We used to 1) not bail like above, and 2) not intersect the target rect with the scrollport after scrolling it. The simplest thing to fix this bug is probably just to not bail, and maybe not intersect as well?

Comment 6

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

Yeah, I think both.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 8

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

Comment on attachment 483389 [details] [diff] [review]
patch

The code is OK but something is wrong with your reftest; the test and reference are identical and neither of them triggers scrolling into view?

Comment 9

[Timothy Nikkel (:tnikkel)]

Good catch. The test and reference are different, the test has <div style="position: relative; overflow:hidden;">, the ref has <div style="position: relative;">. The reftest.list file should have "#test2" appended to both the test and reference (I was using a temp reftest.list file to test that it failed without the patch and worked with and didn't copy from it, but instead just wrote the line anew and forgot that part.) I will make those changes in my queue so it is actually a valid test.

Comment 10

[Timothy Nikkel (:tnikkel)]

This causes toolkit/content/tests/widgets/test_mousecapture.xul to fail because it uses some synth mouse moves to drag and select some text and scroll an iframe in the test. But this patch causes every all scrollable ancestors to scroll where they didn't, even the the root content document scroll frame that contains the iframe the test is run in. I'll probably fix it by just resetting the scroll position of the things we don't want scrolled.

Comment 11

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

Bug 601352 will fix that issue, right?

Comment 12

[Timothy Nikkel (:tnikkel)]

The patch in bug 601352 has no effect on the problem in comment 10.

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: fix test

This is what I plan to land to fix the test fail.

Comment 14

[[not reading bugmail]]

Testcase in comment 0 and comment 4 works with latest build:
Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101124 Firefox/4.0b8pre ID:20101124174809

Comment 15

[Timothy Nikkel (:tnikkel)]

That's probably because I landed this bug. I wait until I'm sure a push will stick before marking the relevant bugs resolved, which is why I hadn't changed this bug yet.

Comment 16

[[not reading bugmail]]

I know what you mean and didn't want to get ahead of you, I figured you'd probably update once your ready :)

Comment 17

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/3c680a3b987f

Comment 18

[DavidC]

Not fixed for this site: http://climateprogress.org/2010/11/17/confusionist-judith-curry-wicked-martin-weitzman/#comment-307396

Comment 19

[Timothy Nikkel (:tnikkel)]

Please file a new bug for that with steps to reproduce and cc me, thanks!

Comment 20

[DavidC]

Done: https://bugzilla.mozilla.org/show_bug.cgi?id=614909