Hide home banner when in editing mode

RESOLVED FIXED in Firefox 29

Status

()

Firefox for Android
Awesomescreen
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Margaret, Assigned: jdover)

Tracking

Trunk
Firefox 30
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed, fennec29+)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
The home banner eats up valuable real estate, we shouldn't show it when the keyboard is open.
tracking-fennec: ? → +
(Reporter)

Updated

5 years ago
Assignee: nobody → margaret.leibovic
(Reporter)

Comment 1

5 years ago
Whatever we do in this bug, we'll probably use a similar solution for bug 907857. While trying to come up with a solution here, I tried listening for config changes, but that wasn't working well. Maybe I'll try experimenting with urlbar focus like Lucas suggested in bug 907857 comment 2.

However, I realized these bugs aren't a major problem, since as soon as the user starts typing we show the BrowserSearch list, which overlays the about:home content anyway.
(Assignee)

Comment 2

4 years ago
Created attachment 8366997 [details] [diff] [review]
Hide banner when URL bar is focused

I'm looking for some other ideas on this one, as this does work, I feel like the BrowserToolbar really shouldn't know about HomeBanner, however I can't think of a particularly good way to do this because:

1) As far as I could find, there is no reliable way to detect that the keyboard is present between Android versions and devices.
2) You cannot register more than one onFocusChangeListener to a view.

I think this would be a case where having an event bus (eg. Otto) would be a good way to decouple this functionality.
Attachment #8366997 - Flags: feedback?(margaret.leibovic)
(Reporter)

Comment 3

4 years ago
Comment on attachment 8366997 [details] [diff] [review]
Hide banner when URL bar is focused

I'm going to re-route this to lucasr, as he has more experience with the toolbar :)

I agree it feels pretty dirty to be poking at the home banner from inside BrowserToolbar... maybe there's a notification we can fire to let consumers know when we enter/exit editing mode.
Attachment #8366997 - Flags: feedback?(margaret.leibovic) → feedback?(lucasr.at.mozilla)
(Assignee)

Updated

4 years ago
Assignee: margaret.leibovic → jdover
Comment on attachment 8366997 [details] [diff] [review]
Hide banner when URL bar is focused

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

Yeah, this is a bit too hand-wavy :-) Here are some possible solutions:
- Track window resizes (which are only really triggered by the activity anyway) and hide banner accordingly.
- Add a OnFocusListener to BrowserToolbar and let BrowserApp tell HomePager that it needs to hide the banner.

Experiment with them and see which one looks cleaner?
Attachment #8366997 - Flags: feedback?(lucasr.at.mozilla) → feedback-
(Assignee)

Comment 5

4 years ago
Created attachment 8368053 [details] [diff] [review]
Hide banner when URL bar is focused

Much better this time :)
Attachment #8366997 - Attachment is obsolete: true
Attachment #8368053 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Comment on attachment 8368053 [details] [diff] [review]
Hide banner when URL bar is focused

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

Nice. Here's what I suggest:
1. Add an onToolbarFocusChange(boolean hasFocus) method to HomePager
2. In the OnFocusChangeListener, call onToolbarFocusChange() You must check isHomePagerVisible() before doing so btw.
3. You already have a mHomeBannger member in HomePager (added in bug 960359), call hideBanner()/showBanner() in HomePager.onToolbarFocusChange().

::: mobile/android/base/GeckoApp.java
@@ +198,5 @@
>      protected DoorHangerPopup mDoorHangerPopup;
>      protected FormAssistPopup mFormAssistPopup;
>      protected TabsPanel mTabsPanel;
>      protected ButtonToast mToast;
> +    protected HomeBanner mHomeBanner;

Neither GeckoApp nor BrowserApp should know anything about HomeBanner. This should be encapsulated behind an API in HomePager.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +88,5 @@
>      public interface OnActivateListener {
>          public void onActivate();
>      }
>  
> +    public interface onDeactivateListener {

Unrelated? Or did you mean OnFocusChangeListener here?

@@ +319,5 @@
>          mUrlEditLayout.setOnFocusChangeListener(new View.OnFocusChangeListener() {
>              @Override
>              public void onFocusChange(View v, boolean hasFocus) {
>                  setSelected(hasFocus);
> +                mFocusChangeListener.onFocusChange(v, hasFocus);

You should check if mFocusChangeListener is not null before calling it. I'd skip the view argument from the focus as this is pretty much breaking the encapsulated views in BrowserToolbar i.e. users of BrowserToolbar's focus listener should need to know which specific view got focus.
Attachment #8368053 - Flags: review?(lucasr.at.mozilla) → feedback+
Ah, I think the HomeBanner methods should actually be hide()/show() instead of the redundant hideBanner()/showBanner() btw.
(Assignee)

Comment 8

4 years ago
Created attachment 8368182 [details] [diff] [review]
patch

Moved logic into HomePager as well as renamed showBanner/hideBanner -> show/hide
Attachment #8368053 - Attachment is obsolete: true
Attachment #8368182 - Flags: review?(lucasr.at.mozilla)

Updated

4 years ago
Attachment #8368182 - Flags: review?(lucasr.at.mozilla) → review+
(Reporter)

Comment 9

4 years ago
Comment on attachment 8368182 [details] [diff] [review]
patch

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

::: mobile/android/base/home/HomePager.java
@@ +318,5 @@
> +    public void onToolbarFocusChange(boolean hasFocus) {
> +        if (hasFocus) {
> +            mHomeBanner.hide();
> +        } else {
> +            mHomeBanner.show();

Will this make the banner show up on non-default panels?
(In reply to :Margaret Leibovic from comment #9)
> Comment on attachment 8368182 [details] [diff] [review]
> patch
> 
> Review of attachment 8368182 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomePager.java
> @@ +318,5 @@
> > +    public void onToolbarFocusChange(boolean hasFocus) {
> > +        if (hasFocus) {
> > +            mHomeBanner.hide();
> > +        } else {
> > +            mHomeBanner.show();
> 
> Will this make the banner show up on non-default panels?

Is that part of the design? I wasn't aware of this. What about the case where the user has hidden all panels?
(Reporter)

Comment 11

4 years ago
(In reply to Lucas Rocha (:lucasr) from comment #10)
> (In reply to :Margaret Leibovic from comment #9)
> > Comment on attachment 8368182 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8368182 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/home/HomePager.java
> > @@ +318,5 @@
> > > +    public void onToolbarFocusChange(boolean hasFocus) {
> > > +        if (hasFocus) {
> > > +            mHomeBanner.hide();
> > > +        } else {
> > > +            mHomeBanner.show();
> > 
> > Will this make the banner show up on non-default panels?
> 
> Is that part of the design? I wasn't aware of this. What about the case
> where the user has hidden all panels?

The banner should only show up on the default panel (that's the logic jdover recently added in bug 960359). In that bug I suggested we file a follow-up to make sure we just never show the banner if no panels are enabled (I think it would be weird to show the banner if the rest of the screen is empty).
(Assignee)

Comment 12

4 years ago
Created attachment 8368243 [details] [diff] [review]
patch

(In reply to Lucas Rocha (:lucasr) from comment #10)
> (In reply to :Margaret Leibovic from comment #9)
> > ::: mobile/android/base/home/HomePager.java
> > @@ +318,5 @@
> > > +    public void onToolbarFocusChange(boolean hasFocus) {
> > > +        if (hasFocus) {
> > > +            mHomeBanner.hide();
> > > +        } else {
> > > +            mHomeBanner.show();
> > 
> > Will this make the banner show up on non-default panels?
> 
> Is that part of the design? I wasn't aware of this. What about the case
> where the user has hidden all panels?

Good catches, updated patch to handle these cases, verified to work!
Attachment #8368182 - Attachment is obsolete: true
Attachment #8368243 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8368243 [details] [diff] [review]
patch

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

Awesome.
Attachment #8368243 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 14

4 years ago
To clarify, the way this works is for the banner to show up even if there are no panels. Will file a bug for showing no banner when there are no panels. May need to have a discussion about whether there will be any important snippets that the user could miss?
(Assignee)

Comment 15

4 years ago
See bug 966047 for hiding banner when there are no panels
Keywords: checkin-needed
(In reply to Josh Dover from comment #14)
> To clarify, the way this works is for the banner to show up even if there
> are no panels. Will file a bug for showing no banner when there are no
> panels. May need to have a discussion about whether there will be any
> important snippets that the user could miss?

Yeah, I think we should show the banner even when all panels are disabled. In any case, let's use bug 966047 to discuss that.
https://hg.mozilla.org/integration/fx-team/rev/a4b73c79ac44
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

4 years ago
Blocks: 966307
https://hg.mozilla.org/mozilla-central/rev/a4b73c79ac44
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
(Reporter)

Updated

4 years ago
No longer blocks: 966307
Depends on: 960359
(Reporter)

Comment 19

4 years ago
I had to back this out because of its dependency on bug 960359:
https://hg.mozilla.org/integration/fx-team/rev/b1f8c61bb310
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 29 → ---
(Reporter)

Comment 21

4 years ago
Updating the summary to make this bug more general.
Summary: Hide home banner when keyboard is open → Hide home banner when in editing mode
This is not an accurate summary btw. The UI can be in editing mode with the keyboard hidden, in which case we do want to show the banner, no?
(Reporter)

Comment 23

4 years ago
(In reply to Lucas Rocha (:lucasr) from comment #22)
> This is not an accurate summary btw. The UI can be in editing mode with the
> keyboard hidden, in which case we do want to show the banner, no?

Do we? I thought this would be a good way to err on the side of not showing the banner too often. When the user is in editing mode, I assume they want to be going somewhere, not getting distracted by the banner.
(In reply to :Margaret Leibovic from comment #23)
> (In reply to Lucas Rocha (:lucasr) from comment #22)
> > This is not an accurate summary btw. The UI can be in editing mode with the
> > keyboard hidden, in which case we do want to show the banner, no?
> 
> Do we? I thought this would be a good way to err on the side of not showing
> the banner too often. When the user is in editing mode, I assume they want
> to be going somewhere, not getting distracted by the banner.

Ah, now I understand the intent here. Then the summary should be something along the lines of "Only show home banner when accessing about:home directly". But that would also include when you create a new tab. I assume we only want to show snippets on startup? How does this behaviour interact with add-on snippets?
(Reporter)

Comment 25

4 years ago
(In reply to Lucas Rocha (:lucasr) from comment #24)
> (In reply to :Margaret Leibovic from comment #23)
> > (In reply to Lucas Rocha (:lucasr) from comment #22)
> > > This is not an accurate summary btw. The UI can be in editing mode with the
> > > keyboard hidden, in which case we do want to show the banner, no?
> > 
> > Do we? I thought this would be a good way to err on the side of not showing
> > the banner too often. When the user is in editing mode, I assume they want
> > to be going somewhere, not getting distracted by the banner.
> 
> Ah, now I understand the intent here. Then the summary should be something
> along the lines of "Only show home banner when accessing about:home
> directly". But that would also include when you create a new tab. I assume
> we only want to show snippets on startup?

I think showing the banner on a new tab is fine, since that's similar to the startup experience. Basically, to generalize, I think we should show the banner when the user is actually on "about:home", but not when they get to this interface from tapping on the toolbar.

> How does this behaviour interact with add-on snippets?

I imagine any fix we make here will apply to the home banner, which does not know whether a banner message is coming from the snippets server or an add-on. I don't think we should worry about differentiating the two, at least for now, since I haven't actually seen any add-ons using this banner API (besides the add-ons I've made to test it).
(Reporter)

Updated

4 years ago
No longer depends on: 960359
(Assignee)

Comment 26

4 years ago
Created attachment 8379887 [details] [diff] [review]
patch

Updated to reflect new changes in 960359 and added a test.
Attachment #8368243 - Attachment is obsolete: true
Attachment #8379887 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 27

4 years ago
Comment on attachment 8379887 [details] [diff] [review]
patch

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

Looking good, r+ with comments addressed.

::: mobile/android/base/BrowserApp.java
@@ +510,5 @@
>  
> +        mBrowserToolbar.setOnFocusChangeListener(new View.OnFocusChangeListener() {
> +            @Override
> +            public void onFocusChange(View v, boolean hasFocus) {
> +                mHomePager.onToolbarFocusChange(hasFocus);

The original patch checked isHomePagerVisible in here. We probably still want that, right?

::: mobile/android/base/home/HomePager.java
@@ +285,5 @@
>          return super.dispatchTouchEvent(event);
>      }
>  
> +    public void onToolbarFocusChange(boolean hasFocus) {
> +        boolean enabled = !hasFocus && getCurrentItem() == mDefaultPageIndex;

Nit: final. Also, add a comment in here explaining the goal of this logic.

::: mobile/android/base/tests/testHomeBanner.java
@@ +99,5 @@
>          mAboutHome.assertBannerNotVisible();
>      }
>  
> +    private void hideOnToolbarFocusTest() {
> +        addBannerMessage();

Nothing is removing the banner that was added in addBannerTest, so we don't need to add another one. However, if we re-enable removeBannerTest, we will need to do something to add a banner back, so perhaps just put a comment about that in here.

These small testcase methods make it easier to read the code, but unfortunately, the app does maintain state from one to the next.

@@ +108,5 @@
> +        mToolbar.enterEditingMode();
> +        mAboutHome.assertBannerNotVisible();
> +
> +        mToolbar.dismissEditingMode();
> +        mAboutHome.assertBannerVisible();

Sweet, this is a nice little test.
Attachment #8379887 - Flags: review?(margaret.leibovic) → review+
(Reporter)

Comment 28

4 years ago
Also please make a try run!
(Assignee)

Comment 29

4 years ago
Created attachment 8380013 [details] [diff] [review]
patch

Fixed! Running on try: https://tbpl.mozilla.org/?tree=Try&rev=c46874207fa9
Attachment #8379887 - Attachment is obsolete: true
Attachment #8380013 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/ee23500859a4
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ee23500859a4
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Depends on: 981614
(Reporter)

Comment 32

4 years ago
This was set to tracking+ long ago, but we should really try to get this on 29, since that's when we're going to start showing users banners.
tracking-fennec: + → ?
(Reporter)

Comment 33

4 years ago
Comment on attachment 8380013 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): enabling snippets and sync promo banner
User impact if declined: banner appears while keyboard is open
Testing completed (on m-c, etc.): landed on m-c 2/25
Risk to taking this patch (and alternatives if risky): This caused a regression (bug 981614), so we'll need to uplift that as well. Other than that, no regressions found.
String or IDL/UUID changes made by this patch: none
Attachment #8380013 - Flags: approval-mozilla-aurora?
tracking-fennec: ? → 29+
status-firefox29: --- → affected
status-firefox30: --- → fixed
Attachment #8380013 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/81fa76ec7c16

I made sure to take bug 977155 into account since that was already uplifted to Aurora prior to this.
status-firefox29: affected → fixed
You need to log in before you can comment on or make changes to this bug.