514623 - strange behavior after clicking anchor links (doesn't scroll directly to anchor?)

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

See bug 512277 comment 6. Clicking the links in attachment 370215 [details] results in strange behavior that needs to be investigated.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: Sync browser scroll and content scroll

There were two problems:
1) When converting from content scrollbox to browser scrollbox, we must consider the toolbar height.
2) If we jump to an anchor, we updated the browser scrollbox.  If we scroll back up to the link to the anchor, the browser will still think it is at the anchor and will not fire a scroll event.  We fix this by scrolling browser whenever we pan content.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 401350 [details] [diff] [review]
Sync browser scroll and content scroll

Looks good and works on some test case. Good job

Can you file a bug for the "incorrect behavior if page scrolled to y=0" and we can reference it in the comment.

Comment 3

[Benjamin Stover (:stechz)]

Attachment: Flag and scrolling on load

Now we use an ugly flag to ignore scroll events created by us (making sure not to set the flag unless we really really scrolled).  Also, as discussed on IRC, on loading with anchors we scroll to the anchor and float the toolbar.  It disappears upon panning.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Will bug 500732 && bug 479862 be fixed by this?

Comment 5

[Ben Combee (:bcombee)]

Comment on attachment 401499 [details] [diff] [review]
Flag and scrolling on load

+'d with a nit:

move _ignorePageScroll out of the BrowserView.prototype and add it to the BrowserView._init function instead.

This doesn't fix Facebook float bar, BTW... that seems to be related to another browser variable and also is affected by zoom level.

Comment 6

[Benjamin Stover (:stechz)]

Attachment: Take _ignorePageScroll out of prototype

Fix nit

Comment 7

[Ben Combee (:bcombee)]

Comment on attachment 401946 [details] [diff] [review]
Take _ignorePageScroll out of prototype

nit fixed

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
https://hg.mozilla.org/mobile-browser/rev/66ba098af8ac

Comment 9

[Aakash Desai [:aakashd]]

I'm still getting some funky anchor link behavior via http://quality.mozilla.org/bugs-life-walkthrough . 

Clicking on an anchor link will move the content pane up to the top of the page and then scroll the page down to the location of anchor.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #9)
> I'm still getting some funky anchor link behavior via
> http://quality.mozilla.org/bugs-life-walkthrough . 
> 
> Clicking on an anchor link will move the content pane up to the top of the page
> and then scroll the page down to the location of anchor.

Which is probably what happens, you're just seeing an intermediate step. On long, slow loading pages, I see the same thing in Firefox

Comment 11

[Benjamin Stover (:stechz)]

Attachment: Scroll to top moved to tab startLoading

It's because onLocationChange will scroll the page to the top before the click goes through, erroneously assuming all location changes require a page load.  Moved scrollContentToTop to the network start loading code for tabs, what do you think?

Comment 12

[Benjamin Stover (:stechz)]

Reopening for Aakash's concerns.

Comment 13

[Stuart Parmenter]

    2.78 -    this.contentScrollbox.customDragger.dragMove(0, dy, this.contentScrollboxScroller);
    2.79 +    Browser.contentScrollboxScroller.scrollBy(0, dy);
    2.80 +    bv.scrollBrowserToContent();
    2.81  

This code needs to move sidebars, so the dragMove call is more-correct.  The scrollBy here gets overridden by the call to scrollBrowserToContent -- probably need to merge these things together.  Also fix the issue from 518763.

Comment 14

[Benjamin Stover (:stechz)]

@stuart: Actually I think scrollBrowserToContent is correct.  scrollBy in zoom scrolls the content area, but the scrollBrowserToContent scrolls the browser element to the content's new scroll position.

The other problem is the panning regression.  The entire viewport is rerendered on updating the browser scroll, even if no position:fixed elements need repainting.

Comment 15

[Benjamin Stover (:stechz)]

This patch is being backed out in bug 519231.

Comment 16

[Benjamin Stover (:stechz)]

Comment on attachment 402118 [details] [diff] [review]
Scroll to top moved to tab startLoading

Cleaning up review requests, sorry for spam

Comment 18

[Matt Brubeck (:mbrubeck)]

Fixed in Fennec trunk, fixed in part by bug 598391.