716403 - (dynamic-toolbar) Request to hide the navigation bar when scrolling down content

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[bug.zilla]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Steps to reproduce:

Scrolled down a page.


Actual results:

Navigation bar did not hide, like the current v.10 of Firefox (i.e. the non-native version). This leads to too much wasted space.


Expected results:

Navigation bar should hide when scrolling down and re-appear when scrolling all the way back to the top of the page.

Comment 1

[Andreea Pod]

This is reproducible on Native Nightly: Firefox 14.0a1 build from 2012-04-03.

Comment 2

[microrffr]

Looks like Bug 766957 might fix this. Hooray!

Comment 3

[Aaron Train [:aaronmt]]

(In reply to microrffr from comment #2)
> Looks like Bug 766957 might fix this. Hooray!

It's a design request for Reader Mode; it's new to Nightly; so nothing related to this.

Comment 4

[bug.zilla]

Actually, there might be a better way to make the URL bar reappear, which is to click anywhere at the top of the page (on a thin slice of the screen).

When you click there the URL bar will reappear. This will prevent having to scroll all the way up to the top.

Have a look at the blackberry default browser.

Comment 5

[Ian Barlow (:ibarlow)]

The desired UX we would like here is

1. Hide the header when you start scrolling down a page
2a. Reveal the header when you scroll back to the top of the page
2b. Reveal the header at any time, if you scroll down from anywhere in the top 25% of the screen. For reference, ICS's stock browser has a similar, albeit less reliable interaction.

I'll post some mockups / animations shortly. 

BTW, this behaviour should apply to Bug 766957 as well.

Comment 6

[Ian Barlow (:ibarlow)]

Hi folks, here is a series of animated mockups the describe the desired title bar scrolling interactions: http://cl.ly/3d461B1v3E3g0W3R3H0Y

1. Scrolling up after a page has fully loaded should move the title bar off the top of the screen (note: we should always keep the header on screen while the page is loading)

2. Scrolling down in the bottom 75% of the screen should scroll page content down, but not reveal the title bar. 

3. Scrolling down in the top 25% of the screen should scroll page content down *and* reveal the title bar. 

----

We'd like to get a test build of this running and do some early user testing to see how this feels to people. Failing this, we have a range of other possible interactions we could also explore for revealing the title bar:

1. Area-based scrolling (what we are proposing here)
2. Velocity-based scrolling (pull down quickly to reveal the title bar)
3. Distance-based scrolling (pull down a certain distance and then reveal title bar)
4. Tap screen to reveal title bar

Comment 7

[bug.zilla]

Thanks Ian,

as we're on Windows can you please export the "key" file as a PPT or some other compatible format?

All 4 methods have potential, but may need a bit of testing first - let us know if there are any builds.

I guess 1 and 4 would be the easiest day to day, i.e. require the least effort from users and avoid any mistakes.

One thing to consider: hide the status bar, or at least provide an option to.
It's only a small part of the screen but real estate is at a premium especially on small screens plus it can be a bit distracting.

Comment 8

[Ian Barlow (:ibarlow)]

Here, I made a little movie from the slides here: https://dl.dropbox.com/u/2182500/mobile%20header%20interactions.mov

Comment 9

[Ian Barlow (:ibarlow)]

Here, I made a little movie from the slides: https://dl.dropbox.com/u/2182500/mobile%20header%20interactions.mov

Comment 11

[bug.zilla]

Thanks Ian,

I think (1) is the best option here as:

(2) Velocity based: too easy to trigger by accident
(3) Distance based: too easy to trigger by accident
(4) Tap screen: this one has potential but again could end up clicking on a link instead especially on sites that have a lot of tightly-packed content

Comment 12

[Brian Nicholson (:bnicholson)]

Here's the behavior I noticed in the stock ICS browser, which feels pretty nice:
* Scrolling down always hides the header
* Scrolling up always reveals the header
* If the header is revealed but the user is not at the top of the page, the header is automatically hidden again after a few seconds

This has the advantage of being able to show the header at any time - even when you're at the bottom of the page - but it also doesn't take up space since it auto-hides.

Comment 13

[Ian Barlow (:ibarlow)]

> * Scrolling down always hides the header
> * Scrolling up always reveals the header
> * If the header is revealed but the user is not at the top of the page, the
> header is automatically hidden again after a few seconds

This also seems reasonable. (This isn't always the behaviour on the stock browser, though, the header doesn't always appear when you scroll down. It's hard to tell if it is velocity based, area based, or based on something else, but it's kind of annoying because it feels so inconsistent.)

I would be happy to try an implementation based on Brian's suggestion - anyone want to take this? :)

Comment 15

[Ian Barlow (:ibarlow)]

Hi there, any progress on this? It's a feature that would greatly benefit both normal and Reader browsing.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

I talked to sriram and he said he's working on this. This is a quick WIP for him (or anyone else) who wants to take this. This uses messages from the PanZoomController to pan the urlbar/content.

about:home is a bit broken with it, and the panning of the urlbar is a bit jerky/sometimes fails, I think because of some rounding errors moving from Gecko's floating point scrolling to Androids integer only scrolling.

I think we need to also trigger the urlbar to lock to either "on" or "off" when the user releases their finger.

Comment 17

[Wesley Johnston (:wesj)]

I realized yesterday that this prototype doesn't handle pages that prevent or aren't big enough to actually pan very well. Looks like we want to go back to have an old "escape" header as well, so that pages like Google Maps work.

Comment 18

[Chris Lord [:cwiiis]]

Attachment: WIP Patch, rebased

This is wesj's patch, rebased.

I love how simple and elegant this is, and it works surprisingly well. Unless we use TextureView though, I don't think this method will ever provide truly synchronous page and header movement, which is always going to look a bit bad.

I think to do this (and other animations) properly, we want to be able to render the UI into a GL texture and let it use the same drawing loop that we use to render page content. I don't know enough about Android UI atm to know if this is possible, and if it is, how many versions back it remains possible...

Alternatively, we want TextureView not to incur a performance penalty, but that's ICS+ only as far as I'm aware.

We can discuss this on IRC, I'm sure wesj and lucasr have better ideas on this matter than I do.

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: wip

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

I uploaded my current work in progress, but I think it's time for me to raise the white flag on this one. The current patch is busted when viewport height=page height, and I'd guess fullscreen is busted too. I think the way forward may be to just do the toolbar in xul, sadly.

Comment 22

[bug.zilla]

Hi, has there been any progress on this?

Comment 23

[bug.zilla]

This add on does something similar:
https://addons.mozilla.org/en-us/android/addon/auto-fullscreen/

Comment 24

[Morrison Cole]

I have a working proof of concept, but it hasn't been rebased to the latest trunk in a while. 

I'll upload the WIP at the next opportunity, although I think (as snorp noted) this approach has been scrapped.

Comment 25

[Wesley Johnston (:wesj)]

Attachment: WIP Rebased

Just updating my old patch a bit. This doesn't bother to scroll the GeckoView at all. Just makes it fullscreen and puts the header over the top of it. Also has fixes to animate things and lock the header either entirely on or off when you lift your finger. The interaction isn't bad, even on old phones. About:home needs work. Wanted to save the patch somewhere.

Comment 26

[Chris Lord [:cwiiis]]

(In reply to Wesley Johnston (:wesj) from comment #25)
> Created attachment 702950 [details]
> WIP Rebased
> 
> Just updating my old patch a bit. This doesn't bother to scroll the
> GeckoView at all. Just makes it fullscreen and puts the header over the top
> of it. Also has fixes to animate things and lock the header either entirely
> on or off when you lift your finger. The interaction isn't bad, even on old
> phones. About:home needs work. Wanted to save the patch somewhere.

This is really nice :) Assuming, UX-wise, that this is similar to the experience we want, I think this is how we should go about it... I still don't like that we're breaking the layers paradigm completely with this, but perhaps this can go hand-in-hand with the UI refresh. Some UX input would be awesome here. This method wouldn't allow the page to appear over the URL bar, or for completely synchronous animation of the page and the bar, but I think those things are both ok.

Wes, I was going to look at this, but you've already made good progress - do you want to continue with it, or would you like me to pick up from your patch?

Comment 27

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Scroll header off the top

This is just a tidied up version of wesj's patch with some of the workings changed. Instead of modifying TouchEventHandler and PanZoomController, this registers BrowserApp as a touch event interceptor and diverts the panning events that way. It provides the same behaviour as wesj's patch with my limited testing.

I've also factored out the animation code into the toolbar itself, to make it more easily usable in multiple places (I'm basing some gamepad control patches on this, so it made sense).

Sorry for the liberal r?/f?. I've asked for wesj's feedback to make sure I didn't misunderstand the original patch, kats' review as he knows the interactions of touch events well and lucasr as he knows the Android UI bits well. Feel free to un-? yourself if you think I've gone overboard :)

This will be a part 1, I intend to change the behaviour slightly (especially with short pages/pages with meta viewports) and add gamepad controls in follow-up patches.

Comment 28

[Chris Lord [:cwiiis]]

Realised I forgot the toggleChrome bits... I'd still appreciate review, will upload an updated patch on Monday taking into account any comments between now and then.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 706509 [details] [diff] [review]
Part 1 - Scroll header off the top

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

This looks pretty good. I like the way the MotionEvent flow works. Various comments below. Changing r? to f+ since you said you'd have an updated patch later anyway.

::: mobile/android/base/BrowserApp.java
@@ +113,1 @@
>                          hideAboutHome();

In the else case, do we want to hide the toolbar with some delay if the tab is done loading? i.e. if you have two tabs open - about:home and a loaded page, and you switch from about:home (which has the toolbar showing) to the loaded page, I assume we want to slide out the toolbar.

@@ +185,5 @@
>          updateAboutHomeTopSites();
>      }
>  
>      @Override
> +    public boolean onInterceptTouchEvent(View view, MotionEvent event) {

I believe this function needs to replace all "return false"s with "return super.onInterceptTouchEvent(view, event)" calls.

@@ +206,5 @@
> +            float y = event.getY();
> +            int dy = mToolbarScrollStartY + Math.round(mTouchStartY - y);
> +            dy = Math.max(0, Math.min(dy, toolbarHeight));
> +
> +            toolbarView.scrollTo(0, dy);

This part is a bit hard to follow. More comments/variable renaming would be nice. For example, I would assume that a variable called "dy" is a delta and so it seems wrong that it gets passed to a scrollTo function (as opposed to a scrollBy).

@@ +217,5 @@
> +            if (dy < toolbarHeight && dy > 0) {
> +                return true;
> +            }
> +            return false;
> +        } else {

I don't think the unconditional else here is correct; you could get something like ACTION_POINTER_DOWN and you don't want to animate in that case. I think you should explicitly check for ACTION_UP and ACTION_CANCEL. Probably best to pull out the getActionMasked() into a temp var at the start of the function too.

@@ +275,5 @@
>  
>          super.onCreate(savedInstanceState);
>  
>          LinearLayout actionBar = (LinearLayout) getActionBarLayout();
> +        mMainLayout.addView(actionBar, 1);

Not sure what this change implies.

::: mobile/android/base/BrowserToolbar.java
@@ +427,5 @@
>  
> +    public void animateVisibility(boolean show, long delay) {
> +        if (mVisibility != ToolbarVisibility.INCONSISTENT &&
> +            ((delay > 0) == (mDelayedVisibilityTask != null)) &&
> +            (show == (mVisibility == ToolbarVisibility.VISIBLE))) {

Comment this please. I can't tell if it's right or not.

@@ +438,5 @@
> +        mVisibilityAnimator = new PropertyAnimator(VISIBILITY_ANIMATION_DURATION);
> +        mVisibilityAnimator.attach(mLayout, PropertyAnimator.Property.SCROLL_Y,
> +                                   show ? 0 : mLayout.getHeight());
> +        if (delay > 0) {
> +          final PropertyAnimator a = mVisibilityAnimator;

You don't need this. Inner classes can reference outer class variables directly. You'd only need need this if mVisibilityAnimator were local to this function.

@@ +444,5 @@
> +              public void run() {
> +                  a.start();
> +              }
> +          };
> +          mLayout.postDelayed(mDelayedVisibilityTask, delay);

Indentation

@@ +464,5 @@
> +        }
> +    }
> +
> +    public boolean getVisibility() {
> +        return mVisibility == ToolbarVisibility.VISIBLE;

Looks like this function should really be called isVisible(), or maybe isFullyVisible().

::: mobile/android/base/GeckoApp.java
@@ +2673,5 @@
> +    public boolean onTouch(View view, MotionEvent event) {
> +        if (event == null)
> +            return true;
> +
> +        int action = event.getAction();

Might as well make this a event.getActionMasked() call and remove the later & operations.

Comment 30

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Scroll header off the top

This differs quite a bit from the previous patch in that it doesn't block events ever, but modifies them to remove the vertical movement component (as suggested on IRC, with the alternative being some method to withhold vertical scrolling in PanZoomController).

Comment 31

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Scroll fixed layers with the toolbar

This scrolls fixed layers with the toolbar, so top-bars on pages aren't obscured when the toolbar is visible. Lucas was also interested in the possibility of adding margins to fixed layers for reader mode.

Comment 32

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Make sure the top of the page is accessible

This adds the ability to add margins to PanZoomController (via GeckoLayerClient), and uses that and a little hack to make the top of the page accessible, and make JS scrolling to position 0 expose the toolbar.

Comment 33

[Chris Lord [:cwiiis]]

What's missing now is dynamic page sizing/meta viewport handling so that full-screen/small/no-scrolling pages have a fixed toolbar, then some testing on sites like Google Maps. I'm tempted to say that these three could go in without that fourth part, to get them more testing, but there are definitely still issues left.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 710350 [details] [diff] [review]
Part 1 - Scroll header off the top

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

Looks pretty good overall. Some nits/questions. My main concern (and the reason I'm not r+'ing yet) is the manual mPointersDown tracking which seems wrong to me.

::: mobile/android/base/BrowserApp.java
@@ +54,5 @@
>  
>  abstract public class BrowserApp extends GeckoApp
>                                   implements TabsPanel.TabsLayoutChangeListener,
> +                                            PropertyAnimator.PropertyAnimationListener,
> +                                            OnInterceptTouchListener {

I don't think you need to implement OnInterceptTouchListener explicitly here. It gets inherited from GeckoApp anyway.

@@ +146,5 @@
>              case STOP:
> +                if (Tabs.getInstance().isSelectedTab(tab)) {
> +                    if (!mAboutHomeShowing) {
> +                        // Hide the toolbar after a delay.
> +                        mBrowserToolbar.animateVisibility(false, 2000);

Pull out the magic number into a private static final int above.

@@ +214,5 @@
> +
> +        View toolbarView = mBrowserToolbar.getLayout();
> +        if (action == MotionEvent.ACTION_DOWN ||
> +            action == MotionEvent.ACTION_POINTER_DOWN) {
> +            mPointersDown ++;

I'm not a fan of this approach of manually tracking how many pointers are down. It seems like you should be able to tell that from event.getPointerCount() wherever you need it. I feel like tracking it manually is really brittle and it could easily get out of sync with reality.

For example, if you do an ACTION_DOWN followed by an ACTION_UP while mCurrentTabLoading is true, I think you'll end up in state where mPointersDown = 1 even though no pointers are actually down. (This is just from my reading through the code, I could be wrong)

@@ +226,5 @@
> +
> +                // Animate the toolbar to the fully on/off position.
> +                mBrowserToolbar.animateVisibility(
> +                    toolbarView.getScrollY() > toolbarView.getHeight() / 2 ?
> +                      false : true, 0);

Does this animateVisibility make sense here? If I'm reading this right then when the user puts a second finger down while the toolbar is half-shown, it will slide to visible or hidden rather than staying put. I think that would be disconcerting as a user; if my finger is static on the screen I expect to be in control and not have things moving under me.

@@ +259,5 @@
> +                // clamping to fully visible or fully hidden.
> +                float deltaY = mLastY - eventY;
> +
> +                // Don't let the toolbar scroll off the top if it's just exposing
> +                // overscroll area.

Thanks for these comments!

@@ +264,5 @@
> +                ImmutableViewportMetrics metrics =
> +                  mLayerView.getLayerClient().getViewportMetrics();
> +                int toolbarMaxY = Math.min(toolbarHeight,
> +                    Math.max(0, toolbarHeight - Math.round(metrics.pageRectTop -
> +                                                           metrics.viewportRectTop)));

The pageRectTop - viewportRectTop seems wrong to me, because pageRectTop is always zero. It'll take me a few hours to wrap my head around this coordinate system though :)

@@ +279,5 @@
> +                if (newToolbarY == 0 || newToolbarY == toolbarHeight) {
> +                    mLastY = eventY;
> +                }
> +            } else if (action == MotionEvent.ACTION_UP ||
> +                       action == MotionEvent.ACTION_CANCEL) {

action will never be CANCEL here; that case was handled at the top of this method.

::: mobile/android/base/BrowserToolbar.java
@@ +432,5 @@
> +        // Do nothing if there's a delayed animation pending that does the
> +        // same thing and this request also has a delay.
> +        if (mVisibility != ToolbarVisibility.INCONSISTENT &&
> +            ((delay > 0) == (mDelayedVisibilityTask != null)) &&
> +            (show == (mVisibility == ToolbarVisibility.VISIBLE))) {

You could replace the mVisibility == VISIBLE check with a call to isVisible(). There's arguments either way, your call.

@@ +445,5 @@
> +                                   show ? 0 : mLayout.getHeight());
> +        if (delay > 0) {
> +            mDelayedVisibilityTask = new TimerTask() {
> +                public void run() {
> +                    mVisibilityAnimator.start();

Should this run() method also set mDelayedVisibilityTask to null? Otherwise it could be left as a non-null "dead" task that will interfere with the logic in the if condition at the start of the method. It looks like all this code only ever runs on the UI thread so synchronization shouldn't be a problem.

@@ +467,5 @@
> +        }
> +    }
> +
> +    public boolean getVisibility() {
> +        return mVisibility == ToolbarVisibility.VISIBLE;

This function should be called isVisible() rather than getVisibility()

Comment 35

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 710351 [details] [diff] [review]
Part 2 - Scroll fixed layers with the toolbar

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

Looks pretty good. Some comments below. The synchronization will need to be addressed before I'll r+. I didn't really look at the CompositorParent changes.

::: mobile/android/base/BrowserApp.java
@@ +408,5 @@
>          super.initializeChrome(uri, isExternalURL);
>  
>          mBrowserToolbar.updateBackButton(false);
>          mBrowserToolbar.updateForwardButton(false);
> +        ((BrowserToolbarLayout)mBrowserToolbar.getLayout()).refreshMargins();

Instead of doing this cast and explicit call, can we override some other initialization-like method in BrowserToolbarLayout and call refreshMargins from there? Maybe in onFinishInflate or something?

::: mobile/android/base/BrowserToolbarLayout.java
@@ +20,5 @@
> +    }
> +
> +    @Override
> +    protected void onScrollChanged(int l, int t, int oldl, int oldt) {
> +        refreshMargins();

Also call super.onScrollChanged(..)

@@ +25,5 @@
> +    }
> +
> +    @Override
> +    protected void onSizeChanged(int w, int h, int oldw, int oldh) {
> +        refreshMargins();

Also call super.onSizeChanged(..)

@@ +32,5 @@
> +    public void refreshMargins() {
> +        LayerView view = GeckoApp.mAppContext.getLayerView();
> +        if (view != null) {
> +            view.getLayerClient().setFixedLayerMargins(0, getHeight() - getScrollY(), 0, 0);
> +            view.getLayerClient().setPageMargins(0, getHeight() - getScrollY(), 0, 0);

The setPageMargins(..) isn't defined in this patch. This line should be moved to part 3, I think.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +346,5 @@
> +    /**
> +     * Sets margins on fixed-position layers, to be used when compositing.
> +     */
> +    public void setFixedLayerMargins(int left, int top, int right, int bottom) {
> +        mFixedLayerMargins.set(left, top, right, bottom);

This is called on the UI thread but read on the compositor thread. Synchronization needed.

::: mozglue/android/APKOpen.cpp
@@ +297,5 @@
>  SHELL_WRAPPER0(scheduleComposite)
>  SHELL_WRAPPER0(schedulePauseComposition)
>  SHELL_WRAPPER2(scheduleResumeComposition, jint, jint)
>  SHELL_WRAPPER0_WITH_RETURN(computeRenderIntegrity, jfloat)
> +SHELL_WRAPPER4(setFixedLayerMargins, jint, jint, jint, jint)

Once bug 794982 lands you won't need the changes to this file.

Comment 36

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 710352 [details] [diff] [review]
Part 3 - Make sure the top of the page is accessible

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

I'm running out of steam so I'll defer this review until tomorrow. However, something to think about: it might make sense to add the page margins as actual fields in the ImmutableViewportMetrics rather than floating them around in PZC. I think that might result in a little cleaner design.

Comment 37

[Chris Lord [:cwiiis]]

Excellent review, thanks :)

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #34)
> Comment on attachment 710350 [details] [diff] [review]
> Part 1 - Scroll header off the top
> 
> Review of attachment 710350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty good overall. Some nits/questions. My main concern (and the
> reason I'm not r+'ing yet) is the manual mPointersDown tracking which seems
> wrong to me.

I think you're right, and good call.

> ::: mobile/android/base/BrowserApp.java
> @@ +54,5 @@
> >  
> >  abstract public class BrowserApp extends GeckoApp
> >                                   implements TabsPanel.TabsLayoutChangeListener,
> > +                                            PropertyAnimator.PropertyAnimationListener,
> > +                                            OnInterceptTouchListener {
> 
> I don't think you need to implement OnInterceptTouchListener explicitly
> here. It gets inherited from GeckoApp anyway.

Correct.

> @@ +146,5 @@
> >              case STOP:
> > +                if (Tabs.getInstance().isSelectedTab(tab)) {
> > +                    if (!mAboutHomeShowing) {
> > +                        // Hide the toolbar after a delay.
> > +                        mBrowserToolbar.animateVisibility(false, 2000);
> 
> Pull out the magic number into a private static final int above.

Will do.

> @@ +214,5 @@
> > +
> > +        View toolbarView = mBrowserToolbar.getLayout();
> > +        if (action == MotionEvent.ACTION_DOWN ||
> > +            action == MotionEvent.ACTION_POINTER_DOWN) {
> > +            mPointersDown ++;
> 
> I'm not a fan of this approach of manually tracking how many pointers are
> down. It seems like you should be able to tell that from
> event.getPointerCount() wherever you need it. I feel like tracking it
> manually is really brittle and it could easily get out of sync with reality.
> 
> For example, if you do an ACTION_DOWN followed by an ACTION_UP while
> mCurrentTabLoading is true, I think you'll end up in state where
> mPointersDown = 1 even though no pointers are actually down. (This is just
> from my reading through the code, I could be wrong)

You're right, and I'll do this - I misunderstood what getPointerCount did originally, and thought it wouldn't be useful for this (it is).

> @@ +226,5 @@
> > +
> > +                // Animate the toolbar to the fully on/off position.
> > +                mBrowserToolbar.animateVisibility(
> > +                    toolbarView.getScrollY() > toolbarView.getHeight() / 2 ?
> > +                      false : true, 0);
> 
> Does this animateVisibility make sense here? If I'm reading this right then
> when the user puts a second finger down while the toolbar is half-shown, it
> will slide to visible or hidden rather than staying put. I think that would
> be disconcerting as a user; if my finger is static on the screen I expect to
> be in control and not have things moving under me.

This is intentional, and feels good when you actually try it :) (imo, at least) The alternative is to have a partially visible toolbar while you zoom, or to have it moving semi-randomly about the place while you zoom, both of which felt bad when I tested them. UX can make the call on this, but I feel this way is the best way to start.

> @@ +259,5 @@
> > +                // clamping to fully visible or fully hidden.
> > +                float deltaY = mLastY - eventY;
> > +
> > +                // Don't let the toolbar scroll off the top if it's just exposing
> > +                // overscroll area.
> 
> Thanks for these comments!

I've read too much of my own code I didn't understand at first glance :)

> @@ +264,5 @@
> > +                ImmutableViewportMetrics metrics =
> > +                  mLayerView.getLayerClient().getViewportMetrics();
> > +                int toolbarMaxY = Math.min(toolbarHeight,
> > +                    Math.max(0, toolbarHeight - Math.round(metrics.pageRectTop -
> > +                                                           metrics.viewportRectTop)));
> 
> The pageRectTop - viewportRectTop seems wrong to me, because pageRectTop is
> always zero. It'll take me a few hours to wrap my head around this
> coordinate system though :)

I guess I was just covering my bases using pageRectTop, but I think it's right. Might read better as toolbarHeight + Math.round(metrics.viewportRectTop + metrics.pageRectTop) - the idea is that when the viewportRectTop is above the page top, that much of the toolbar must be visible (to avoid scrolling it off while the page is in overscroll)

> @@ +279,5 @@
> > +                if (newToolbarY == 0 || newToolbarY == toolbarHeight) {
> > +                    mLastY = eventY;
> > +                }
> > +            } else if (action == MotionEvent.ACTION_UP ||
> > +                       action == MotionEvent.ACTION_CANCEL) {
> 
> action will never be CANCEL here; that case was handled at the top of this
> method.

It was handled at the top, but it doesn't return - it will be handled here.

> ::: mobile/android/base/BrowserToolbar.java
> @@ +432,5 @@
> > +        // Do nothing if there's a delayed animation pending that does the
> > +        // same thing and this request also has a delay.
> > +        if (mVisibility != ToolbarVisibility.INCONSISTENT &&
> > +            ((delay > 0) == (mDelayedVisibilityTask != null)) &&
> > +            (show == (mVisibility == ToolbarVisibility.VISIBLE))) {
> 
> You could replace the mVisibility == VISIBLE check with a call to
> isVisible(). There's arguments either way, your call.

True, I think this would read nicer, especially with the suggested rename below.

> @@ +445,5 @@
> > +                                   show ? 0 : mLayout.getHeight());
> > +        if (delay > 0) {
> > +            mDelayedVisibilityTask = new TimerTask() {
> > +                public void run() {
> > +                    mVisibilityAnimator.start();
> 
> Should this run() method also set mDelayedVisibilityTask to null? Otherwise
> it could be left as a non-null "dead" task that will interfere with the
> logic in the if condition at the start of the method. It looks like all this
> code only ever runs on the UI thread so synchronization shouldn't be a
> problem.

It should, nice catch.

> @@ +467,5 @@
> > +        }
> > +    }
> > +
> > +    public boolean getVisibility() {
> > +        return mVisibility == ToolbarVisibility.VISIBLE;
> 
> This function should be called isVisible() rather than getVisibility()

I called it getVisibility to ape Android's equivalent function, but I agree, will do.

Comment 38

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Lord [:cwiiis] from comment #37)
> > Does this animateVisibility make sense here? If I'm reading this right then
> > when the user puts a second finger down while the toolbar is half-shown, it
> > will slide to visible or hidden rather than staying put. I think that would
> > be disconcerting as a user; if my finger is static on the screen I expect to
> > be in control and not have things moving under me.
> 
> This is intentional, and feels good when you actually try it :) (imo, at
> least) The alternative is to have a partially visible toolbar while you
> zoom, or to have it moving semi-randomly about the place while you zoom,
> both of which felt bad when I tested them. UX can make the call on this, but
> I feel this way is the best way to start.

Fair enough.

> > @@ +264,5 @@
> > > +                ImmutableViewportMetrics metrics =
> > > +                  mLayerView.getLayerClient().getViewportMetrics();
> > > +                int toolbarMaxY = Math.min(toolbarHeight,
> > > +                    Math.max(0, toolbarHeight - Math.round(metrics.pageRectTop -
> > > +                                                           metrics.viewportRectTop)));
> > 
> > The pageRectTop - viewportRectTop seems wrong to me, because pageRectTop is
> > always zero. It'll take me a few hours to wrap my head around this
> > coordinate system though :)
> 
> I guess I was just covering my bases using pageRectTop, but I think it's
> right. Might read better as toolbarHeight +
> Math.round(metrics.viewportRectTop + metrics.pageRectTop) - the idea is that
> when the viewportRectTop is above the page top, that much of the toolbar
> must be visible (to avoid scrolling it off while the page is in overscroll)

Ah ok, that makes sense. In that case leave the code as the original version, I think that reads better than the one with the pluses.

> > action will never be CANCEL here; that case was handled at the top of this
> > method.
> 
> It was handled at the top, but it doesn't return - it will be handled here.

Oh, true. My mistake.

Comment 39

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Scroll header off the top

Taken all review comments into account (I think), and fixed an issue after part 3 where scrolling off the top bar would apply edge resistance.

Comment 40

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 711424 [details] [diff] [review]
Part 1 - Scroll header off the top

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

r=me, with the one question about pointerCount below resolved.

::: mobile/android/base/BrowserApp.java
@@ +221,5 @@
> +                mTouchOffsetY = 0.0f;
> +                mToolbarSubpixelAccumulation = 0.0f;
> +                mLastY = event.getY();
> +                return super.onInterceptTouchEvent(view, event);
> +            } else {

else-after-return. I don't care either way but mozilla style says to get rid of the else.

@@ +240,5 @@
> +        }
> +
> +        // If a pointer has been lifted so that there's only one pointer left,
> +        // unlock the toolbar and track that remaining pointer.
> +        if (pointerCount == 1 && action == MotionEvent.ACTION_POINTER_UP) {

For ACTION_POINTER_UP and ACTION_UP events, is the pointer count the number of pointers that are remaining on the screen? Or does the pointer count include the pointer that was lifted? I thought it was the latter - if that's the case then you may want to just add a check for that and decrement pointerCount by one up above where you initialize pointerCount.

@@ +271,5 @@
> +                int toolbarY = toolbarView.getScrollY();
> +                float newToolbarYf = Math.max(0, Math.min(toolbarMaxY,
> +                    toolbarY + deltaY + mToolbarSubpixelAccumulation));
> +                int newToolbarY = (int)Math.floor(newToolbarYf);
> +                mToolbarSubpixelAccumulation = (newToolbarYf - newToolbarY);

Kind of icky that this is necessary but I don't see any other way either.

Comment 41

[Benoit Girard (:BenWa)]

Comment on attachment 710351 [details] [diff] [review]
Part 2 - Scroll fixed layers with the toolbar

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

I'm going to defer the review to Bas since this will have a heavy impact on the layers refactoring which I'm not involved with.

Comment 42

[Wesley Johnston (:wesj)]

What happens when doorhangers are shown with this (and what should happen?) I would guess we show the urlbar until the doorhanger is dismissed?

Comment 43

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Scroll header off the top

Carrying r+ - Removed offsetting (on UX request) and rebased.

Comment 44

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Scroll fixed layers with the toolbar

Mostly the same, but requesting r=kats again just in case of a rebase mistake. BenWa, I've added f? from nrc about the layers refactor, could you review just to see if it's ok irrespective of that?

Comment 45

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Make sure the top of the page is accessible

Really can't remember what I've changed with this, but it works much better than the previous patch.

Comment 46

[Chris Lord [:cwiiis]]

I'm still working on the dynamic page resizing, but I think we should get these bits landed regardless. We mostly want the resizing for games, which ought to use fullscreen API anyway...

Google Maps is usable with these patches - it's not a fantastic experience mind, but it's still usable.

Comment 47

[Nick Cameron [:nrc]]

Comment on attachment 713996 [details] [diff] [review]
Part 2 - Scroll fixed layers with the toolbar

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

Won't affect the layers refactoring; landing this before or after makes no difference to us. Thanks for f? me about it!

Comment 48

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 713996 [details] [diff] [review]
Part 2 - Scroll fixed layers with the toolbar

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

r+ with comments addressed

::: mobile/android/base/BrowserToolbarLayout.java
@@ +20,5 @@
> +    }
> +
> +    @Override
> +    protected void onScrollChanged(int l, int t, int oldl, int oldt) {
> +        refreshMargins();

Call super.onScrollChanged() before calling refreshMargins()

@@ +25,5 @@
> +    }
> +
> +    @Override
> +    protected void onSizeChanged(int w, int h, int oldw, int oldh) {
> +        refreshMargins();

Ditto

::: mobile/android/base/gfx/ImmutableViewportMetrics.java
@@ +156,5 @@
>              FloatUtils.interpolate(zoomFactor, to.zoomFactor, t));
>      }
>  
> +    public ImmutableViewportMetrics setViewportSize(float width, float height,
> +                                                    ImmutableViewportMetrics aMetricsForMargins) {

Rename this to setViewportSizeAndMargins, or get rid of this entirely and make the call in GeckoLayerClient look more like:

metrics = messageMetrics.setViewportSize(oldMetrics.getWidth(), oldMetrics.getHeight()).setFixedLayerMargins(oldMetrics.fixedLayerMarginLeft, ...);

@@ +263,5 @@
>          // of the cssPageRectXXX values and the zoomFactor, except with more rounding
>          // error. Checking those is both inefficient and can lead to false negatives.
> +        // XXX This doesn't return false if the fixed layer margins differ as none
> +        //     of the users of this function are interested in the margins in that
> +        //     way.

No need for the "XXX" in this comment.

Comment 49

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 713998 [details] [diff] [review]
Part 3 - Make sure the top of the page is accessible

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

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +485,5 @@
> +            // If we're meant to be scrolled to the top, take into account any
> +            // margin set on the pan zoom controller.
> +            if (offsetY == 0) {
> +                offsetY = -currentMetrics.fixedLayerMarginTop;
> +            }

This looks wrong to me. Can you explain what this is for?

Comment 50

[Kartikaya Gupta (email:[e-mail])]

Also: I didn't look at the CompositorParent changes in patch 3; might want to get BenWa to look at that. And please address the question in comment 40 before landing part 1.

Comment 51

[Benoit Girard (:BenWa)]

Comment on attachment 713996 [details] [diff] [review]
Part 2 - Scroll fixed layers with the toolbar

This needs to fit into the new layers refactor. Deferring review to nrc.

Comment 52

[Nick Cameron [:nrc]]

Comment on attachment 713996 [details] [diff] [review]
Part 2 - Scroll fixed layers with the toolbar

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

On the basis that Kats has given this a proper review (which it looks like), then I will give r+ from the point of view of the refactoring because I don't think it will affect us. But, I don't understand most of what the code is doing, so this is not an r+ in any other sense.

Comment 53

[Nick Cameron [:nrc]]

Let me know if you do need a proper review from me and I will try to get to grips with the code.

Comment 54

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Make sure the top of the page is accessible

Ugh, so I folded a patch into this, but I meant to fold it into part 2 - thus the removal of SetFixedLayerMargins in this patch. If you wouldn't mind reviewing like this, that'd be great, or I could fold parts 2 and 3 (please don't make me separate out the change!)

I found that I needed to add the fixed layer margins to syncViewportInfo instead, as I need to change them in sync with local viewport changes.

Comment 55

[Chris Lord [:cwiiis]]

Attachment: Part 4 - Resize viewport depending on page height

This patch resizes the css viewport depending on the page height - I've tested with pages of varying heights, and it seems to do the right thing and not cause any height oscillations, but I'd really like some extra eyes and testing on this.

Going to Google Maps with this patch Does The Right Thing(tm).

Comment 56

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #49)
> Comment on attachment 713998 [details] [diff] [review]
> Part 3 - Make sure the top of the page is accessible
> 
> Review of attachment 713998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/gfx/GeckoLayerClient.java
> @@ +485,5 @@
> > +            // If we're meant to be scrolled to the top, take into account any
> > +            // margin set on the pan zoom controller.
> > +            if (offsetY == 0) {
> > +                offsetY = -currentMetrics.fixedLayerMarginTop;
> > +            }
> 
> This looks wrong to me. Can you explain what this is for?

This is so that if JS/Gecko scrolls to position 0, we expose the toolbar. This only affects the metrics Java-side, as we clamp metrics before sending them to Gecko (so they'll end up at 0).

Comment 57

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

Comment on attachment 715187 [details] [diff] [review]
Part 4 - Resize viewport depending on page height

I'm not a fan of global variables in browser.js, but we have "prior art" for this case. Overall, the changes in browser.js seem sane.

I'd remove the extra Log and dump calls before landing. Did you run on Try server to see how this affects our viewport tests?

Comment 58

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 715186 [details] [diff] [review]
Part 3 - Make sure the top of the page is accessible

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

This looks good to me. But yeah, please fold this into part 2 on landing to avoid the code churn.

Comment 59

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Lord [:cwiiis] from comment #56)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #49)
> > Comment on attachment 713998 [details] [diff] [review]
> > Part 3 - Make sure the top of the page is accessible
> > 
> > Review of attachment 713998 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/gfx/GeckoLayerClient.java
> > @@ +485,5 @@
> > > +            // If we're meant to be scrolled to the top, take into account any
> > > +            // margin set on the pan zoom controller.
> > > +            if (offsetY == 0) {
> > > +                offsetY = -currentMetrics.fixedLayerMarginTop;
> > > +            }
> > 
> > This looks wrong to me. Can you explain what this is for?
> 
> This is so that if JS/Gecko scrolls to position 0, we expose the toolbar.
> This only affects the metrics Java-side, as we clamp metrics before sending
> them to Gecko (so they'll end up at 0).

Actually looking at it a bit more, I'm not sure this belongs in setFirstPaintViewport. That only gets called on switching tabs or when a new page loads. What happens if the content scrolls to zero some time after page load? Don't we want this to happen there as well?

Comment 60

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 715187 [details] [diff] [review]
Part 4 - Resize viewport depending on page height

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

Some comments below. I'll need to take another look at this later to finish reviewing it.

::: mobile/android/base/BrowserToolbar.java
@@ +452,5 @@
> +    }
> +
> +    private void startVisibilityAnimation() {
> +        if (mVisibility != ToolbarVisibility.HIDDEN ||
> +            canToolbarHide()) {

Should this be && instead of || ?

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +567,5 @@
> +        // when we're in overscroll and the page is shorter/narrower than the
> +        // viewport.
> +        float maxMarginWidth = Math.max(0,
> +            mFrameMetrics.getPageWidth() - mFrameMetrics.getViewportWidth() -
> +            (mFrameMetrics.fixedLayerMarginLeft + mFrameMetrics.fixedLayerMarginRight));

See comment in ImmutableViewportMetrics; doing that will simplify this expression to Math.max(0, mFrameMetrics.getPageWidth() - mFrameMetrics.getWidthWithMargins()).

@@ +573,5 @@
> +            mFrameMetrics.getPageHeight() - mFrameMetrics.getViewportHeight() -
> +            (mFrameMetrics.fixedLayerMarginTop + mFrameMetrics.fixedLayerMarginBottom));
> +
> +        mCurrentViewTransform.fixedLayerMarginLeft =
> +            Math.min(mFrameMetrics.fixedLayerMarginLeft, maxMarginWidth);

I'm not sure this is right. Let's say that above, getPageWidth() == (getViewportWidth() + fixedLayerMarginLeft + fixedLayerMarginRight). In that case maxMarginWidth will come out to be zero, and so here mCurrentViewTransform.fixedLayerMarginLeft will be set to 0 as well. However I think in that scenario you want to end up with the mCurrentviewTransform.fixedLayerMarginLeft equal to mFrameMetrics.fixedLayerMarginLeft, no?

::: mobile/android/base/gfx/ImmutableViewportMetrics.java
@@ +126,5 @@
>          return pageRectBottom - pageRectTop;
>      }
>  
> +    public float getViewportWidth() {
> +        return viewportRectRight - viewportRectLeft;

This is the same as getWidth(); it is redundant. I would prefer if you renamed this to getWidthWithMargins, and returned getWidth() + marginleft + marginright.

@@ +130,5 @@
> +        return viewportRectRight - viewportRectLeft;
> +    }
> +
> +    public float getViewportHeight() {
> +        return viewportRectBottom - viewportRectTop;

Ditto

Comment 61

[Chris Lord [:cwiiis]]

Attachment: Part 4 - Resize viewport depending on page height

Updated patch with dump/log stuff removed and fixed viewport often not being extended to the full size and causing elements to be inaccessible at the bottom of the screen. Sorry for the review churn.

Comment 62

[Chris Lord [:cwiiis]]

Realised a stupid thing with part 2, offsetting fixed-pos layers in the compositor doesn't effect event handling, so this breaks pretty regularly :/

I can envisage fixing it by duplicating this offset during the drawing that happens during touch handling, which I guess would require another nsIDOMWindowUtils method or the like.

So to break down the problem;

- Fixed position layers are offset so that the toolbar doesn't occlude them
- This offset happens in the compositor so it can be animated smoothly
- This offset is not duplicated anywhere on the layout side
- Input event handling happens Gecko side and is (possibly) broken by this offset

Possible fixes;

1- Add a method to nsIDOMWindowUtils, something along the lines of SetCompositorFixedLayerMargins that could be respected while drawing the display list for touch event handling (this could be tricky as we'd need some logic to distinguish between a fixed-pos element and a fixed-pos element that ends up on a fixed layer - possibly we could just ignore this situation)
2- Actually respect the above added layer margins layout-side at all times and annotate resultant layers so the difference can be reconciled in CompositorParent (possibly complicated, but likely the most reliable solution)
3- When there's a margin set, check what element's under the cursor when negatively offset by the fixed layer margins and try to offset input events when this is a fixed-pos element. Same issue as above in distinguishing between fixed elements and fixed layers, also tricky when multiple margins are set...
4- Just don't offset fixed-pos elements and let the toolbar cover them (lame, and the Android stock browser manages to not do this)
- Other?

This really can't land until this is fixed, I imagine people aren't going to take kindly to breaking Twitter :(

Comment 63

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #62)
> 2- Actually respect the above added layer margins layout-side at all times
> and annotate resultant layers so the difference can be reconciled in
> CompositorParent (possibly complicated, but likely the most reliable
> solution)

Ftr, implementing this. Not as complicated as I thought it would be.

Comment 64

[Chris Lord [:cwiiis]]

Attachment: Part 5 - Add fixed position margins to layout

Picked reviewer based on hg log, hope that's ok.

This patch adds the ability to add additional margins to the absolute containing block on the viewport of a document via nsIDOMWindowUtils/nsIPresShell.

The rationale for doing this, is that in a subsequent patch I'll add annotation to layers about these margins so that we can do async fixed position margin animation in the compositor and input should work correctly (at least when Gecko and compositor match up, so it might be slightly off if you tapped a fixed-pos element during an animation).

Doing this also minimises the chance of rendering errors when offsetting the fixed position elements in the compositor (in the situation that we hit bug 792415, for example).

Comment 65

[Chris Lord [:cwiiis]]

Comment on attachment 716442 [details] [diff] [review]
Part 5 - Add fixed position margins to layout

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

::: layout/generic/nsAbsoluteContainingBlock.cpp
@@ +369,5 @@
> +  nsMargin border = aReflowState.mStyleBorder->GetComputedBorder();
> +
> +  // Respect fixed position margins.
> +  if (aDelegatingFrame->GetAbsoluteListID() == nsIFrame::kFixedList) {
> +    nsMargin fixedMargins = aDelegatingFrame->PresContext()->PresShell()->

s/aDelegatingFrame->PresContext()->presContext

Please ignore this :)

Comment 66

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #65)
> Comment on attachment 716442 [details] [diff] [review]
> Part 5 - Add fixed position margins to layout
> 
> Review of attachment 716442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsAbsoluteContainingBlock.cpp
> @@ +369,5 @@
> > +  nsMargin border = aReflowState.mStyleBorder->GetComputedBorder();
> > +
> > +  // Respect fixed position margins.
> > +  if (aDelegatingFrame->GetAbsoluteListID() == nsIFrame::kFixedList) {
> > +    nsMargin fixedMargins = aDelegatingFrame->PresContext()->PresShell()->
> 
> s/aDelegatingFrame->PresContext()->presContext
> 
> Please ignore this :)

aPresContext even, sorry for churn.

Comment 67

[Daniel Holbert [:dholbert]]

Comment on attachment 716442 [details] [diff] [review]
Part 5 - Add fixed position margins to layout

I don't think I've reviewed significant PresShell or nsDOMWindowUtils changes before; I'd prefer that someone with more ownership over those files reviewed this.

Having said that, here's my feedback:

>+NS_IMETHODIMP
>+nsDOMWindowUtils::SetContentDocumentFixedPositionMargins(float aLeft, float aTop,
>+                                                         float aRight, float aBottom)
>+{

Change the param-order there -- move aLeft to the end.  It should be (aTop, aRight, aBottom, aLeft) to be consistent w/ CSS margin/border/padding specifications and nsDOMWindowUtils::NodesFromRect() ordering.

(It looks like you used the current order because it matches the nsMargin constructor.  I think the nsMargin constructor is wrong; since this is a public-ish API, you should follow our other public APIs' conventions, rather than the internal nsMargin constructor's convention. I may file a bug on fixing up the nsMargin constructor's param-order to be consistent as well...)

>+  if (!(aLeft >= 0.0 && aTop >= 0.0 && aRight >= 0.0 && aBottom >= 0.0)) {
>+    return NS_ERROR_ILLEGAL_VALUE;
>+  }

Move aLeft to the end, to match the change above.

>+  presShell->SetContentDocumentFixedPositionMargins(
>+    nsPresContext::CSSPixelsToAppUnits(aLeft),
>+    nsPresContext::CSSPixelsToAppUnits(aTop),
>+    nsPresContext::CSSPixelsToAppUnits(aRight),
>+    nsPresContext::CSSPixelsToAppUnits(aBottom));

Here as well -- *BUT* it'd be even better to pass a "const nsMargin&" here, rather than 4 individual nscoord values.  That reduces the number of places where you have to worry about the ordering of these nscoord values, and it reduces unnecessary variable-copying.

>+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl
> 
>+   * Set margins for the layout of fixed position elements in the content
>+   * document.
>+   *
>+   * The caller of this method must have chrome privileges.
>+   */
>+  void setContentDocumentFixedPositionMargins(in float aLeft, in float aTop,
>+                                              in float aRight, in float aBottom);
>+

(As above, move aLeft to the end there)

Also: I think this might need superreview, since it changes an interface...?

>diff --git a/layout/base/nsIPresShell.h b/layout/base/nsIPresShell.h
>--- a/layout/base/nsIPresShell.h
>+++ b/layout/base/nsIPresShell.h

I think (?) you need to bump the IID of nsIPresShell, at the top of this file.

>   /**
>+   * Calls FrameNeedsReflow on all fixed position children of the root frame.
>+   */
>+  virtual void ReflowFixedPositionChildren(IntrinsicDirty aIntrinsicDirty);

This function name is misleading -- it makes it sound like it synchronously reflows stuff (which it doesn't).

Maybe "RequestReflowForFixedPositionChildren"?  (I also don't love using the term "Children" here, because the pres shell doesn't have "children"... I don't have any better suggestions though.)

>+  void SetContentDocumentFixedPositionMargins(nscoord aLeft, nscoord aTop, nscoord aRight, nscoord aBottom);

As noted above, this should just take a const nsMargin& (or, failing that, move aLeft to the end).

>+  nsMargin GetContentDocumentFixedPositionMargins() {
>+    return mContentDocumentFixedPositionMargins;
>+  }

This should return const nsMargin&, so that its caller doesn't have to copy the margin.

> 
>+  nsMargin                  mContentDocumentFixedPositionMargins;
>+

This needs a comment explaining what it's there for.  (Note that nearly all of the member-variables listed around it have explanatory comments.)

Also, does this need to be initialized somewhere?

> void
>+nsIPresShell::ReflowFixedPositionChildren(IntrinsicDirty aIntrinsicDirty)
>+{
>+  nsIFrame* rootFrame = mFrameConstructor->GetRootFrame();
>+  if (rootFrame) {
>+    const nsFrameList& childList = rootFrame->GetChildList(nsIFrame::kFixedList);
>+    for (nsIFrame* child = childList.FirstChild(); child;
>+         child = child->GetNextSibling()) {
>+      FrameNeedsReflow(child, aIntrinsicDirty, NS_FRAME_IS_DIRTY);

nsFrameList::Enumerator is the New Hot Way to iterate across frame lists.  Use it here.

See e.g. 
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp#1089
for sample usage, with s/mFrames/childList/

>+void
>+nsIPresShell::SetContentDocumentFixedPositionMargins(nscoord aLeft, nscoord aTop,
>+                                                     nscoord aRight, nscoord aBottom)
>+{
>+  nsMargin newMargins(aLeft, aTop, aRight, aBottom);

As noted above, this method should just take a const nsMargin&

> void
> PresShell::SetupFontInflation()
> {
>   mFontSizeInflationEmPerLine = nsLayoutUtils::FontSizeInflationEmPerLine();
>   mFontSizeInflationMinTwips = nsLayoutUtils::FontSizeInflationMinTwips();
>   mFontSizeInflationLineThreshold = nsLayoutUtils::FontSizeInflationLineThreshold();
>diff --git a/layout/generic/nsAbsoluteContainingBlock.cpp b/layout/generic/nsAbsoluteContainingBlock.cpp
>+  // Respect fixed position margins.
>+  if (aDelegatingFrame->GetAbsoluteListID() == nsIFrame::kFixedList) {
>+    nsMargin fixedMargins = aDelegatingFrame->PresContext()->PresShell()->
>+      GetContentDocumentFixedPositionMargins();

const nsMargin& fixedMargins

>+
>+    border += fixedMargins;

I don't immediately understand why you're adding the margins to the border here; some commenting (and/or variable-renaming) would help.

Comment 68

[Daniel Holbert [:dholbert]]

> >+  nsMargin                  mContentDocumentFixedPositionMargins;
> >+
[...]
> Also, does this need to be initialized somewhere?

(Actually, looks like it'll be initialized to 0,0,0,0, which is probably (?) what you want).

Comment 69

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #67)
> ...I may file a bug on
> fixing up the nsMargin constructor's param-order to be consistent as well...)

(I filed bug 843719 on this, btw)

Comment 70

[Chris Lord [:cwiiis]]

Comment on attachment 716442 [details] [diff] [review]
Part 5 - Add fixed position margins to layout

Bearing in mind dholbert's comment, r?'ing roc - I'll fix all the points raised too.

Comment 71

[Chris Lord [:cwiiis]]

Attachment: Part 6 - Add async fixed margin setting

This annotates layers with the fixed margins set in layout and adds the necessary code in CompositorParent to reconcile temporary differences.

Comment 72

[Chris Lord [:cwiiis]]

Attachment: Part 7 - Use async margins so input on offset fixed elements works

I'm attaching this for reference and asking for feedback. I think there's some reduction that can be done involving part 3 of this patch, so not asking for review just yet but feel free to treat this as review.

There are still some issues, Java-side I think, with input at the top of the page when the header isn't visible - that's the last thing seriously wrong with this series of patches, otherwise it works reasonably well.

Comment 73

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

Comment on attachment 716442 [details] [diff] [review]
Part 5 - Add fixed position margins to layout

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

Can we get a description in comments of what these fixed margins actually mean and why we need them?

Comment 74

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #73)
> Comment on attachment 716442 [details] [diff] [review]
> Part 5 - Add fixed position margins to layout
> 
> Review of attachment 716442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we get a description in comments of what these fixed margins actually
> mean and why we need them?

I would add something like this:

/**
 * Adds margins to the viewport of the presShell. These are used on mobile, where
 * the viewable area can be temporarily obscured by the toolbar. In this
 * situation, we're ok with scrollable page content being obscured, but fixed
 * position content cannot be revealed without removing the obscuring chrome, so
 * we use these margins so that it can remain visible. These margins are also
 * duplicated on the compositor, to allow a smooth transition when chrome appears
 * on the screen.
 */

Comment 75

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 715611 [details] [diff] [review]
Part 4 - Resize viewport depending on page height

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

Sorry for the late review.

I'm not a huge fan of leaking toolbar specific details into browser.js here, it feels like the code is getting really tightly coupled. I could be convinced otherwise though, but I'd like to come up with better ways of doing this.

::: mobile/android/base/BrowserToolbar.java
@@ +450,5 @@
> +    }
> +
> +    private void startVisibilityAnimation() {
> +        if (mVisibility != ToolbarVisibility.HIDDEN ||
> +            canToolbarHide()) {

Shouldn't this be && ?

::: mobile/android/base/BrowserToolbarLayout.java
@@ +33,1 @@
>          refreshMargins();

Still missing a super.onSizeChanged call here (I had that as a review comment in part 2, please fix). Same for onScrollChanged.

::: mobile/android/base/gfx/ImmutableViewportMetrics.java
@@ +126,5 @@
>          return pageRectBottom - pageRectTop;
>      }
>  
> +    public float getViewportWidth() {
> +        return viewportRectRight - viewportRectLeft;

See https://bugzilla.mozilla.org/show_bug.cgi?id=716403#c60

::: mobile/android/chrome/content/browser.js
@@ +2778,5 @@
>  let gScreenHeight = 1;
>  
> +// The height of the visible toolbar, to use to reduce
> +// the window size of unscrollable pages.
> +let gToolbarHeight = 0;

Isn't gToolbarHeight a property of the tab, rather than a global property? I guess currently you can only switch tabs if the toolbar is fully visible, but have you given any thought to the case where we add some tab-switch gesture? If tab A is loaded and the toolbar is hidden, and tab B is still loading (i.e. should be displaying the toolbar), and you switch back and forth between them via some swipe gesture, what's supposed to happen to the toolbar?

Also I'm not a huge fan of the JS code here having to explicitly track things like "toolbar height" which are Java UI features. I prefer the generic "page margins" type of approach; I'm not sure if we can re-use those margins here or if we should make this more generic somehow. In particular I dislike how this only affects the y-axis, it makes the code less symmetrical and harder to catch errors.

Comment 76

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #75)
> Comment on attachment 715611 [details] [diff] [review]
> Part 4 - Resize viewport depending on page height
> 
> Review of attachment 715611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the late review.
> 
> I'm not a huge fan of leaking toolbar specific details into browser.js here,
> it feels like the code is getting really tightly coupled. I could be
> convinced otherwise though, but I'd like to come up with better ways of
> doing this.

Comments in-line.

> ::: mobile/android/base/BrowserToolbar.java
> @@ +450,5 @@
> > +    }
> > +
> > +    private void startVisibilityAnimation() {
> > +        if (mVisibility != ToolbarVisibility.HIDDEN ||
> > +            canToolbarHide()) {
> 
> Shouldn't this be && ?

No, I'll add a comment as such - mVisibility is the destination state in this case - we always want to be able to show, but we can't always hide - so this basically says continue as long as we're not trying to hide, or that we're trying to hide but that's ok.

> ::: mobile/android/base/BrowserToolbarLayout.java
> @@ +33,1 @@
> >          refreshMargins();
> 
> Still missing a super.onSizeChanged call here (I had that as a review
> comment in part 2, please fix). Same for onScrollChanged.

Sorry, I think I've lost track of things a bit - as such, I'm going to re-upload all parts for review after I've dealt with comments (sorry for the extra churn, but I'm going to forget something otherwise).

> ::: mobile/android/base/gfx/ImmutableViewportMetrics.java
> @@ +126,5 @@
> >          return pageRectBottom - pageRectTop;
> >      }
> >  
> > +    public float getViewportWidth() {
> > +        return viewportRectRight - viewportRectLeft;
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=716403#c60
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +2778,5 @@
> >  let gScreenHeight = 1;
> >  
> > +// The height of the visible toolbar, to use to reduce
> > +// the window size of unscrollable pages.
> > +let gToolbarHeight = 0;
> 
> Isn't gToolbarHeight a property of the tab, rather than a global property? I
> guess currently you can only switch tabs if the toolbar is fully visible,
> but have you given any thought to the case where we add some tab-switch
> gesture? If tab A is loaded and the toolbar is hidden, and tab B is still
> loading (i.e. should be displaying the toolbar), and you switch back and
> forth between them via some swipe gesture, what's supposed to happen to the
> toolbar?

gToolbarHeight is a property of the browesr chrome, rather than the tab - it's global and doesn't change when you switch tabs (well, it does, but it changes in a global way in response to load/stop events, the same as the toolbar spinner) - there's no per-tab state that the toolbar responds to.

Of course, we'll need to think of this if we do add tab changing gestures, but let's deal with that when we get there - I think there are going to be much bigger problems before getting to this.

> Also I'm not a huge fan of the JS code here having to explicitly track
> things like "toolbar height" which are Java UI features. I prefer the
> generic "page margins" type of approach; I'm not sure if we can re-use those
> margins here or if we should make this more generic somehow. In particular I
> dislike how this only affects the y-axis, it makes the code less symmetrical
> and harder to catch errors.

Like mfinkle says, we have form here... It does need to be a global, though it could be a more generically named one. I can rename this global and add support for all sides if it makes this read better. In this case, how would gViewportMargins sound? (where it would be an object with top/right/bottom/left properties)

Comment 77

[Nick Cameron [:nrc]]

Comment on attachment 717065 [details] [diff] [review]
Part 6 - Add async fixed margin setting

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

Mostly looks good.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +653,5 @@
>  
>      // Offset this translation by the fixed layer margins, depending on what
> +    // side of the viewport the layer is anchored to, reconciling the
> +    // difference between the current fixed layer margins and the Gecko-side
> +    // fixed layer margins.

I find this comment a bit confusing. There are three kinds of fixed layer margins - vanilla, "current", and "Gecko-side". Is the unadorned kind current - gecko or just current? How do these map to fixedMargins and aFixedLayerMargins? (Which, btw, should have more descriptive names).

::: gfx/layers/ipc/PLayers.ipdl
@@ +185,5 @@
>    bool useClipRect;
>    nsIntRect clipRect;
>    bool isFixedPosition;
>    gfxPoint fixedPositionAnchor;
> +  float fixedPositionMarginLeft; // IPDL doesn't allow us to use types without a void constructor

Could you add a void constructor to gfx::Margin instead? Initialising to 0,0,0,0 seems sensible.

Comment 78

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Lord [:cwiiis] from comment #76)
> No, I'll add a comment as such - mVisibility is the destination state in
> this case - we always want to be able to show, but we can't always hide - so
> this basically says continue as long as we're not trying to hide, or that
> we're trying to hide but that's ok.

Ok.

> Sorry, I think I've lost track of things a bit - as such, I'm going to
> re-upload all parts for review after I've dealt with comments (sorry for the
> extra churn, but I'm going to forget something otherwise).

Yeah, please do re-upload; I'd like to see versions of the patches with the comments addressed and as much hunk re-grouping as you care to do.

> gToolbarHeight is a property of the browesr chrome, rather than the tab -
> it's global and doesn't change when you switch tabs (well, it does, but it
> changes in a global way in response to load/stop events, the same as the
> toolbar spinner) - there's no per-tab state that the toolbar responds to.

That's true, but I still think conceptually it (and the toolbar spinner) are both per-tab properties, because switching tabs means switching the displayed state. But I don't really have any more concrete suggestions here so I'll let it slide.

> Like mfinkle says, we have form here... It does need to be a global, though
> it could be a more generically named one. I can rename this global and add
> support for all sides if it makes this read better. In this case, how would
> gViewportMargins sound? (where it would be an object with
> top/right/bottom/left properties)

Yeah I think I would prefer that, if it's not too much work.

Comment 79

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

(In reply to Chris Lord [:cwiiis] from comment #74)
> /**
>  * Adds margins to the viewport of the presShell. These are used on mobile,
> where
>  * the viewable area can be temporarily obscured by the toolbar. In this
>  * situation, we're ok with scrollable page content being obscured, but fixed
>  * position content cannot be revealed without removing the obscuring
> chrome, so
>  * we use these margins so that it can remain visible. These margins are also
>  * duplicated on the compositor, to allow a smooth transition when chrome
> appears
>  * on the screen.
>  */

So basically you have some content from outside the document floating over the document's viewport and you want to push the document's fixed-pos content inward, away from that floating content?

Comment 80

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

Comment on attachment 716442 [details] [diff] [review]
Part 5 - Add fixed position margins to layout

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

r+ with that comment in the right place

Comment 81

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Scroll header off the top

This is rebased, and fixes input in the area underneath the toolbar.

Comment 82

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Scroll fixed layers with the toolbar

rebased, revise, carrying r=nrc, will r?kats.

Comment 83

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Make sure the top of the page is accessible

Rebased, revised.

Comment 84

[Chris Lord [:cwiiis]]

Attachment: Part 4 - Resize viewport depending on page height

Although this is functionality equivalent to what was there before, I think it's changed enough to warrant re-review.

Comment 85

[Chris Lord [:cwiiis]]

Attachment: Part 5 - Add fixed position margins to layout

Addressed dholbert's and roc's comments, carrying r=roc,dholbert.

Comment 86

[Chris Lord [:cwiiis]]

Attachment: Part 6 - Add async fixed margin setting

Rebased, revised.

Comment 87

[Chris Lord [:cwiiis]]

Attachment: Part 7 - Use async margins so input on offset fixed elements works

Rebased, revised. I'm not a huge fan of setting mForceRedraw here, but otherwise the margins very easily get out of sync, and we want to avoid that whenever possible. (and we certainly want to avoid them remaining out of sync once animations end)

Comment 88

[Chris Lord [:cwiiis]]

I realise there may still be some small issues after this series of patches, but I'm a fan of landing these and dealing with them in other bugs - rebasing this was a bit of a pain...

Comment 89

[Chris Lord [:cwiiis]]

Plain try build: https://tbpl.mozilla.org/?tree=Try&rev=72e15d3b7c8a
Try build with part 1: https://tbpl.mozilla.org/?tree=Try&rev=78a97f237efc
Try build with everything: https://tbpl.mozilla.org/?tree=Try&rev=1c39fa0fc999

I think part 1 can land on its own, the other parts probably ought to land together.

Comment 90

[Nick Cameron [:nrc]]

Comment on attachment 718525 [details] [diff] [review]
Part 6 - Add async fixed margin setting

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

r=me with an improvement to the comment

::: gfx/layers/ipc/CompositorParent.cpp
@@ +667,2 @@
>      } else {
> +      translation.y += aFixedLayerMargins.top - fixedMargins.top;

I would like some clue in either the names or the comments about the difference between aFixedLayerMargins and fixedMargins.

Does 'Gecko-side' mean not on the compositor (content-side) or not in Java/js?

Comment 91

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 718514 [details] [diff] [review]
Part 1 - Scroll header off the top

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

Some minor things that can be handled in a follow-up patch or bug(s) if you don't want to do it here.

::: mobile/android/base/BrowserApp.java
@@ +86,5 @@
>      private FindInPageBar mFindInPageBar;
>  
> +    // Variables used for scrolling the toolbar on/off the page.
> +    private static final int TOOLBAR_ONLOAD_HIDE_DELAY = 2000;
> +    private float mLastY = 0.0f;

nit: Rename this to mLastTouchY instead?

@@ +134,5 @@
> +                    invalidateOptionsMenu();
> +
> +                    // Show the toolbar.
> +                    mBrowserToolbar.animateVisibility(true, 0);
> +                    mCurrentTabLoading = true;

This patch has the problem that if you switch from a loading tab to a completely loaded tab, you get stuck with mCurrentTabLoading = true, and the toolbar never hides. You can use https://staktrace.com/pub/slow.php as a 10-second page load to verify. Instead of use mCurrentTabLoading, it might be better to just do Tabs.getInstance().selectedTab().getState() == Tab.STATE_LOADING where you need it.

@@ +210,5 @@
> +                mToolbarLocked = false;
> +                mToolbarSubpixelAccumulation = 0.0f;
> +                mLastY = event.getY();
> +                return super.onInterceptTouchEvent(view, event);
> +            } else {

nit: else after return is unneeded.

Comment 92

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 718517 [details] [diff] [review]
Part 2 - Scroll fixed layers with the toolbar

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

Again, changes can be done in followups. No major issues here.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +888,4 @@
>      TransformFixedLayers(
>        aLayer,
>        -treeTransform.mTranslation / treeTransform.mScale,
> +      treeTransform.mScale, fixedLayerMargins);

nit: bump the last argument down to the next line for consistency.

::: mobile/android/base/BrowserToolbarLayout.java
@@ +34,5 @@
> +    @Override
> +    protected void onScrollChanged(int l, int t, int oldl, int oldt) {
> +        refreshMargins();
> +
> +        super.onScrollChanged(l, t, oldl, oldt);

Probably doesn't matter, but I would have put the call to super above the call to refreshMargins(), in case the super modifies getHeight() or getScrollY() for whatever reason. Same with onSizeChanged().

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +98,1 @@
>      public GeckoLayerClient(Context context, LayerView view, EventDispatcher eventDispatcher) {

nit: remove blank line

@@ +294,5 @@
>              switch (type) {
>              default:
>              case UPDATE:
>                  // Keep the old viewport size
> +                metrics = messageMetrics.setViewportSize(oldMetrics.getWidth(), oldMetrics.getHeight(), oldMetrics);

I think I'd prefer making this line look like:

messageMetrics.setViewportSize(oldMetrics.getWidth(), oldMetrics.getHeight())
              .setFixedLayerMarginsFrom(oldMetrics)

and then updating ImmutableViewportMetrics accordingly (i.e. get rid of the changes to setViewportSize and the new setViewportSize function, add a new setFixedLayerMarginsFrom(ImmutableViewportMetrics) function).

Comment 93

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 718519 [details] [diff] [review]
Part 3 - Make sure the top of the page is accessible

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

I think this one has a bug, but it won't hit often and can be fixed in a follow-up.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +560,5 @@
> +
> +        // Adjust the fixed layer margins so that overscroll subtracts from them.
> +        mCurrentViewTransform.fixedLayerMarginLeft =
> +            Math.max(0, mFrameMetrics.fixedLayerMarginLeft +
> +                     Math.min(0, mFrameMetrics.viewportRectLeft));

This line should be Math.min(0, mFrameMetrics.viewportRectLeft - mFrameMetrics.pageRectLeft)

I think this actually makes a difference on RTL pages where the pageRectLeft can be negative and the viewportRectLeft can be negative and not in overscroll. Same change applies to the next statement (the "top" case).

Comment 94

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 718521 [details] [diff] [review]
Part 4 - Resize viewport depending on page height

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

This version looks better, thanks!

::: mobile/android/chrome/content/browser.js
@@ +3271,5 @@
> +    // ratio calculation below works correctly in these situations.
> +    // We take away the margin size over two to account for rounding errors,
> +    // as the browser size set in updateViewportSize doesn't allow for any
> +    // size between these two values (and thus anything between them is
> +    // attributable to rounding error).

Good comment :)

Comment 95

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

Comment on attachment 718521 [details] [diff] [review]
Part 4 - Resize viewport depending on page height


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>   setScrollClampingSize: function(zoom) {

>+    if ((pageHeight * zoom) < gScreenHeight - (gViewportMargins.top + gViewportMargins.bottom)/2) {

>+    if ((pageWidth * zoom) < gScreenWidth - (gViewportMargins.left + gViewportMargins.right)/2) {

nit: spaces around the last ")/2"

Comment 96

[Chris Lord [:cwiiis]]

Ref tests not looking so good - The failure for patch 1 is likely that the modified layout causes some slightly different sub-pixel alignment. I expect we'll just have to fuzz this (it's quite a high amount of fuzz, but there's no actual visible difference).

The failures in subsequent patches I'm not really sure about either - possibly margins being set when they shouldn't (do tests get run with the chrome visible? Do we have a flag to check so we can disable margins in this case?)

Thanks for the fast reviews everyone :)

Comment 97

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 718529 [details] [diff] [review]
Part 7 - Use async margins so input on offset fixed elements works

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

Another bug that affects RTL pages only. Can be fixed in a follow-up patch if you prefer.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +257,5 @@
> +        // unnecessary.
> +        if ((metrics.fixedLayerMarginLeft > 0 && metrics.viewportRectLeft < 0) ||
> +            (metrics.fixedLayerMarginTop > 0 && metrics.viewportRectTop < 0) ||
> +            (metrics.fixedLayerMarginRight > 0 && metrics.pageRectRight - metrics.viewportRectRight < 0) ||
> +            (metrics.fixedLayerMarginBottom > 0 && metrics.pageRectBottom - metrics.viewportRectBottom < 0)) {

I think the first two lines of this condition need to take into account metrics.viewportRectLeft and metrics.viewportRectTop as well. Also I'd prefer rewriting the comparison clauses like so:

(metrics.fixedLayerMarginLeft > 0 && metrics.viewportRectLeft < metrics.pageRectLeft)
(metrics.fixedLayerMarginRight > 0 && metrics.viewportRectRight > metrics.pageRectRight)

(and similarly for top and bottom). It's just clearer to me to have a comparison between two values rather than a subtraction and a compare to zero.

@@ +260,5 @@
> +            (metrics.fixedLayerMarginRight > 0 && metrics.pageRectRight - metrics.viewportRectRight < 0) ||
> +            (metrics.fixedLayerMarginBottom > 0 && metrics.pageRectBottom - metrics.viewportRectBottom < 0)) {
> +            clampedMetrics = clampedMetrics.setFixedLayerMargins(
> +              Math.max(0, metrics.fixedLayerMarginLeft + Math.min(0, metrics.viewportRectLeft)),
> +              Math.max(0, metrics.fixedLayerMarginTop + Math.min(0, metrics.viewportRectTop)),

I believe these two lines also need to take into the pageRectLeft and pageRectTop.

Comment 98

[Chris Lord [:cwiiis]]

Attachment: Part 8 - Hide behind a pref, default off

I think addressing kats' ltr comments will fix some of the failures, but not all of them - We should probably add more tests for the layout features and for the UI feature too, if possible.

This patch does what's necessary to be able to turn the feature on/off and defaults it to off (accessible via browser.chrome.dynamictoolbar).

Try build in progress: https://tbpl.mozilla.org/?tree=Try&rev=2ab5f9998e93

Comment 99

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #98)
> Try build in progress: https://tbpl.mozilla.org/?tree=Try&rev=2ab5f9998e93

It's green, it's green!!!

Comment 100

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 719424 [details] [diff] [review]
Part 8 - Hide behind a pref, default off

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

I'd like to see an updated version of this patch with at least the last comment (TouchEventHandler) addressed. The rest aren't as serious but if you can address those as well that would be great. I'm concerned that the change to TouchEventHandler will introduce subtle errors in our pan/zoom code.

::: mobile/android/base/BrowserApp.java
@@ +394,5 @@
> +        // Load the dynamic toolbar pref
> +        JSONArray prefs = new JSONArray();
> +        prefs.put(PREF_CHROME_DYNAMICTOOLBAR);
> +
> +        PrefsHelper.getPrefs(prefs, new PrefsHelper.PrefHandlerBase() {

Use PrefsHelper.getPref(String, ...). No need for the JSONArray here.

@@ +401,5 @@
> +            @Override public void prefValue(String pref, boolean value) {
> +                mValues.put(pref, value);
> +            }
> +
> +            @Override public void finish() {

You can put all this code directly in the prefValue() function; no need to store the boolean in mValues and do it in finish.

@@ +972,5 @@
> +                    int toolbarHeight = mBrowserToolbar.getLayout().getHeight();
> +                    mAboutHomeContent.setPadding(0, toolbarHeight, 0, 0);
> +                    if (!mDynamicToolbarEnabled) {
> +                        mLayerView.setPadding(0, toolbarHeight, 0, 0);
> +                    }

Is setting the padding on the layerview needed here? This is just when about:home is shown, and the layerview shouldn't be modified at all since it was created/initialized. If it's not needed I'd rather get rid of it as it's a bit confusing.

::: mobile/android/base/BrowserToolbarLayout.java
@@ +54,5 @@
> +        // viewport margins (to stop the toolbar from obscuring fixed-pos
> +        // content).
> +        GeckoAppShell.sendEventToGecko(
> +            GeckoEvent.createBroadcastEvent("Viewport:FixedMarginsChanged",
> +                "{ \"top\" : " + topMargin + ", \"right\" : 0, \"bottom\" : 0, \"left\" : 0 }"));

This is now getting sent on each call to onScrollChanged as well, is that intentional? It's not a huge deal but if it's not needed it would be nice to avoid flooding gecko with these messages.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +202,5 @@
>  
>          IntSize newScreenSize = new IntSize(metrics.widthPixels, metrics.heightPixels);
> +        IntSize newWindowSize = new IntSize(
> +            mView.getWidth() - mView.getPaddingLeft() - mView.getPaddingRight(),
> +            mView.getHeight() - mView.getPaddingTop() - mView.getPaddingBottom());

I'm going to have to do this in my patch 1 of bug 844275 too, right? (Assuming you land first; if I land first you'll need to update that code too)

::: mobile/android/base/gfx/TouchEventHandler.java
@@ +142,5 @@
>  
>      /* This function MUST be called on the UI thread */
>      public boolean handleEvent(MotionEvent event) {
> +        // Modify the touch event coordinates if there's padding on the View
> +        event.offsetLocation(-mView.getPaddingLeft(), -mView.getPaddingTop());

Hmm, I don't think this is the right place for this. I'd rather it be done in LayerView, in each of the onTouchEvent/onHoverEvent/onGenericMotionEvent methods.

Comment 101

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #100)
> Comment on attachment 719424 [details] [diff] [review]
> Part 8 - Hide behind a pref, default off
> 
> Review of attachment 719424 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to see an updated version of this patch with at least the last
> comment (TouchEventHandler) addressed. The rest aren't as serious but if you
> can address those as well that would be great. I'm concerned that the change
> to TouchEventHandler will introduce subtle errors in our pan/zoom code.

All comments addressed, coming just as soon as I make sure it still works.

> ::: mobile/android/base/gfx/GeckoLayerClient.java
> @@ +202,5 @@
> >  
> >          IntSize newScreenSize = new IntSize(metrics.widthPixels, metrics.heightPixels);
> > +        IntSize newWindowSize = new IntSize(
> > +            mView.getWidth() - mView.getPaddingLeft() - mView.getPaddingRight(),
> > +            mView.getHeight() - mView.getPaddingTop() - mView.getPaddingBottom());
> 
> I'm going to have to do this in my patch 1 of bug 844275 too, right?
> (Assuming you land first; if I land first you'll need to update that code
> too)
> 
> ::: mobile/android/base/gfx/TouchEventHandler.java
> @@ +142,5 @@
> >  
> >      /* This function MUST be called on the UI thread */
> >      public boolean handleEvent(MotionEvent event) {
> > +        // Modify the touch event coordinates if there's padding on the View
> > +        event.offsetLocation(-mView.getPaddingLeft(), -mView.getPaddingTop());
> 
> Hmm, I don't think this is the right place for this. I'd rather it be done
> in LayerView, in each of the onTouchEvent/onHoverEvent/onGenericMotionEvent
> methods.


Correct. Ping me when you land that and vice-versa.

Comment 102

[Kartikaya Gupta (email:[e-mail])]

FYI, I landed bug 844275. However I believe the new approach we discussed for the pref-disabled patch shouldn't need this code, and so shouldn't be impacted.

Comment 103

[Chris Lord [:cwiiis]]

Attachment: Part 8 - Hide behind a pref, default off

This does the same thing the way we discussed - it'd be possible to not modify the layout and do it in code, but this seemed far easier and more robust.

Comment 104

[Chris Lord [:cwiiis]]

Attachment: Part 8

Sorry, left some import cruft from the previous patch.

Comment 105

[Chris Lord [:cwiiis]]

And a try push: https://tbpl.mozilla.org/?tree=Try&rev=4b26fa82223f

Comment 106

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #105)
> And a try push: https://tbpl.mozilla.org/?tree=Try&rev=4b26fa82223f

ok, this one is actually green, including robocop :)

Comment 107

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 719902 [details] [diff] [review]
Part 8

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

LGTM. I'm probably not the best person to review the XML change, but I assume any breakage there will be caught quickly. Some comments below but nothing that blocks landing.

::: mobile/android/base/BrowserApp.java
@@ +357,5 @@
>          JavaAddonManager.getInstance().init(getApplicationContext());
> +
> +        // Load the dynamic toolbar pref
> +        PrefsHelper.getPref(PREF_CHROME_DYNAMICTOOLBAR, new PrefsHelper.PrefHandlerBase() {
> +            @Override public void prefValue(String pref, boolean value) {

For consistency, please leave the @Override on a line of its own, and move public down to the next line

@@ +444,5 @@
>      }
>  
> +    public void setToolbarHeight(int aHeight, int aVisibleHeight) {
> +        if (!mDynamicToolbarEnabled ||
> +            (mAboutHomeShowing != null && mAboutHomeShowing)) {

You should be able to do Boolean.TRUE.equals(mAboutHomeShowing) instead of the null check && boolean check. I don't know if that's more readable though.

@@ +445,5 @@
>  
> +    public void setToolbarHeight(int aHeight, int aVisibleHeight) {
> +        if (!mDynamicToolbarEnabled ||
> +            (mAboutHomeShowing != null && mAboutHomeShowing)) {
> +            mToolbarSpacer.setPadding(0, aVisibleHeight, 0, 0);

Maybe add a comment here as to why this is using aVisibleHeight rather than aHeight (basically what you told me on IRC), since it wasn't obvious at first glance.

Comment 108

[Chris Lord [:cwiiis]]

Pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/43826205de85
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/050032870b72
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c0077efaaa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f91e3eea5d13
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/de86401bc11b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dde8f72c6aca
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d2e3890569a7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/51b94bfe643a

Adding leave-open, need to work on getting tests passing to enable the feature by default...

Comment 109

[Kartikaya Gupta (email:[e-mail])]

I'd rather file a new bug for getting the feature enabled. This bug is getting pretty long and it's going to be hard to follow a new set of patches.

Comment 110

[Chris Lord [:cwiiis]]

You're right, I'll file another bug that we can use as a meta-bug to track the issues we need to fix before enabling it.

Comment 111

[Ryan VanderMeulen [:RyanVM]]

Sorry, but I had to back this out for frequent Android mochitest-8 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/23d0ee5b4483

https://tbpl.mozilla.org/php/getParsedLog.php?id=20226522&tree=Mozilla-Inbound

271 INFO TEST-START | /tests/editor/libeditor/base/tests/test_bug795785.html
272 INFO TEST-INFO | /tests/editor/libeditor/base/tests/test_bug795785.html | must wait for load
273 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | textarea's scrollTop isn't 0
274 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | textarea was not scrolled by inserting line breaks
275 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | textarea was not scrolled by up key events
276 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | textarea was not scrolled by down key events
277 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | textarea was not scrolled by typing long word
278 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | textarea was not scrolled by left key events
279 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | textarea was not scrolled by right key events
280 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | div (contenteditable)'s scrollTop isn't 0
281 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | div (contenteditable) was not scrolled by inserting line breaks
282 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | div (contenteditable) was not scrolled by up key events
283 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | div (contenteditable) was not scrolled by down key events
284 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | div (contenteditable) was not scrolled by typing long word
285 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | div (contenteditable) was not scrolled by left key events
286 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | div (contenteditable) was not scrolled by right key events
287 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | textarea's scrollTop isn't 0
288 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | textarea was not scrolled by composition
289 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | textarea was not scrolled back to the top by canceling composition
290 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | div (contenteditable)'s scrollTop isn't 0
291 INFO TEST-PASS | /tests/editor/libeditor/base/tests/test_bug795785.html | div (contenteditable) was not scrolled by composition
292 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/base/tests/test_bug795785.html | div (contenteditable) was not scrolled back to the top by canceling composition - got 370, expected 0
293 INFO TEST-END | /tests/editor/libeditor/base/tests/test_bug795785.html | finished in 3123ms

Comment 112

[Chris Lord [:cwiiis]]

(In reply to Daniel Holbert [:dholbert] from comment #67)
> Comment on attachment 716442 [details] [diff] [review]
> Part 5 - Add fixed position margins to layout
> >   /**
> >+   * Calls FrameNeedsReflow on all fixed position children of the root frame.
> >+   */
> >+  virtual void ReflowFixedPositionChildren(IntrinsicDirty aIntrinsicDirty);
> 
> This function name is misleading -- it makes it sound like it synchronously
> reflows stuff (which it doesn't).
> 
> Maybe "RequestReflowForFixedPositionChildren"?  (I also don't love using the
> term "Children" here, because the pres shell doesn't have "children"... I
> don't have any better suggestions though.)

I forgot to address this, so seeing as I've been backed out, I've taken the opportunity to rename this to MarkFixedFramesForReflow - This better reflects what the function actually does.

Comment 113

[Chris Lord [:cwiiis]]

So it seems there are intermittent failures on this test (libeditor/base/tests/test_bug795785.html) after just part 1 of this patch, and even after part 8 that puts all of this behind a pref and disables it by default... needinfo'ing Ehsan to see if he has any ideas.

Comment 114

[Chris Lord [:cwiiis]]

Attachment: Part 8 - Hide behind a pref, default off

More recent version of part 8 that fixes a couple of bugs (carrying r=kats, confirmed offline)

Comment 115

[Chris Lord [:cwiiis]]

This is the most recent patch queue with M8 retried a few times: https://tbpl.mozilla.org/?tree=Try&rev=92ecb8776341

Comment 116

[Chris Lord [:cwiiis]]

Went over this failure with cpeterson - I think it's just a racy test. It does some work, then it pumps the event loop 20 times, waits 20ms and tests things. I think the UI change has caused some extra work to happen at some point that has caused this intermittent failure. I imagine just upping the 20ms a bit would fix this - it's an ugly fix, but we can file a bug to fix this test properly if it's required (I guess it passed review, so perhaps it isn't?)

Comment 117

[Chris Lord [:cwiiis]]

Attachment: Part 9 - Fix intermittent failure on test from bug 795785

So 100ms seemed safe. 40ms still gave oranges 1 in 10 times.

Try push with 40ms: https://tbpl.mozilla.org/?tree=Try&rev=eb5a62ce325d
Try push with 100ms: https://tbpl.mozilla.org/?tree=Try&rev=00e22a9bcff4

I'll file a separate bug about this test, as it's liable to break again at some point. I'd like to land a quick fix so I can rid myself of having to maintain this patch queue while there's still development to be done on it.

Comment 118

[(no longer active)]

Comment on attachment 721730 [details] [diff] [review]
Part 9 - Fix intermittent failure on test from bug 795785

Yeah, unfortunately there is no good way for us to test for scrolling here, so if this fixes the test failures for you, then so be it!

Comment 119

[Chris Lord [:cwiiis]]

Thanks for the quick review Ehsan, pushed to inbound again:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/66cac6d99503
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5d0c3b244e89
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2d0a1504d138
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/487578151a1a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f065ac54e83e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7f0e7e4af43
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b94646221356
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/425903ba777d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5948f1c95fbe

Fingers crossed...

Comment 120

[Chris Lord [:cwiiis]]

(In reply to :Ehsan Akhgari from comment #118)
> Comment on attachment 721730 [details] [diff] [review]
> Part 9 - Fix intermittent failure on test from bug 795785
> 
> Yeah, unfortunately there is no good way for us to test for scrolling here,
> so if this fixes the test failures for you, then so be it!

Is it worth me filing a bug about this test, or shall we just leave it as is?

Comment 121

[Chris Lord [:cwiiis]]

In case there were any concerns, I was aware of bug 843719 and reordered the necessary bits accordingly (I hope).

Comment 122

[(no longer active)]

(In reply to comment #120)
> (In reply to :Ehsan Akhgari from comment #118)
> > Comment on attachment 721730 [details] [diff] [review]
> > Part 9 - Fix intermittent failure on test from bug 795785
> > 
> > Yeah, unfortunately there is no good way for us to test for scrolling here,
> > so if this fixes the test failures for you, then so be it!
> 
> Is it worth me filing a bug about this test, or shall we just leave it as is?

Hmm, well you're definitely welcome to file a bug, but I don't have any idea what we would want to do there.

Comment 123

[Ryan VanderMeulen [:RyanVM]]

Backed out for robocop crashes on Android 4.0.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f266cb6f39

https://tbpl.mozilla.org/php/getParsedLog.php?id=20390420&tree=Mozilla-Inbound

0 INFO SimpleTest START
1 INFO TEST-START | testNewTab
2 INFO TEST-PASS | testNewTab | Checking elements - all elements present
3 INFO TEST-PASS | testNewTab | Initial number of tabs correct - 1 should equal 1
4 INFO TEST-PASS | testNewTab | checking that tabs clicked - tabs element clicked
5 INFO TEST-PASS | testNewTab | waiting for add tab view - add tab view available
6 INFO TEST-PASS | testNewTab | checking that add_tab clicked - add_tab element clicked
INFO | automation.py | Application ran for: 0:00:08.328657
INFO | automation.py | Reading PID log: /tmp/tmp81MCyKpidlog
PROCESS-CRASH | java-exception | java.lang.ClassCastException: android.widget.RelativeLayout$LayoutParams cannot be cast to android.widget.LinearLayout$LayoutParams	at org.mozilla.gecko.BrowserApp.onPropertyAnimationEnd(BrowserApp.java:885)
WARNING | automationutils.processLeakLog() | refcount logging is off, so leaks can't be detected!

INFO | runtests.py | Running tests: end.

Comment 124

[Chris Lord [:cwiiis]]

Kind of amazed this crash didn't come up before... Possibly some new code and an incomplete rebase... Anyway, pushed again:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2c41f3de51
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f408fd0979
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e88b3eb677fe
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/14feb7d2c538
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd870f4a55b6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bfda0f5c7a12
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/683c7068d3be
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2f49f5198e20
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/893517236e48

Fingers crossed...

Comment 125

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf2c41f3de51
https://hg.mozilla.org/mozilla-central/rev/b0f408fd0979
https://hg.mozilla.org/mozilla-central/rev/e88b3eb677fe
https://hg.mozilla.org/mozilla-central/rev/14feb7d2c538
https://hg.mozilla.org/mozilla-central/rev/cd870f4a55b6
https://hg.mozilla.org/mozilla-central/rev/bfda0f5c7a12
https://hg.mozilla.org/mozilla-central/rev/683c7068d3be
https://hg.mozilla.org/mozilla-central/rev/2f49f5198e20
https://hg.mozilla.org/mozilla-central/rev/893517236e48

Comment 126

[Aaron Train [:aaronmt]]

This broke launching of web-apps (bug 849539) on trunk

Comment 127

[Chris Lord [:cwiiis]]

(In reply to Aaron Train [:aaronmt] from comment #126)
> This broke launching of web-apps (bug 849539) on trunk

A simple bug, can this wait until Monday? I'd like to avoid a back-out.

Comment 128

[Aaron Train [:aaronmt]]

(In reply to Chris Lord [:cwiiis] from comment #127)
> (In reply to Aaron Train [:aaronmt] from comment #126)
> > This broke launching of web-apps (bug 849539) on trunk
> 
> A simple bug, can this wait until Monday? I'd like to avoid a back-out.

yeah

Comment 129

[Alex Keybl [:akeybl]]

This bug has been listed as part of the Aurora 22 release notes in either [1], [2], or both. If you find any of the information or wording incorrect in the notes, please email [e-mail] asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/

Comment 130

[Teodora Vermesan (:TeoVermesan)]

Test Cases added:
https://moztrap.mozilla.org/manage/cases/_detail/43714/
https://moztrap.mozilla.org/manage/cases/_detail/43718/
https://moztrap.mozilla.org/manage/cases/_detail/43724/

Comment 132

[Alex Keybl [:akeybl]]

Disabled in FF22 in bug 867637, so relnote?

Comment 133

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/firefox/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.

Comment 134

[Gabriela [:gaby2300]]

I and cuantocarlos(a Mozilla Hispano QA contributor) can confirm the bug is fixed using the latest beta.