Closed Bug 514623 Opened 11 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED WORKSFORME
fennec1.0b4

People

(Reporter: Gavin, Unassigned)

References

Details

Attachments

(2 files, 2 obsolete files)

See bug 512277 comment 6. Clicking the links in attachment 370215 [details] results in strange behavior that needs to be investigated.
Assignee: nobody → webapps
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.
Attachment #401350 - Flags: review?(combee)
Blocks: 499212
Attachment #401350 - Flags: review?(mark.finkle)
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.
Attachment #401350 - Flags: review?(mark.finkle) → review+
Attached patch Flag and scrolling on load (obsolete) — Splinter Review
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.
Attachment #401350 - Attachment is obsolete: true
Attachment #401499 - Flags: review?(mark.finkle)
Attachment #401350 - Flags: review?(combee)
Attachment #401499 - Flags: review?(combee)
Attachment #401499 - Flags: review?(mark.finkle) → review+
Will bug 500732 && bug 479862 be fixed by this?
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.
Attachment #401499 - Flags: review?(combee) → review+
Fix nit
Attachment #401499 - Attachment is obsolete: true
Comment on attachment 401946 [details] [diff] [review]
Take _ignorePageScroll out of prototype

nit fixed
Attachment #401946 - Flags: review+
pushed:
https://hg.mozilla.org/mobile-browser/rev/66ba098af8ac
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → B4
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.
(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
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?
Attachment #402118 - Flags: review?(mark.finkle)
Reopening for Aakash's concerns.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 518763
Blocks: 496644
    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.
@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.
This patch is being backed out in bug 519231.
Attachment #402118 - Flags: review?(mark.finkle) → review?
Comment on attachment 402118 [details] [diff] [review]
Scroll to top moved to tab startLoading

Cleaning up review requests, sorry for spam
Attachment #402118 - Flags: review?
Assignee: webapps → nobody
No longer blocks: 499212
Duplicate of this bug: 499212
Fixed in Fennec trunk, fixed in part by bug 598391.
Status: REOPENED → RESOLVED
Closed: 11 years ago9 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.