Bug 716403 (dynamic-toolbar)

Request to hide the navigation bar when scrolling down content

RESOLVED FIXED in Firefox 23

Status

()

enhancement
RESOLVED FIXED
7 years ago
11 months ago

People

(Reporter: bug.zilla, Assigned: cwiiis)

Tracking

(Blocks 2 bugs, {uiwanted})

Firefox Tracking Flags

(firefox22 disabled, relnote-firefox 23+)

Details

(Whiteboard: [testday-20130823])

Attachments

(10 attachments, 20 obsolete attachments)

wip
52.99 KB, patch
Details | Diff | Splinter Review
33.89 KB, patch
kats
: review+
Details | Diff | Splinter Review
38.31 KB, patch
cwiiis
: review+
kats
: review+
Details | Diff | Splinter Review
14.95 KB, patch
kats
: review+
Details | Diff | Splinter Review
17.07 KB, patch
mfinkle
: review+
kats
: review+
Details | Diff | Splinter Review
11.53 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
12.45 KB, patch
roc
: review+
nrc
: review+
Details | Diff | Splinter Review
6.93 KB, patch
kats
: review+
Details | Diff | Splinter Review
18.09 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
965 bytes, patch
Ehsan
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
OS: Windows 7 → Android
Hardware: x86_64 → ARM
Component: General → General
Product: Fennec Native → Fennec

Comment 1

7 years ago
This is reproducible on Native Nightly: Firefox 14.0a1 build from 2012-04-03.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Component: General → General
Ever confirmed: true
Product: Fennec → Fennec Native
Version: Firefox 12 → Trunk
Keywords: uiwanted
Summary: Cannot hide Navigation bar like in Firefox 10 (i.e. the non-native version) → Request to hide the navigation bar when scrolling down content

Comment 2

7 years ago
Looks like Bug 766957 might fix this. Hooray!
(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.
(Reporter)

Comment 4

7 years ago
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.
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.
Summary: Request to hide the navigation bar when scrolling down content → Request to hide the browser toolbar when scrolling down content
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
Summary: Request to hide the browser toolbar when scrolling down content → Request to hide the navigation bar when scrolling down content
(Reporter)

Comment 7

7 years ago
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.
Here, I made a little movie from the slides here: https://dl.dropbox.com/u/2182500/mobile%20header%20interactions.mov
Duplicate of this bug: 768759
(Reporter)

Comment 11

7 years ago
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
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.
> * 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? :)
Assignee: nobody → michael.l.comella
Duplicate of this bug: 766957
Assignee: michael.l.comella → nobody
Status: ASSIGNED → NEW
Hi there, any progress on this? It's a feature that would greatly benefit both normal and Reader browsing.
Blocks: 795401
Posted patch WIP Patch (obsolete) — Splinter Review
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.
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.
(Assignee)

Comment 18

7 years ago
Posted patch WIP Patch, rebased (obsolete) — Splinter Review
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.
Attachment #669758 - Attachment is obsolete: true
Duplicate of this bug: 807808
Assignee: nobody → snorp
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.
Assignee: snorp → nobody
(Reporter)

Comment 22

6 years ago
Hi, has there been any progress on this?
(Reporter)

Comment 23

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

Comment 24

6 years ago
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.
Posted file WIP Rebased (obsolete) —
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.
Attachment #673829 - Attachment is obsolete: true
(Assignee)

Comment 26

6 years ago
(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?
(Assignee)

Comment 27

6 years ago
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.
Assignee: nobody → chrislord.net
Attachment #702950 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #706509 - Flags: review?(lucasr.at.mozilla)
Attachment #706509 - Flags: review?(bugmail.mozilla)
Attachment #706509 - Flags: feedback?(wjohnston)
(Assignee)

Comment 28

6 years ago
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 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.
Attachment #706509 - Flags: review?(bugmail.mozilla) → feedback+
(Assignee)

Comment 30

6 years ago
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).
Attachment #706509 - Attachment is obsolete: true
Attachment #706509 - Flags: review?(lucasr.at.mozilla)
Attachment #706509 - Flags: feedback?(wjohnston)
Attachment #710350 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 31

6 years ago
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.
Attachment #710351 - Flags: review?(bugmail.mozilla)
Attachment #710351 - Flags: review?(bgirard)
(Assignee)

Comment 32

6 years ago
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.
Attachment #710352 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 33

6 years ago
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 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 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 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.
(Assignee)

Comment 37

6 years ago
Excellent review, thanks :)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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.
(Assignee)

Comment 39

6 years ago
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.
Attachment #710350 - Attachment is obsolete: true
Attachment #710350 - Flags: review?(bugmail.mozilla)
Attachment #711424 - Flags: review?(bugmail.mozilla)
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.
Attachment #711424 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #710351 - Flags: review?(bgirard) → review?(bas)
What happens when doorhangers are shown with this (and what should happen?) I would guess we show the urlbar until the doorhanger is dismissed?
(Assignee)

Comment 43

6 years ago
Carrying r+ - Removed offsetting (on UX request) and rebased.
Attachment #711424 - Attachment is obsolete: true
Attachment #713995 - Flags: review+
(Assignee)

Comment 44

6 years ago
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?
Attachment #710351 - Attachment is obsolete: true
Attachment #710351 - Flags: review?(bugmail.mozilla)
Attachment #710351 - Flags: review?(bas)
Attachment #713996 - Flags: review?(bugmail.mozilla)
Attachment #713996 - Flags: review?(bgirard)
Attachment #713996 - Flags: feedback?(ncameron)
(Assignee)

Comment 45

6 years ago
Really can't remember what I've changed with this, but it works much better than the previous patch.
Attachment #710352 - Attachment is obsolete: true
Attachment #710352 - Flags: review?(bugmail.mozilla)
Attachment #713998 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 46

6 years ago
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 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!
Attachment #713996 - Flags: feedback?(ncameron) → feedback+
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.
Attachment #713996 - Flags: review?(bugmail.mozilla) → review+
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?
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 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.
Attachment #713996 - Flags: review?(bgirard) → review?(ncameron)
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.
Attachment #713996 - Flags: review?(ncameron) → review+
Let me know if you do need a proper review from me and I will try to get to grips with the code.
(Assignee)

Comment 54

6 years ago
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.
Attachment #713998 - Attachment is obsolete: true
Attachment #713998 - Flags: review?(bugmail.mozilla)
Attachment #715186 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 55

6 years ago
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).
Attachment #715187 - Flags: review?(mark.finkle)
Attachment #715187 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 56

6 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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?
Attachment #715187 - Flags: review?(mark.finkle) → review+
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.
Attachment #715186 - Flags: review?(bugmail.mozilla) → review+
(In reply to Chris Lord [:cwiiis] from comment #56)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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

Updated

6 years ago
(Assignee)

Comment 61

6 years ago
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.
Attachment #715187 - Attachment is obsolete: true
Attachment #715187 - Flags: review?(bugmail.mozilla)
Attachment #715611 - Flags: review?(mark.finkle)
Attachment #715611 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 62

6 years ago
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 :(
(Assignee)

Comment 63

6 years ago
(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.
(Assignee)

Comment 64

6 years ago
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).
Attachment #716442 - Flags: review?(dholbert)
(Assignee)

Comment 65

6 years ago
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 :)
(Assignee)

Comment 66

6 years ago
(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 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.
Attachment #716442 - Flags: review?(dholbert) → feedback+
> >+  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).
(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)
(Assignee)

Comment 70

6 years ago
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.
Attachment #716442 - Flags: review?(roc)
(Assignee)

Comment 71

6 years ago
This annotates layers with the fixed margins set in layout and adds the necessary code in CompositorParent to reconcile temporary differences.
Attachment #717065 - Flags: review?(roc)
Attachment #717065 - Flags: review?(ncameron)
(Assignee)

Comment 72

6 years ago
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.
Attachment #717159 - Flags: feedback?(bugmail.mozilla)
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?
(Assignee)

Comment 74

6 years ago
(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 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.
Attachment #715611 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 76

6 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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.
Attachment #717065 - Flags: review?(ncameron)
(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.
(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 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
Attachment #716442 - Flags: review?(roc) → review+
(Assignee)

Comment 81

6 years ago
This is rebased, and fixes input in the area underneath the toolbar.
Attachment #713995 - Attachment is obsolete: true
Attachment #718514 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 82

6 years ago
rebased, revise, carrying r=nrc, will r?kats.
Attachment #713996 - Attachment is obsolete: true
Attachment #718517 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #718517 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 83

6 years ago
Rebased, revised.
Attachment #715186 - Attachment is obsolete: true
Attachment #718519 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 84

6 years ago
Although this is functionality equivalent to what was there before, I think it's changed enough to warrant re-review.
Attachment #715611 - Attachment is obsolete: true
Attachment #715611 - Flags: review?(mark.finkle)
Attachment #718521 - Flags: review?(mark.finkle)
Attachment #718521 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 85

6 years ago
Addressed dholbert's and roc's comments, carrying r=roc,dholbert.
Attachment #716442 - Attachment is obsolete: true
Attachment #718523 - Flags: review+
(Assignee)

Comment 86

6 years ago
Rebased, revised.
Attachment #717065 - Attachment is obsolete: true
Attachment #717065 - Flags: review?(roc)
Attachment #718525 - Flags: review?(roc)
Attachment #718525 - Flags: review?(ncameron)
(Assignee)

Comment 87

6 years ago
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)
Attachment #717159 - Attachment is obsolete: true
Attachment #717159 - Flags: feedback?(bugmail.mozilla)
Attachment #718529 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 88

6 years ago
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...
(Assignee)

Comment 89

6 years ago
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 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?
Attachment #718525 - Flags: review?(ncameron) → review+
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.
Attachment #718514 - Flags: review?(bugmail.mozilla) → review+
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).
Attachment #718517 - Flags: review?(bugmail.mozilla) → review+
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).
Attachment #718519 - Flags: review?(bugmail.mozilla) → review+
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 :)
Attachment #718521 - Flags: review?(bugmail.mozilla) → review+
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"
Attachment #718521 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 96

6 years ago
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 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.
Attachment #718529 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 98

6 years ago
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
Attachment #719424 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 99

6 years ago
(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 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.
Attachment #719424 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 101

6 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
(Assignee)

Comment 103

6 years ago
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.
Attachment #719424 - Attachment is obsolete: true
Attachment #719901 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 104

6 years ago
Posted patch Part 8 (obsolete) — Splinter Review
Sorry, left some import cruft from the previous patch.
Attachment #719901 - Attachment is obsolete: true
Attachment #719901 - Flags: review?(bugmail.mozilla)
Attachment #719902 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 106

6 years ago
(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 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.
Attachment #719902 - Flags: review?(bugmail.mozilla) → review+
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.
(Assignee)

Comment 110

6 years ago
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.
Whiteboard: [leave-open]
(Assignee)

Updated

6 years ago
Blocks: 846772
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
(Assignee)

Comment 112

6 years ago
(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.
(Assignee)

Comment 113

6 years ago
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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 114

6 years ago
More recent version of part 8 that fixes a couple of bugs (carrying r=kats, confirmed offline)
Attachment #719902 - Attachment is obsolete: true
Attachment #721293 - Flags: review+
(Assignee)

Comment 115

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

Comment 116

6 years ago
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?)
(Assignee)

Comment 117

6 years ago
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.
Attachment #721730 - Flags: review?(bugs)

Comment 118

6 years ago
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!
Attachment #721730 - Flags: review?(bugs) → review+
Flags: needinfo?(ehsan)
(Assignee)

Comment 120

6 years ago
(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?
(Assignee)

Comment 121

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

Comment 122

6 years ago
(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.
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.
Flags: in-moztrap?(fennec)

Updated

6 years ago
Depends on: 849254
Depends on: 849539
This broke launching of web-apps (bug 849539) on trunk
(Assignee)

Comment 127

6 years ago
(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.
(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
Depends on: 849573
(Assignee)

Updated

6 years ago
Depends on: 850154
Depends on: 850217
Depends on: 848317
Blocks: 850690
Depends on: 850724
No longer blocks: 850690
Depends on: 850690
Alias: dynamic-toolbar
Blocks: 850690
No longer depends on: 850690
No longer blocks: 850690
Depends on: 850690
Depends on: 850783
Depends on: 850789
Depends on: 850827
Depends on: 850889
Depends on: 850923

Updated

6 years ago
Depends on: 851824
(Assignee)

Updated

6 years ago
Depends on: 852158
Depends on: 852277
Flags: in-testsuite?
(Assignee)

Updated

6 years ago
Depends on: 852565
No longer depends on: 850217

Updated

6 years ago
Depends on: 849922, 853300
(Assignee)

Updated

6 years ago
Depends on: 852955
(Assignee)

Updated

6 years ago
No longer depends on: 849922
Depends on: 853820
Depends on: 853831

Updated

6 years ago
Depends on: 854180
Depends on: 854289
No longer depends on: 854289
Depends on: 854850
Depends on: 855240
(Assignee)

Updated

6 years ago
No longer depends on: 854850
(Assignee)

Updated

6 years ago
No longer depends on: 848317
(Assignee)

Updated

6 years ago
No longer depends on: 850827
Flags: sec-review?
Depends on: 856252
(Assignee)

Updated

6 years ago
Depends on: 856932
Depends on: 857265
Depends on: 853986
(Assignee)

Updated

6 years ago
Depends on: 855019
Depends on: 858550
No longer depends on: 858550
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 release-mgmt@mozilla.com asap. Thanks!

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

Updated

6 years ago
Depends on: 859100
(Assignee)

Updated

6 years ago
Depends on: 859031
Depends on: 860054
Flags: sec-review? → sec-review?(dveditz)
(Assignee)

Updated

6 years ago
Depends on: 860497

Updated

6 years ago
Depends on: 863437
Depends on: 864554
(Assignee)

Updated

6 years ago
Depends on: 865298
Depends on: 865872
Depends on: 866461
Duplicate of this bug: 866491
Depends on: 867262
Depends on: 867281
Depends on: 868052
Depends on: 868607
No longer depends on: 868607
Depends on: 868998
Depends on: 869411
(Assignee)

Updated

6 years ago
No longer depends on: 863437
(Assignee)

Updated

6 years ago
Depends on: 873016
Disabled in FF22 in bug 867637, so relnote?
Depends on: 866772
Depends on: 869156
Depends on: 872961
Depends on: 873958

Updated

6 years ago
Depends on: 874247
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.
Depends on: 876743

Updated

6 years ago
Depends on: 876562
Target Milestone: Firefox 22 → Firefox 23
Depends on: 886077
Depends on: 886576
Depends on: 858483
Depends on: 888659
Depends on: 888690
Blocks: 889412
Depends on: 892246
Depends on: 896547
No longer depends on: 896547
Depends on: 898877
Depends on: 896547
Depends on: 900091
Depends on: 902138
Depends on: 903473
Depends on: 872528
Depends on: 906421

Updated

6 years ago
Depends on: 906426
No longer depends on: 906426
I and cuantocarlos(a Mozilla Hispano QA contributor) can confirm the bug is fixed using the latest beta.
Whiteboard: [testday-20130823]
Depends on: 911152
Depends on: 955886
Flags: sec-review?(dveditz)
You need to log in before you can comment on or make changes to this bug.