Closed Bug 944142 Opened 7 years ago Closed 6 years ago

Implement NavigationHelper.goForward for phones

Categories

(Firefox for Android :: Testing, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: mcomella, Assigned: sebastian)

References

Details

Attachments

(1 file, 5 obsolete files)

No description provided.
Summary: Implement NavigationHelper.goForward → Implement NavigationHelper.goForward for phones
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Okay! So this patch is my best attempt so far. I introduced AppMenuComponent for clicking on items in the app menu. I tried to make it independent from version checks etc. to not clone the functionality of GeckoApp / BrowserApp.

The testSessionHistory test cases passed successfully on all devices / configurations I could get my hands on:
* HTC Sensation XL - Android 4.0.3: Overflow menu triggerd by hardware menu button
* Nexus 5 - Android 4.4.2: Overflow menu button
* HTC Desire HD - Android 2.3.5: Default Android menu
* Nexus 7 (2013) - Android 4.4.2: Forward is not in the app menu but on the toolbar

In AppMenuComponent I only implemented as much as I need to resolve this bug. Therefore the only public method right now is pressForwardMenuItem(). For making pressMenuItem(String) public there might be additional challenges to tackle e.g. if the menu is very long and the menu item to click is somewhere at the end then we might need to scroll the list to inflate and click the menu item.

Oh, and should I split the addition of AppMenuComponent and using it for testSessionHistory to two patches? One for this bug and one for 910802 (As said before it might not be generic enough for every use case yet)?
Attachment #8373171 - Flags: review?(michael.l.comella)
Comment on attachment 8373171 [details] [diff] [review]
944142_appmenucomponent_testSessionHistory.patch

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

You have no idea how excited I am about this: this is a great step in the right direction! :)

I noticed we largerly do not use the Robotium menu item methods which, after reading the source, I think was a good idea - it seems to assume the menu is the only dialog that would be visible, and always uses the menu key (when we want to test the overflow menu button). If you agree, I'd throw a comment into the class javadoc saying why we avoid using those methods.

nit: I like to use `final`... a lot. I won't r- you for it, but if you feel like throwing it in, I'd be happy. :P

nit: I tried to organize the order of the methods in the component files (and messed it up :P) as follows:
  constructor
  assertions
  getViews
  actions (preferably, public to the top, but use your judgement)

I think it helps readability, but let me know if you think otherwise.

::: mobile/android/base/tests/UITest.java
@@ +58,5 @@
>      private String mBaseIpUrl;
>  
>      protected AboutHomeComponent mAboutHome;
>      protected ToolbarComponent mToolbar;
> +    protected AppMenuComponent mAppMenu;

nit: Alphabetize.

@@ +121,5 @@
>  
>      private void initComponents() {
>          mAboutHome = new AboutHomeComponent(this);
>          mToolbar = new ToolbarComponent(this);
> +        mAppMenu = new AppMenuComponent(this);

nit: Alphabetize.

@@ +168,5 @@
>              case TOOLBAR:
>                  return mToolbar;
>  
> +            case APPMENU:
> +                return mAppMenu;

nit: Alphabetize.

::: mobile/android/base/tests/UITestContext.java
@@ +21,5 @@
>  
>      public static enum ComponentType {
>          ABOUTHOME,
> +        TOOLBAR,
> +        APPMENU

nit: Alphabetize.

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +8,5 @@
> +
> +import org.mozilla.gecko.R;
> +import org.mozilla.gecko.menu.MenuItemDefault;
> +import org.mozilla.gecko.menu.MenuItemActionBar;
> +import org.mozilla.gecko.util.HardwareUtils;

nit: Alphabetize. R too.

@@ +13,5 @@
> +import org.mozilla.gecko.tests.helpers.*;
> +import org.mozilla.gecko.tests.UITestContext;
> +
> +import com.jayway.android.robotium.solo.Solo;
> +import com.jayway.android.robotium.solo.RobotiumUtils;

nit: Alphabetize.

@@ +16,5 @@
> +import com.jayway.android.robotium.solo.Solo;
> +import com.jayway.android.robotium.solo.RobotiumUtils;
> +
> +import java.util.List;
> +import android.view.View;

nit: Alphabetize.

@@ +18,5 @@
> +
> +import java.util.List;
> +import android.view.View;
> +
> +public class AppMenuComponent extends BaseComponent {

nit: Javadoc explaining what this component is.

@@ +27,5 @@
> +    public void pressForwardMenuItem() {
> +        pressMenuItem(mSolo.getString(R.string.forward));
> +    }
> +
> +    private void pressMenuItem(String text) {

I would rather this take a `enum MenuItem`, and then we can avoid the numerous press*MenuItem methods. You can store the text associated with a menu item in as an enum member variable.

@@ +31,5 @@
> +    private void pressMenuItem(String text) {
> +        openAppMenu();
> +
> +        View forwardButton = findAppMenuItem(text);
> +        if (forwardButton != null) {

This isn't necessarily the forwardButton - I'd rename the variable.

@@ +34,5 @@
> +        View forwardButton = findAppMenuItem(text);
> +        if (forwardButton != null) {
> +            mSolo.clickOnView(forwardButton);
> +        } else {
> +            mSolo.clickOnMenuItem(text, true);

Does findAppMenuItem ever fail to return a View? Is this necessary?

If not, we should `assertNotNull`.

@@ +38,5 @@
> +            mSolo.clickOnMenuItem(text, true);
> +        }
> +    }
> +
> +    private void openAppMenu() {

You'll want to assert that the menu isn't already open. The best way is probably to ensure that Robotium can't find the View or, if Robotium finds hidden Views, use `View.getVisibility()` to ensure the View is hidden.

@@ +45,5 @@
> +        } else {
> +            pressOverflowMenuButton();
> +        }
> +
> +        mSolo.waitForDialogToOpen();

Wrap this with assertTrue - we want to ensure that this was not a timeout.

@@ +50,5 @@
> +    }
> +
> +    private void pressOverflowMenuButton() {
> +        View overflowMenuButton = getOverflowMenuButtonView();
> +        assertNotNull("The overflow menu button is not null", overflowMenuButton);

I believe the overflow menu can be disabled or hidden at times (e.g. editing mode) - you probably want to assert that the view is both enabled and visible. See ToolbarComponent.pressButton.

@@ +59,5 @@
> +    private View getOverflowMenuButtonView() {
> +        return mSolo.getView(R.id.menu);
> +    }
> +
> +    private View findAppMenuItem(String text) {

You should assert that the menu is open.

You should also comment that this method is dependent on not having two views with equivalent contentDescriptions/text.

nit: findAppMenuItemView.
Attachment #8373171 - Flags: review?(michael.l.comella) → review-
> Does findAppMenuItem ever fail to return a View? Is this necessary?

Yes! For Android 2.x devices that do not use the GeckoMenu at all but the default Android menu.

> You'll want to assert that the menu isn't already open. The best way is
> probably to ensure that Robotium can't find the View or, if Robotium finds
> hidden Views, use `View.getVisibility()` to ensure the View is hidden.

I'm not sure if I can do that. It's easy for GeckoMenu but I don't know if I can check that the Android menu is open (again for 2.2 and 2.3 devices). I'll have a look at the Robotium code - they should have the same problem when clickOnMenuItem(..) is called. The tricky part is that the overflow button always opens the overflow menu but the menu button can trigger the overflow menu OR the old Android menu.


For all the other points mentioned I'll work on an updated patch!
(In reply to :Sebastian Kaspari from comment #3)
> > Does findAppMenuItem ever fail to return a View? Is this necessary?
> 
> Yes! For Android 2.x devices that do not use the GeckoMenu at all but the
> default Android menu.

We should comment the code and name the methods to better showcase the distinction between the two menus (e.g. "findAppMenuItem returns null when the legacy Android menu is in use"). Perhaps this information should be included in the class javadoc too.
> 
> I'm not sure if I can do that. It's easy for GeckoMenu but I don't know if I
> can check that the Android menu is open (again for 2.2 and 2.3 devices).

As per IRC, perhaps we could try `getText(R.strings.settings)`? Just comment the code to show that it's a hack.
Attachment #8373171 - Attachment is obsolete: true
Attachment #8373654 - Flags: review?(michael.l.comella)
Comment on attachment 8373654 [details] [diff] [review]
944142_appmenucomponent_revised.patch

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

Getting closer. :) The biggest remaining issue is the submenu content.

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +29,5 @@
> +        FORWARD(R.string.forward),
> +        NEW_TAB(R.string.new_tab),
> +        RELOAD(R.string.reload),
> +        SETTINGS(R.string.settings),
> +        SHARE(R.string.share);

If you're going to include all these other buttons, test and make sure that they work, especially on the legacy Android menu! If not, leave them out and file a followup.

I'd leave them out until there used in tests because that's the best way to ensure that they'll work (and not confuse other developers who try to use them when they're not working!).

@@ +33,5 @@
> +        SHARE(R.string.share);
> +
> +        protected int resourceId;
> +
> +        MenuItem(int resourceId) {

You can add some `final` to your parameter lists too. ;)

@@ +48,5 @@
> +    }
> +
> +    /**
> +     * Try to find a MenuItemActionBar/MenuItemDefault with the given text set as contentDescription / text.
> +     * 

nit: ws.

@@ +50,5 @@
> +    /**
> +     * Try to find a MenuItemActionBar/MenuItemDefault with the given text set as contentDescription / text.
> +     * 
> +     * Will return null when the Android legacy menu is in use.
> +     * 

nit: ws

@@ +74,5 @@
> +        return null;
> +    }
> +
> +    public void pressMenuItem(MenuItem menuItem) {
> +        openAppMenu();

I just realized we need a solution for submenus since we can't call `openAppMenu` every time.

@@ +76,5 @@
> +
> +    public void pressMenuItem(MenuItem menuItem) {
> +        openAppMenu();
> +
> +        final String text = mSolo.getString(menuItem.resourceId);

Perhaps we should take care of this conversation in the enum constructor.

@@ +85,5 @@
> +            assertEquals("The menu item is visible", View.VISIBLE, menuItemView.getVisibility());
> +
> +            mSolo.clickOnView(menuItemView);
> +        } else {
> +            // We could not find a view representing this menu item: Let's try to

nit: "Let's let Robotium try..."

@@ +95,5 @@
> +    private void openAppMenu() {
> +        // The appearance of the "New tab" menu item is our best guess about whether
> +        // the menu is open or not.
> +        final String newTabLabel = mSolo.getString(R.string.new_tab);
> +        assertFalse("The menu is not open", mSolo.searchText(newTabLabel));

Move this functionality to a private assertion method: `assertMenuIsNotOpen`. Don't forget to `return this;`!

To be more generally, you may want to have a `isMenuOpen()` method that `assertMenuIsNotOpen` calls `assertFalse` on (e.g. see `ToolbarComponent.assertIsEditing`).

@@ +104,5 @@
> +            pressOverflowMenuButton();
> +        }
> +
> +        // Now the "New tab" menu item should appear
> +        assertTrue("Menu is open", mSolo.waitForText(newTabLabel));

I'd abstract this out for readability: `waitForOpenMenu` or `waitForMenuOpen`

::: mobile/android/base/tests/helpers/NavigationHelper.java
@@ +30,5 @@
>          sContext = context;
>          sSolo = context.getSolo();
>  
> +        sAppMenu = (AppMenuComponent) context.getComponent(ComponentType.APPMENU);
> +        sToolbar = (ToolbarComponent) context.getComponent(ComponentType.TOOLBAR);        

nit: ws
Attachment #8373654 - Flags: review?(michael.l.comella) → review-
(In reply to Michael Comella (:mcomella) from comment #6)
> Getting closer. :) The biggest remaining issue is the submenu content.

Then again, if you wanted to file a followup and don't mind potentially rewriting some things (unless you've already given thought to it and this is your product!), we could go with this. One initial idea that works as a followup is that maybe we could just overload the `pressMenuItem` method to take an Array/List of things to click in the menu (perhaps give the enum a "children" list and assert that we always have a correct sequence).
Yeah, I'd like to work on the submenu and the other menu items in bug 910802 and/or bug 944144 (or other follow-up bugs) and limit the patch here to everything needed for the "forward" use case.

(In reply to Michael Comella (:mcomella) from comment #6)
> @@ +76,5 @@
> > +
> > +    public void pressMenuItem(MenuItem menuItem) {
> > +        openAppMenu();
> > +
> > +        final String text = mSolo.getString(menuItem.resourceId);
> 
> Perhaps we should take care of this conversation in the enum constructor.

That's what I wanted to do initially but the constructor of the enum doesn't have any access to mSolo in AppMenuComponent. As the enum is part of the AppMenuComponent class I think it's okay for the surrounding AppMenuComponent class to know about the internals of the enum. What do you think? However I would like to define resourceId as final so that it can't be changed during runtime form inside AppMenuComponent.

Besides that I'm going to upload a new patch today!
(In reply to :Sebastian Kaspari from comment #8)
> That's what I wanted to do initially but the constructor of the enum doesn't
> have any access to mSolo in AppMenuComponent.

So it seems: enums are implicitly static! https://stackoverflow.com/questions/4827326/are-enum-types-declared-in-a-class-implicitly-static

The answer links to the motivations if you're curious (though a quick skim didn't give me any good insight :( ).

> As the enum is part of the
> AppMenuComponent class I think it's okay for the surrounding
> AppMenuComponent class to know about the internals of the enum. What do you
> think? However I would like to define resourceId as final so that it can't
> be changed during runtime form inside AppMenuComponent.

Under the guise that it's final, then yes, this could be a reasonable approach.

However, consider that each time we want the String (which does not change) we call `mSolo.getString(int)` which is not easy to remember nor very intuitive - it'd be better to store this information in the enum directly! Rather, what if we added an accessor method to the enum that does this for us? The method would take the Solo instance (i.e. MenuItem.getString(Solo)). This can wrap `Solo.getString(int)` directly.

Additionally, who knows how performant Solo.getString might be? If we find that it's underperformant, we can always cache these values so mSolo.getString only needs to be called once. Turns out it calls Solo -> Getter -> (Android's)Context -> Resources -> Resources -> AssetManager -> AssetManager -> native code that I don't want to look at. :P I'd say cache it (though since it's test code, it might not matter so much).

> Besides that I'm going to upload a new patch today!

Looking forward to it! ^_^
I agree. I didn't consider performance. I'll add a getter and cache the String value.
Here's the updated patch!

"testSessionHistory" passed successfully on the following devices/configurations:
* HTC Sensation XL (Android 4.0.3): Custom menu + Hardware menu key
* Nexus 5 (Android 4.4.2): Custom menu + Overflow menu button
* HTC Desire HD (Android 2.3.5): Android legacy menu + Hardware menu key
* Nexus 7 (2013) (Android 4.4.2): "Forward" is not in the menu at all but on the toolbar
Attachment #8375074 - Flags: review?(michael.l.comella)
Attachment #8373654 - Attachment is obsolete: true
Comment on attachment 8375074 [details] [diff] [review]
944142_appmenucomponent_forward_rev2.patch

This patch is missing the getter for the MenuItem enum
Attachment #8375074 - Attachment is obsolete: true
Attachment #8375074 - Flags: review?(michael.l.comella)
Attachment #8375081 - Flags: review?(michael.l.comella)
Comment on attachment 8375081 [details] [diff] [review]
944142_appmenucomponent_forward_rev3.patch

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

Almost there: it looks functionally correct! Just a few more nits and I think we're good to land! :)

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +26,5 @@
> +public class AppMenuComponent extends BaseComponent {
> +    public enum MenuItem {
> +        FORWARD(R.string.forward);
> +
> +        private final int resourceId;

nit: resourceId -> resourceID

@@ +33,5 @@
> +        MenuItem(final int resourceId) {
> +            this.resourceId = resourceId;
> +        }
> +
> +        public String getString(Solo solo) {

nit: final. Again, you don't have to be as pedantic as me if you don't want to, but I thought I'd bring it up again. :P Here and below.

@@ +95,5 @@
> +            mSolo.clickOnView(menuItemView);
> +        } else {
> +            // We could not find a view representing this menu item: Let's let Robotium try to
> +            // locate it in the legacy Android menu (devices with Android 2.x)
> +            mSolo.clickOnMenuItem(text, true);

Add a comment mentioning that this also tries to open the menu (via https://github.com/RobotiumTech/robotium/blob/master/robotium-solo/src/main/java/com/robotium/solo/Clicker.java#L284), but it's safer to open it ourselves since they just look for any old dialog.

@@ +114,5 @@
> +
> +    private void pressOverflowMenuButton() {
> +        final View overflowMenuButton = getOverflowMenuButtonView();
> +
> +        assertNotNull("The overflow menu button is not null", overflowMenuButton);

I actually discovered in another bug with margaret that Solo.getView will throw if the view is not discoverable! This check is unnecessary (my bad!).

@@ +124,5 @@
> +
> +    private boolean isMenuOpen() {
> +        // The presence of the "New tab" menu item is our best guess about whether
> +        // the menu is open or not.
> +        final String newTabLabel = mSolo.getString(R.string.new_tab);

Use MenuItem enum.

@@ +129,5 @@
> +        return mSolo.searchText(newTabLabel);
> +    }
> +
> +    private void waitForMenuOpen() {
> +        WaitHelper.waitFor("Menu to open", new Condition() {

nit: Menu -> menu. It prepends "Waiting for " to the text so it'd look a little better that way.

::: mobile/android/base/tests/helpers/NavigationHelper.java
@@ +23,5 @@
>      private static UITestContext sContext;
>      private static Solo sSolo;
>  
>      private static ToolbarComponent sToolbar;
> +    private static AppMenuComponent sAppMenu;

nit: Alphabetize.
Attachment #8375081 - Flags: review?(michael.l.comella) → review-
Here's the updated patch.

Again testSessionHistory passed successfully on the following devices/configurations:
* HTC Sensation XL (Android 4.0.3)
* Nexus 5 (Android 4.4.2)
* HTC Desire HD (Android 2.3.5)
* Nexus 7 (2013) (Android 4.4.2)
Attachment #8375081 - Attachment is obsolete: true
Attachment #8375764 - Flags: review?(michael.l.comella)
Comment on attachment 8375764 [details] [diff] [review]
944142_appmenucomponent_forward_rev4.patch

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

lgtm. Fix the nit, upload, and feel free to land - no additional review necessary.

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +97,5 @@
> +        } else {
> +            // We could not find a view representing this menu item: Let's let Robotium try to
> +            // locate and click it in the legacy Android menu (devices with Android 2.x).
> +            //
> +            // Robotium will also try to open the menu if it doesn't find an open dialog.

nit: Mention that it shoudln't not find an open dialog because we open it for them (with better wording :P).
Attachment #8375764 - Flags: review?(michael.l.comella) → review+
Duplicate of this bug: 910802
https://hg.mozilla.org/integration/fx-team/rev/b969f8b35464
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b969f8b35464
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Blocks: 967139
No longer depends on: 967139
You need to log in before you can comment on or make changes to this bug.