638753 - arstechnica.com - Scroll to comments anchor fails in Firefox 4 (works fine if javascript disabled)

Status: RESOLVED FIXED

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

Description

[beingalink]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre

Clicking to show the comments for any article on ars technica loads the article+comments but doesn't aotofocus the comments so you have to scroll down. The strange thing is: If you go back in history and click the show comments link again, it does autofocus the comments.

Reproducible: Always

Steps to Reproduce:
1.Load http://arstechnica.com/
2.Click the "COMMENTS" link for any article (doesn't matter)
3.article loads but doesn't autofocus the comments
4.Click the back button in the browser
5. Click the same "COMMENTS" link again
6.Now Firefox autofocuses the comments
Actual Results:  
The first time you click to show the comments, firefox doesn't autofocus them.

Expected Results:  
Firefox should autofocus the comments.

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

Does it work if you Fake the User Agent String using "general.useragent.override" in about:config to the String of e.g. Fx 3.6.x?

Comment 2

[beingalink]

(In reply to comment #1)
> Does it work if you Fake the User Agent String using
> "general.useragent.override" in about:config to the String of e.g. Fx 3.6.x?

I changed the the useragent in "general.useragent.override" to "Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1" and this didn't change the behavior. It's still the same as in the steps to reproduce.

Comment 3

[Ed Morley [:emorley]]

Confirmed with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110325 Firefox/4.2a1pre ID:20110325030426

(I read arstechnica too, this has been bugging me for a while)

Will work out the regression range now.

Comment 4

[Ed Morley [:emorley]]

Note: Weirdly enough, if NoScript is currently blocking the JS;
then scrolling occurs fine, if scripts are allowed; scrolling does not occur.

Last good nightly: 2011-02-06 
First bad nightly: 2011-02-07

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5bf94fcd5115&tochange=4c62984f12d1

Includes a large TM merge, so using TM...

Last good nightly: 2011-02-11 
First bad nightly: 2011-02-12

Pushlog: http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=8c7a2550e761&tochange=bce779f4d16b

The TM range is way after the m-c range, so problem changeset presumably originated from m-c.

Other than that, not really sure what could be the cause.

Can someone who can build locally please reduce the range a bit more please.

Oh and see also bug 645075.

Comment 5

[Ed Morley [:emorley]]

For reference, ars is tracking the issue here:
http://arstechnica.com/civis/viewtopic.php?f=3&t=1139680

Comment 6

[Timothy Nikkel (:tnikkel)]

Caused by one of the changesets in this push
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=4c62984f12d1

Comment 7

[Shawn Rhode]

Simon,
I think it is something in patch fffaf649c714 (http://hg.mozilla.org/mozilla-central/rev/fffaf649c714) that caused it as it contains a lot of changes to the scrolling behavior that you made in an attempt to fix the RTL behavior. 
If you look at 645075, there are a number of examples of this behavior breaking. Simply go to http://www.html-5.com/examples/index.html and click on the link for block quote examples labeled "HTML 5 <blockquote> Tag Examples".  Notice that the link has the "#examples" anchor but the page loads at the top and doesn't auto-scroll to the defined anchor point further down in the page.  Many of the other examples on this site exhibit the same behavior.

Shawn Rhode

Comment 8

[Timothy Nikkel (:tnikkel)]

Bug 645075 is likely a different issue as it has a different regression range.

Comment 9

[Shawn Rhode]

I was not attempting to say that the two bugs were the same issue (although based on the description of 645075 provided by the opener, I personally believe they are the same issue).  I was just trying to provide an easy set of tests that seem fairly static over time rather than a page on a general consumer page that might not be as representative of the issue or as easy to reproduce it on.  I verified that the specific page I pointed to above exhibits the behavior reported in this bug before entering the information above.

Shawn Rhode

Comment 10

[Timothy Nikkel (:tnikkel)]

Yes they have the same buggy behaviour but the changeset that made them stop working correctly are different, so likely different issues. If one uses the html-5.com testcases in trying to debug this bug they are likely to not solve this bug.

Comment 11

[Shawn Rhode]

I understand.  I reported this issue to ArsTechnica and opened the thread that Ed linked to above.  I see what you mean about the behavior being slightly different.  In this case, the issue is that the anchor isn't being honored on reload of a page when retrieving additional content on the page using a link.  In the other case, it would seem the issue is that the anchor isn't being honored when going to a page the first time with a URL that contains the anchor in it.  Similar outcomes but two slightly different behaviors.  

Carry on.

Shawn

Comment 12

[Simon Montagu :smontagu]

This is caused by the change at https://bugzilla.mozilla.org/attachment.cgi?id=510099&action=diff#a/layout/generic/nsGfxScrollFrame.cpp_sec12, which I wasn't quite sure about the correctness of (bug 508816 comment 73).

The solution is probably to make mRestorePos be a logical scroll position as David already suggested.

Comment 13

[Simon Montagu :smontagu]

Attachment: Patch

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

>@@ -2237,18 +2237,23 @@ nsGfxScrollFrameInner::ScrollToRestoredP
>   // logical scroll position, but we scroll to the physical scroll position in
>   // all cases
> 
>   // if we didn't move, we still need to restore
>   if (GetLogicalScrollPosition() == mLastPos) {
>     // if our desired position is different to the scroll position, scroll.
>     // remember that we could be incrementally loading so we may enter
>     // and scroll many times.
>+    if (mRestorePos != GetLogicalScrollPosition()) {

It would be nice to avoid calling GetLogicalScrollPosition() twice here.  And before bug 508816, the code actually did that -- so could you reintroduce the |scrollPos| variable that was here before bug 508816?  (Except now it's GetLogicalScrollPosition?)


r=dbaron with that

Comment 15

[Simon Montagu :smontagu]

(In reply to comment #14)
> It would be nice to avoid calling GetLogicalScrollPosition() twice here.  And
> before bug 508816, the code actually did that -- so could you reintroduce the
> |scrollPos| variable that was here before bug 508816?  (Except now it's
> GetLogicalScrollPosition?)

Or I could get the same result without the extra variable, thusly:
  if (mRestorePos != mLastPos)
right?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, that's fine, although it might be nice to do:

  if (mRestorePos != mLastPos /* GetLogicalScrollPosition() */) {

Comment 17

[Simon Montagu :smontagu]

Attachment: Patch with that change

Transferring r=dbaron.

Comment 18

[Simon Montagu :smontagu]

checkin-needed because the tree never seems to be open at times when I'm available to checkin.

Comment 19

[Simon Montagu :smontagu]

http://hg.mozilla.org/projects/cedar/rev/c9a081747932

Comment 20

[Rob Simpson]

Additional test case, possibly with a different regression range.

>>Quote Shawn Rhode>> In this case [Bug 638753], the issue is that the anchor isn't being honored on reload of a page when retrieving additional content on the page using a link. In the other case [Bug 645075], it would seem the issue is that the anchor isn't being honored when going to a page the first time with a URL that contains the anchor in it.  Similar outcomes but two slightly different behaviors.

In addition to the "anchor isn't being honored on reload of a page when retrieving additional content" behavior on a single story, the Ars Technica site also exhibits the same "anchor isn't being honored when going to a page the first time" as http://www.HTML-5.com/ as described in Bug 645075.

Test case (not sure if this is a test case for this bug or Bug 645075 yet - depends on the regression range):

1. Go to the Ars Technica home page at http://www.ArsTechnica.com/.
2. Click on the "COMMENT" link under any story.
   -> The first time the page loads it stays scrolled to the top.
3. Position the cursor in the address bar and hit Return.
   -> Now it scrolls to where you would have expected.

Comment 21

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c9a081747932

Comment 22

[Timothy Nikkel (:tnikkel)]

(In reply to comment #20)
> Test case (not sure if this is a test case for this bug or Bug 645075 yet -
> depends on the regression range):
> 
> 1. Go to the Ars Technica home page at http://www.ArsTechnica.com/.
> 2. Click on the "COMMENT" link under any story.
>    -> The first time the page loads it stays scrolled to the top.
> 3. Position the cursor in the address bar and hit Return.
>    -> Now it scrolls to where you would have expected.

If I'm not mistaken those are the steps to reproduce this bug. If that is not fixed in tomorrows nightly then speak up.

Comment 23

[Rob Simpson]

Right you are, sorry. However, I knew one of the test scenarios wasn't clearly spelled out, and having what appeared to be two different test scenarios with the same outcome was getting confusing.

So this bug and Bug 645075 have similar test cases, for the "anchor isn't being
honored when going to a page the first time" scenario.

The test case that's not spelled out here _was_ spelled out in the Ars Technica forum post (http://ArsTechnica.com/civis/viewtopic.php?f=3&t=1139680).

Although it is being described as "anchor isn't being honored on reload of a page when retrieving additional content on the page using a link" scenario, I believe that's not quite accurate - when I go through the test case as described, the link in the address bar actually changes - after clicking "Click here to view the ... comments on this story", the query "?comments=1#comments-bar" is appended to the original story URL.

1. Go to the Ars Technica home page at http://www.ArsTechnica.com/.
2. Click on the title of any story to go to the page for that one story.
3. Click on "Click here to view the ... comments on this story", which should expand the comments _and_ scroll to the beginning of those comments.
   -> Instead it scrolls back to the top of the article (the longer the article the more noticeable the issue)
4. If you position the cursor in the address bar and hit Return, it scrolls to where you would have expected.

Comment 24

[Rob Simpson]

>> If that is not fixed in tomorrows nightly then speak up.

Both test cases ("COMMENT" on home page and the "Click/to view/comments" for the Ars Technica site (Feb. 2011 regression) now work. Thanks!

The test case for Bug 645075 (June 2010 regression) still has the "anchor isn't being honored" issue. Drat.

Comment 25

[Shawn Rhode]

I tested using the x64 nightly build from overnight 3-28:3-29 on Windows 7 x64 and it didn't work for me when going to the Ars Technica site as noted above in the test cases.

Comment 26

[Ed Morley [:emorley]]

WFM using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110330 Firefox/4.2a1pre ID:20110330030437

Sure the build you tried included that changeset?

What platform?

Comment 27

[Shawn Rhode]

4.2a1pre ID:20110328030202 
Is what I used.  Windows 7 Professional 64bit

Comment 28

[Shawn Rhode]

It looks like a later build fixes it.  Just tested with Firefox/4.2a1pre ID:20110329030205 and didn't see any issues at the Ars Technica site when using the "View Comments" link.  Appears to be resolved in the latest nightly.

Comment 29

[Rob Simpson]

Shawn,

For 32-bits, it was in the 3-29 nightly, so the 3-28 64-bit build probably wouldn't have had the fix get the current (3-30) one that Ed says works for him.

Comment 30

[Shawn Rhode]

I grabbed the nightly after you posted that it was working in the nightly, so it is surprising that the nightly I grabbed didn't have the fix.  Either way, it does appear that the latest nightly works for Ars Technica so it was likely just a mixup on my part when grabbing the nightly. 

Shawn