Closed Bug 935473 Opened 6 years ago Closed 6 years ago

Exiting editing mode while forward button is visible might leave it visible and disabled

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
firefox26 - wontfix
firefox27 + fixed
firefox28 + fixed
fennec 27+ ---

People

(Reporter: lucasr, Assigned: lucasr)

Details

(Keywords: reproducible)

Attachments

(7 files)

Our current UI never shows a disabled forward button in the toolbar. The assumption here is that the forward button will be hidden whenever the action is not available.

However, with the introduction of editing mode in the toolbar (as part of the new about:home stuff), we might end up with a visible forward button in disabled state.

Steps to reproduce:
- Open fennec, go to any URL
- Go back to previous page (forward button will slide in now)
- Tap on any link and enter editing mode while the next page loads (forward button is still visible here, in disabled state while editing)
- Exit editing mode (e.g. press Android's BACK button).

Expected results: forward button slides away
Actual results: forward button stays visible with disabled state
tracking-fennec: --- → ?
Keywords: reproducible
Attachment #828039 - Flags: review?(sriram)
Attachment #828040 - Flags: review?(wjohnston)
Attachment #828041 - Flags: review?(wjohnston)
Attachment #828042 - Flags: review?(wjohnston)
tracking-fennec: ? → 26+
Comment on attachment 828040 [details] [diff] [review]
Avoid unnecessary animations in forward button (r=wesj)

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

::: mobile/android/base/BrowserToolbar.java
@@ +1664,5 @@
>          if (mForward.getVisibility() != View.VISIBLE)
>              return;
>  
> +        MarginLayoutParams fwdParams = (MarginLayoutParams) mForward.getLayoutParams();
> +        if ((fwdParams.leftMargin > 0 && enabled) || (fwdParams.leftMargin == 0 && !enabled)) {

Can you add a little comment about what this does?
Attachment #828040 - Flags: review?(wjohnston) → review+
Comment on attachment 828041 [details] [diff] [review]
Factor out method to animate forward button's visibility (r=wesj)

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

This is mostly a rename it looks like

::: mobile/android/base/BrowserToolbar.java
@@ +1652,5 @@
>      public void updateBackButton(boolean enabled) {
>          setButtonEnabled(mBack, enabled);
>      }
>  
> +    private void animateForwardButtonVisibility(final boolean visible) {

Since you're changing this, don't pass booleans. Also, animateForwardButton seems more concise to me. TBH, I'd like to move all this into a forwardButton class and clean it out of the toolbar...

@@ +1715,4 @@
>          mForwardAnim.start();
>      }
>  
> +    public void updateForwardButton(boolean enabled) {

We should probably change this as well. Or maybe just rename? setForwardEnabled(boolean) I could live with.
Attachment #828041 - Flags: review?(wjohnston) → review-
Comment on attachment 828042 [details] [diff] [review]
Update forward button visibility when exiting editing mode (r=wesj)

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

Hmm... part of me wants updateForwardButton to take a parameter for this, but I just suggested we change it to setForwardButtonEnabled(bool) which makes that less appealing.... I think we need to better doc what's going on. Maybe it should be setAndAnimateForwardEnabled(bool) to make it clear what's going on....

::: mobile/android/base/BrowserToolbar.java
@@ +1344,5 @@
>              setButtonEnabled(mForward, canDoForward(tab));
> +
> +            if (!mIsEditing) {
> +                animateForwardButtonVisibility(canDoForward(tab));
> +            }

This needs a comment explaining what we're doing and why...
Attachment #828042 - Flags: review?(wjohnston) → review+
Attachment #828039 - Flags: review?(sriram) → review+
(In reply to Wesley Johnston (:wesj) from comment #5)
> Comment on attachment 828040 [details] [diff] [review]
> Avoid unnecessary animations in forward button (r=wesj)
> 
> Review of attachment 828040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserToolbar.java
> @@ +1664,5 @@
> >          if (mForward.getVisibility() != View.VISIBLE)
> >              return;
> >  
> > +        MarginLayoutParams fwdParams = (MarginLayoutParams) mForward.getLayoutParams();
> > +        if ((fwdParams.leftMargin > 0 && enabled) || (fwdParams.leftMargin == 0 && !enabled)) {
> 
> Can you add a little comment about what this does?

Done.
(In reply to Wesley Johnston (:wesj) from comment #6)
> Comment on attachment 828041 [details] [diff] [review]
> Factor out method to animate forward button's visibility (r=wesj)
> 
> Review of attachment 828041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is mostly a rename it looks like
> 
> ::: mobile/android/base/BrowserToolbar.java
> @@ +1652,5 @@
> >      public void updateBackButton(boolean enabled) {
> >          setButtonEnabled(mBack, enabled);
> >      }
> >  
> > +    private void animateForwardButtonVisibility(final boolean visible) {
> 
> Since you're changing this, don't pass booleans.

I don't follow. Don't pass booleans where? The point of this patch is to split the animation bits out of updateForwardButton(). I still need the boolean argument, no?

> Also, animateForwardButton
> seems more concise to me. TBH, I'd like to move all this into a
> forwardButton class and clean it out of the toolbar...

animateForwardButton() seems better indeed, done. I'm planning to factor out the back/forward bits from the toolbar as part of my refactorings under bug 917891.

> @@ +1715,4 @@
> >          mForwardAnim.start();
> >      }
> >  
> > +    public void updateForwardButton(boolean enabled) {
> 
> We should probably change this as well. Or maybe just rename?
> setForwardEnabled(boolean) I could live with.

updateForwardButton() doesn't only set the forward button's 'enabled' state. It also animates it in and out. I feel like updateForwardButton() better matches the intent of the method.
Flags: needinfo?(wjohnston)
(In reply to Wesley Johnston (:wesj) from comment #7)
> Comment on attachment 828042 [details] [diff] [review]
> Update forward button visibility when exiting editing mode (r=wesj)
> 
> Review of attachment 828042 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm... part of me wants updateForwardButton to take a parameter for this,
> but I just suggested we change it to setForwardButtonEnabled(bool) which
> makes that less appealing.... I think we need to better doc what's going on.
> Maybe it should be setAndAnimateForwardEnabled(bool) to make it clear what's
> going on....

I agree updateForwardButton might sound a bit generic/vague but it at least doesn't imply that it only sets the 'enabled' state on the button. setAndAnimateForwardEnabled() sounds a bit weird :-) Naming is hard...

> ::: mobile/android/base/BrowserToolbar.java
> @@ +1344,5 @@
> >              setButtonEnabled(mForward, canDoForward(tab));
> > +
> > +            if (!mIsEditing) {
> > +                animateForwardButtonVisibility(canDoForward(tab));
> > +            }
> 
> This needs a comment explaining what we're doing and why...

Good point. Done.
(In reply to Lucas Rocha (:lucasr) from comment #9)
> I don't follow. Don't pass booleans where? The point of this patch is to
> split the animation bits out of updateForwardButton(). I still need the
> boolean argument, no?
I meant don't pass boolean arguments to methods. They're vague. i.e. given: animateForwardButtonVisibility(true), what does the true mean? We could do an enum, or even something like  animateForwardButtonVisibility(View.VISIBLE) is clearer.

> updateForwardButton() doesn't only set the forward button's 'enabled' state.
> It also animates it in and out. I feel like updateForwardButton() better
> matches the intent of the method.

Heh. I don't have any idea what updateForwardButton(enabled) does. I don't have any better ideas off the top of my head now either. I guess that means it lives on. We could try documenting the method (we should probably do that anyway :) ), but that doesn't honestly help someone better understand what's going on when they see this used somewhere else in the code.
Flags: needinfo?(wjohnston)
(In reply to Wesley Johnston (:wesj) from comment #11)
> (In reply to Lucas Rocha (:lucasr) from comment #9)
> > I don't follow. Don't pass booleans where? The point of this patch is to
> > split the animation bits out of updateForwardButton(). I still need the
> > boolean argument, no?
> I meant don't pass boolean arguments to methods. They're vague. i.e. given:
> animateForwardButtonVisibility(true), what does the true mean? We could do
> an enum, or even something like 
> animateForwardButtonVisibility(View.VISIBLE) is clearer.

Fair enough. Let me send these refactorings in a separate patchset because I'd like to keep the upliftable patches as small as possible. More patches coming in a minute.
 
> > updateForwardButton() doesn't only set the forward button's 'enabled' state.
> > It also animates it in and out. I feel like updateForwardButton() better
> > matches the intent of the method.
> 
> Heh. I don't have any idea what updateForwardButton(enabled) does. I don't
> have any better ideas off the top of my head now either. I guess that means
> it lives on. We could try documenting the method (we should probably do that
> anyway :) ), but that doesn't honestly help someone better understand what's
> going on when they see this used somewhere else in the code.

Ditto.
Comment on attachment 828041 [details] [diff] [review]
Factor out method to animate forward button's visibility (r=wesj)

Re-requesting review after clarifications. I'll cover the review comments in the following set of patches to keep the upliftable patches as simple as possible.
Attachment #828041 - Flags: review- → review?(wjohnston)
Comment on attachment 8338495 [details] [diff] [review]
Change updateBackButton/updateForwardButton to take a Tab argument (r=wesj)

Changing updateBackButton() and updateForwardButton() to get a Tab argument (instead of a vague boolean) makes the 'contract' a bit easier to read.
Attachment #8338495 - Flags: review?(wjohnston)
Comment on attachment 8338496 [details] [diff] [review]
Add comment explaining animateForwardButton's behaviour (r=wesj)

Just a bit more clarification in animateForwardButton().
Attachment #8338496 - Flags: review?(wjohnston)
Comment on attachment 8338497 [details] [diff] [review]
Change animateForwardButton() to take an animation argument (r=wesj)

Change animateForwardButton to get an enum argument instead of a boolean. Easier to read and follow.
Attachment #8338497 - Flags: review?(wjohnston)
I'm assuming this is something you're rushing to get ready in time for FF26 but we're about to go to build on our last Beta next Monday - if this is something you want to nominate for approval and uplift please make sure the risk/reward and the testing is very thorough in the nomination form so we can weigh this well against leaving it to ride with FF27.  From an outsider's perspective this looks like a lot of churn to put in a final beta & RC without time for user feedback.
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #20)
> I'm assuming this is something you're rushing to get ready in time for FF26
> but we're about to go to build on our last Beta next Monday - if this is
> something you want to nominate for approval and uplift please make sure the
> risk/reward and the testing is very thorough in the nomination form so we
> can weigh this well against leaving it to ride with FF27.  From an
> outsider's perspective this looks like a lot of churn to put in a final beta
> & RC without time for user feedback.

I agree. Given how close we are to the release, I think uplifting to FF27 is a safer way to go. Re-nominating.
tracking-fennec: 26+ → ?
Comment on attachment 828041 [details] [diff] [review]
Factor out method to animate forward button's visibility (r=wesj)

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

Sounds good. Sorry I missed this.
Attachment #828041 - Flags: review?(wjohnston) → review+
Comment on attachment 8338495 [details] [diff] [review]
Change updateBackButton/updateForwardButton to take a Tab argument (r=wesj)

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

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +665,5 @@
>          return getWidth() - mTabs.getLeft();
>      }
>  
>      private boolean canDoBack(Tab tab) {
> +        return (tab != null && tab.canDoBack() && !mIsEditing);

Who's passing a null tab in here? Are we wallpapering over a bug?
Attachment #8338495 - Flags: review?(wjohnston) → review+
Comment on attachment 8338496 [details] [diff] [review]
Add comment explaining animateForwardButton's behaviour (r=wesj)

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

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +1365,5 @@
>          setButtonEnabled(mBack, canDoBack(tab));
>      }
>  
>      private void animateForwardButton(final boolean visible) {
> +        // If the forward button is not visible, this means it's being used

There's a missing word in here "that means its <not?> being used at all in the UI"? I'd just write "If the forward button is not visible, we much be in the phone UI"
Attachment #8338496 - Flags: review?(wjohnston) → review+
Comment on attachment 8338497 [details] [diff] [review]
Change animateForwardButton() to take an animation argument (r=wesj)

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

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +1117,5 @@
> +                } else {
> +                    animation = ForwardButtonAnimation.HIDE;
> +                }
> +
> +                animateForwardButton(animation);

I'd just use a ternary here. animateForwardButton(canDoForward(tab) ? Animation.SHOW : Animation.HIDE);

@@ +1461,5 @@
> +        } else {
> +            animation = ForwardButtonAnimation.HIDE;
> +        }
> +
> +        animateForwardButton(animation);

Same.
Attachment #8338497 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #25)
> Comment on attachment 8338497 [details] [diff] [review]
> Change animateForwardButton() to take an animation argument (r=wesj)
> 
> Review of attachment 8338497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +1117,5 @@
> > +                } else {
> > +                    animation = ForwardButtonAnimation.HIDE;
> > +                }
> > +
> > +                animateForwardButton(animation);
> 
> I'd just use a ternary here. animateForwardButton(canDoForward(tab) ?
> Animation.SHOW : Animation.HIDE);

Done.

> @@ +1461,5 @@
> > +        } else {
> > +            animation = ForwardButtonAnimation.HIDE;
> > +        }
> > +
> > +        animateForwardButton(animation);
> 
> Same.

Done.
(In reply to Wesley Johnston (:wesj) from comment #24)
> Comment on attachment 8338496 [details] [diff] [review]
> Add comment explaining animateForwardButton's behaviour (r=wesj)
> 
> Review of attachment 8338496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +1365,5 @@
> >          setButtonEnabled(mBack, canDoBack(tab));
> >      }
> >  
> >      private void animateForwardButton(final boolean visible) {
> > +        // If the forward button is not visible, this means it's being used
> 
> There's a missing word in here "that means its <not?> being used at all in
> the UI"? I'd just write "If the forward button is not visible, we much be in
> the phone UI"

Fixed.
(In reply to Wesley Johnston (:wesj) from comment #23)
> Comment on attachment 8338495 [details] [diff] [review]
> Change updateBackButton/updateForwardButton to take a Tab argument (r=wesj)
> 
> Review of attachment 8338495 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +665,5 @@
> >          return getWidth() - mTabs.getLeft();
> >      }
> >  
> >      private boolean canDoBack(Tab tab) {
> > +        return (tab != null && tab.canDoBack() && !mIsEditing);
> 
> Who's passing a null tab in here? Are we wallpapering over a bug?

Yep, I had to pass null tabs in BrowserApp but it turns I can do this directly in BrowserToolbar. The null check is not necessary anymore. Fixed.
tracking-fennec: ? → 27+
Comment on attachment 828039 [details] [diff] [review]
Replace ViewGroup.MarginLayoutParams with MarginLayoutParams (r=sriram)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new about:home (bug 862793)
User impact if declined: The toolbar might be left into a invalid state on tablets if edit mode starts while a page is loading. 
Testing completed (on m-c, etc.): Landed on m-c. Let's bake it in Nightly for a day or two and then uplift to FF27. 
Risk to taking this patch (and alternatives if risky): Relatively low. These patches are mostly refactorings. The only functional change is to force animation to hide the forward button when you leave edit mode.   
String or IDL/UUID changes made by this patch: n/a
Attachment #828039 - Flags: approval-mozilla-aurora?
Comment on attachment 828040 [details] [diff] [review]
Avoid unnecessary animations in forward button (r=wesj)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new about:home (bug 862793)
User impact if declined: The toolbar might be left into a invalid state on tablets if edit mode starts while a page is loading. 
Testing completed (on m-c, etc.): Landed on m-c. Let's bake it in Nightly for a day or two and then uplift to FF27. 
Risk to taking this patch (and alternatives if risky): Relatively low. These patches are mostly refactorings. The only functional change is to force animation to hide the forward button when you leave edit mode.   
String or IDL/UUID changes made by this patch: n/a
Attachment #828040 - Flags: approval-mozilla-aurora?
Comment on attachment 828041 [details] [diff] [review]
Factor out method to animate forward button's visibility (r=wesj)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new about:home (bug 862793)
User impact if declined: The toolbar might be left into a invalid state on tablets if edit mode starts while a page is loading. 
Testing completed (on m-c, etc.): Landed on m-c. Let's bake it in Nightly for a day or two and then uplift to FF27. 
Risk to taking this patch (and alternatives if risky): Relatively low. These patches are mostly refactorings. The only functional change is to force animation to hide the forward button when you leave edit mode.   
String or IDL/UUID changes made by this patch: n/a
Attachment #828041 - Flags: approval-mozilla-aurora?
Comment on attachment 828042 [details] [diff] [review]
Update forward button visibility when exiting editing mode (r=wesj)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new about:home (bug 862793)
User impact if declined: The toolbar might be left into a invalid state on tablets if edit mode starts while a page is loading. 
Testing completed (on m-c, etc.): Landed on m-c. Let's bake it in Nightly for a day or two and then uplift to FF27. 
Risk to taking this patch (and alternatives if risky): Relatively low. These patches are mostly refactorings. The only functional change is to force animation to hide the forward button when you leave edit mode.   
String or IDL/UUID changes made by this patch: n/a
Attachment #828042 - Flags: approval-mozilla-aurora?
Ping?
Flags: needinfo?(lsblakk)
Attachment #828042 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(lsblakk)
Attachment #828041 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #828040 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #828039 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
NI on :lucasr to help with uplift on FF27 here so this can get into our next beta going to build MOnday(1/6).
Flags: needinfo?(lucasr.at.mozilla)
Flags: needinfo?(lucasr.at.mozilla)
You need to log in before you can comment on or make changes to this bug.