854099 - Regression: the top end of anchored element is hidden by dynamic toolbar.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: screenshot

[Environment]
http://hg.mozilla.org/mozilla-central/rev/3825fdbcec62

[Step to reproduce]
Open the link which has anchor id from other page. e.g: https://mozillalabs.com/en-US/#featured-projects

[Result]
The top end of anchor element will be hidden by dynamic toolbar (see screenshot.)

[Expectation]
Firefox should keep the margin as toolbar height on the top end of anchored element. when dynamic toolbar is enabled.

Comment 1

[Chris Lord [:cwiiis]]

So the scroll-to-anchor code should definitely be looking at the fixed position margins of the pres shell - I'll take a look at this.

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Take fixed position margins into account when scrolling to rect in nsPresShell

Expand the target rect when there are fixed position margins so that those margins apply to scroll-to-anchor.

Comment 3

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

Comment on attachment 733297 [details] [diff] [review]
Take fixed position margins into account when scrolling to rect in nsPresShell

Review of attachment 733297 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsPresShell.cpp
@@ +3151,5 @@
>   * 
>   * This needs to work even if aRect has a width or height of zero.
>   */
>  static void ScrollToShowRect(nsIScrollableFrame*      aScrollFrame,
> +                             nsRect                   aRect,

Instead of modifying the parameter, better to add a new local variable and modify that.

@@ +3163,5 @@
>  
> +  // If this is the root scroll frame, make sure to take into account the
> +  // content document fixed position margins. When set, these indicate that
> +  // chrome is obscuring the viewport.
> +  nsIFrame* frame = do_QueryFrame(aScrollFrame);

Should probably modify callers to pass nsIFrame down here so we're not QueryFraming to nsIScrollableFrame and then QueryFraming back.

Comment 4

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Comment on attachment 733297 [details] [diff] [review]
> Take fixed position margins into account when scrolling to rect in
> nsPresShell
> 
> Review of attachment 733297 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsPresShell.cpp
> @@ +3151,5 @@
> >   * 
> >   * This needs to work even if aRect has a width or height of zero.
> >   */
> >  static void ScrollToShowRect(nsIScrollableFrame*      aScrollFrame,
> > +                             nsRect                   aRect,
> 
> Instead of modifying the parameter, better to add a new local variable and
> modify that.

Agreed and done.

> @@ +3163,5 @@
> >  
> > +  // If this is the root scroll frame, make sure to take into account the
> > +  // content document fixed position margins. When set, these indicate that
> > +  // chrome is obscuring the viewport.
> > +  nsIFrame* frame = do_QueryFrame(aScrollFrame);
> 
> Should probably modify callers to pass nsIFrame down here so we're not
> QueryFraming to nsIScrollableFrame and then QueryFraming back.

The only caller of the function does extra work depending on whether it's a scrollable frame or not, so I'd have to add an extra parameter rather than a different one - I'll do that, but let me know if there's an alternative solution you'd like to see.

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Take fixed position margins into account when scrolling to rect in nsPresShell (v2)

Comment 6

[Chris Lord [:cwiiis]]

Try was green with v1 of the patch, v2 is just small structural changes and works fine locally, pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8ed82bee517

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f8ed82bee517

Comment 8

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
-build: Firefox for Android 23.0a1(2013-05-13)
-device: Samsung Galaxy Nexus
-OS: Android 4.1.1