926310 - Robocop: Add test for 'Reader Mode' feature

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul Feher]

This bug tracks the Robocop tests part of 'Reader Mode' feature on Firefox for Android.
https://moztrap.mozilla.org/manage/cases/?filter-suite=373

This patch tests the Reader Mode feature by:
- adding to reading list by long press on the reader icon
- remove item from reading list by long press on the reader icon
- checks the reader toolbar functionality(share, add/remove to reading list, go to reading list)
- accessing a page from reading list menu the page loads in reader mode
- the reader item has the reader associated associated in History tab
- the reading list is properly populated after adding or removing reader items

Comment 1

[Paul Feher]

Attachment: Reader Mode V1

Comment 2

[Paul Feher]

Try server runs look good.
https://tbpl.mozilla.org/?tree=Try&rev=6f788b542ce2

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 816468 [details] [diff] [review]
Reader Mode V1

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

a lot of questions and a few nits/style issues.

::: mobile/android/base/tests/AboutHomeTest.java.in
@@ +288,3 @@
>              if (AboutHomeTabs.MOST_RECENT == tab || AboutHomeTabs.TABS_FROM_LAST_TIME == tab) {
> +                tabOffset = aboutHomeTabs.indexOf(AboutHomeTabs.HISTORY.toString()) - currentTabIndex;
> +                swipeAboutHome(tabOffset);

why are we adding a swipeAboutHome() here?

::: mobile/android/base/tests/testReaderMode.java.in
@@ +13,5 @@
> +/**
> + * This patch tests the Reader Mode feature by adding and removing items in reading list
> + * checks the reader toolbar functionality(share, add/remove to reading list, go to reading list)
> + * accessing a page from reading list menu, checks that the reader icon is associated in History tab
> + * and that the reading list is properly populated after adding or removing reader items 

nit: trailing whitespace

@@ +15,5 @@
> + * checks the reader toolbar functionality(share, add/remove to reading list, go to reading list)
> + * accessing a page from reading list menu, checks that the reader icon is associated in History tab
> + * and that the reading list is properly populated after adding or removing reader items 
> + */
> +public class testReaderMode extends AboutHomeTest {

why does this extend abouthometest?

@@ +30,5 @@
> +        Actions.EventExpecter contentReaderAddedExpecter;
> +        Actions.EventExpecter faviconExpecter;
> +        ListView list;
> +        View child;
> +        String url_1 = getAbsoluteUrl(StringHelper.ROBOCOP_TEXT_PAGE_URL);

can this be named 'textUrl' instead?

@@ +31,5 @@
> +        Actions.EventExpecter faviconExpecter;
> +        ListView list;
> +        View child;
> +        String url_1 = getAbsoluteUrl(StringHelper.ROBOCOP_TEXT_PAGE_URL);
> +        String url_2 = getAbsoluteUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL);

likewise, name this blankUrl

@@ +33,5 @@
> +        View child;
> +        String url_1 = getAbsoluteUrl(StringHelper.ROBOCOP_TEXT_PAGE_URL);
> +        String url_2 = getAbsoluteUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL);
> +        String devType;
> +        int chNr;

chNr is not very descriptive

@@ +58,5 @@
> +        contentReaderAddedExpecter.unregisterListener();
> +
> +        faviconExpecter = mActions.expectGeckoEvent("Reader:FaviconRequest"); // Waiting for the favicon since is the last element loaded usually
> +        mSolo.clickOnView(getReaderIcon());
> +        mSolo.setActivityOrientation(Solo.PORTRAIT); // Changing devices orientation to be sure that all devices are in portrait when will access the reader toolbar

is portrait mode required for reader mode?  I would prefer to have the comment above the line of code so the total line length is reduced.

@@ +67,5 @@
> +        // Open the share menu for the reader toolbar
> +        height = mDriver.getGeckoTop() + mDriver.getGeckoHeight() - 10;
> +        width = mDriver.getGeckoLeft() + mDriver.getGeckoWidth() - 10;
> +        mAsserter.dumpLog("Long Clicking at width = " + String.valueOf(width) + " and height = " + String.valueOf(height));
> +        mSolo.clickOnScreen(width,height);

will this always be the case?  Can we guarantee that we won't have a sidebar or bottom bar?  I would love to see a getContentAreaPixel() so we could get x,y coordinates reliably and only have 1 place to change code if we redesign the UI.  this could be in a separate bug.

@@ +69,5 @@
> +        width = mDriver.getGeckoLeft() + mDriver.getGeckoWidth() - 10;
> +        mAsserter.dumpLog("Long Clicking at width = " + String.valueOf(width) + " and height = " + String.valueOf(height));
> +        mSolo.clickOnScreen(width,height);
> +        mAsserter.ok(mSolo.waitForText("Share via"), "Waiting for the share menu", "The share menu is present");
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK); // Close the share menu

why verify share menu is present?

@@ +76,5 @@
> +        height = mDriver.getGeckoTop() + mDriver.getGeckoHeight() - 10;
> +        width = mDriver.getGeckoLeft() + 50;
> +        mAsserter.dumpLog("Long Clicking at width = " + String.valueOf(width) + " and height = " + String.valueOf(height));
> +        mSolo.clickOnScreen(width,height);
> +        mAsserter.ok(mSolo.waitForText("Page removed from your Reading List"), "Waiting for the page to removed from your Reading List", "The page is removed from your Reading List");

I don't understand how click at this exact pixel location removes it from the reading list, a more descriptive getPixel<...>() function would help :)

@@ +80,5 @@
> +        mAsserter.ok(mSolo.waitForText("Page removed from your Reading List"), "Waiting for the page to removed from your Reading List", "The page is removed from your Reading List");
> +
> +        //Add page to the Reading List using reader toolbar
> +        mSolo.clickOnScreen(width,height);
> +        mAsserter.ok(mSolo.waitForText("Page added to your Reading List"), "Waiting for the page to be added to your Reading List", "The page was added to your Reading List");

so this clicks the same spot, I assume it was in the reading list prior and now we removed it and added it again?

@@ +103,5 @@
> +        mSolo.clickLongOnView(child);
> +        mAsserter.ok(mSolo.waitForText("Open in Reader"), "Verify if the page is present in history as a Reading List item", "The page is present in history as a Reading List item");
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK); // Dismiss the context menu
> +        mSolo.waitForText("Robocop Text Page");
> +        

nit: trailing whitespace

@@ +135,5 @@
> +        openAboutHomeTab(AboutHomeTabs.READING_LIST);
> +        list = findListViewWithTag("reading_list");
> +        child = list.getChildAt(chNr-1);
> +        mAsserter.ok(child == null, "Verify if the Reading List is empty", "The Reading List is empty");
> +    } 

nit: trailing whitespace

@@ +154,5 @@
> +            JSONObject data = new JSONObject(eventData);
> +                if (data.getInt("result") == 0) {
> +                    mAsserter.ok(true, "Waiting for the page to be added to your Reading List", "The page was added to your Reading List");
> +                }
> +                else { 

nit: trailing whitespace

Comment 4

[Paul Feher]

(In reply to Joel Maher (:jmaher) from comment #3)
> Comment on attachment 816468 [details] [diff] [review]
> Reader Mode V1
> 
> Review of attachment 816468 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> a lot of questions and a few nits/style issues.
> 
> ::: mobile/android/base/tests/AboutHomeTest.java.in
> @@ +288,3 @@
> >              if (AboutHomeTabs.MOST_RECENT == tab || AboutHomeTabs.TABS_FROM_LAST_TIME == tab) {
> > +                tabOffset = aboutHomeTabs.indexOf(AboutHomeTabs.HISTORY.toString()) - currentTabIndex;
> > +                swipeAboutHome(tabOffset);
> 
> why are we adding a swipeAboutHome() here?
Here i'm using this method of selecting AboutHome sections because the initial approach that 
implies just clicking the sections name made the application to crash, when transitions 
from or to the reading list tab where made. So I was forced to use the same approach 
of selecting sections that was used for phones.
> 
> ::: mobile/android/base/tests/testReaderMode.java.in
> @@ +13,5 @@
> > +/**
> > + * This patch tests the Reader Mode feature by adding and removing items in reading list
> > + * checks the reader toolbar functionality(share, add/remove to reading list, go to reading list)
> > + * accessing a page from reading list menu, checks that the reader icon is associated in History tab
> > + * and that the reading list is properly populated after adding or removing reader items 
> 
> nit: trailing whitespace
> 
> @@ +15,5 @@
> > + * checks the reader toolbar functionality(share, add/remove to reading list, go to reading list)
> > + * accessing a page from reading list menu, checks that the reader icon is associated in History tab
> > + * and that the reading list is properly populated after adding or removing reader items 
> > + */
> > +public class testReaderMode extends AboutHomeTest {
> 
> why does this extend abouthometest?
This extends AboutHomeTest because I need the methods defined there for the interaction with the AboutHome menu.
> 
> @@ +30,5 @@
> > +        Actions.EventExpecter contentReaderAddedExpecter;
> > +        Actions.EventExpecter faviconExpecter;
> > +        ListView list;
> > +        View child;
> > +        String url_1 = getAbsoluteUrl(StringHelper.ROBOCOP_TEXT_PAGE_URL);
> 
> can this be named 'textUrl' instead?
> 
> @@ +31,5 @@
> > +        Actions.EventExpecter faviconExpecter;
> > +        ListView list;
> > +        View child;
> > +        String url_1 = getAbsoluteUrl(StringHelper.ROBOCOP_TEXT_PAGE_URL);
> > +        String url_2 = getAbsoluteUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL);
> 
> likewise, name this blankUrl
> 
> @@ +33,5 @@
> > +        View child;
> > +        String url_1 = getAbsoluteUrl(StringHelper.ROBOCOP_TEXT_PAGE_URL);
> > +        String url_2 = getAbsoluteUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL);
> > +        String devType;
> > +        int chNr;
> 
> chNr is not very descriptive
> 
> @@ +58,5 @@
> > +        contentReaderAddedExpecter.unregisterListener();
> > +
> > +        faviconExpecter = mActions.expectGeckoEvent("Reader:FaviconRequest"); // Waiting for the favicon since is the last element loaded usually
> > +        mSolo.clickOnView(getReaderIcon());
> > +        mSolo.setActivityOrientation(Solo.PORTRAIT); // Changing devices orientation to be sure that all devices are in portrait when will access the reader toolbar
> 
> is portrait mode required for reader mode?  I would prefer to have the
> comment above the line of code so the total line length is reduced.
> 
> @@ +67,5 @@
> > +        // Open the share menu for the reader toolbar
> > +        height = mDriver.getGeckoTop() + mDriver.getGeckoHeight() - 10;
> > +        width = mDriver.getGeckoLeft() + mDriver.getGeckoWidth() - 10;
> > +        mAsserter.dumpLog("Long Clicking at width = " + String.valueOf(width) + " and height = " + String.valueOf(height));
> > +        mSolo.clickOnScreen(width,height);
> 
> will this always be the case?  Can we guarantee that we won't have a sidebar
> or bottom bar?  I would love to see a getContentAreaPixel() so we could get
> x,y coordinates reliably and only have 1 place to change code if we redesign
> the UI.  this could be in a separate bug.
> 
> @@ +76,5 @@
> > +        height = mDriver.getGeckoTop() + mDriver.getGeckoHeight() - 10;
> > +        width = mDriver.getGeckoLeft() + 50;
> > +        mAsserter.dumpLog("Long Clicking at width = " + String.valueOf(width) + " and height = " + String.valueOf(height));
> > +        mSolo.clickOnScreen(width,height);
> > +        mAsserter.ok(mSolo.waitForText("Page removed from your Reading List"), "Waiting for the page to removed from your Reading List", "The page is removed from your Reading List");
> 
> I don't understand how click at this exact pixel location removes it from
> the reading list, a more descriptive getPixel<...>() function would help :)
The reader mode toolbar doesn't make part of android UI/gecko views (is defined as css elements)
, so i was forced to find an alternative way of accessing it and i used the exact pixel location 
to click and access the tooolbar options. Based on these constraints I've found a viable solution
for accessing it in the portrait view because the toolbar is evenly split in four sections at 
the bottom of the screen. The logic is, and it can be applied to all displays regardless of width
if they are in portrait view, after the toolbar is displayed to select the bottom right 
side of the screen for the 'share' option, the bottom left side of the screen for 
'add/remove to reading list' option also you can access the other 2 remaining options by clicking on 
bottom screen half width - 10px and bottom screen half width + 10px. In conclusion this method of 
interacting with the reader mode toolbar may not be very elegant but it can be applied 
with success for every type of devices(phones or tablets) regardless of screen width. 
This is not applicable for landscape view because the toolbar icons are all grouped in the right 
side of the screen and they can't be accessed at the same coordinates on every device.
> 
> @@ +69,5 @@
> > +        width = mDriver.getGeckoLeft() + mDriver.getGeckoWidth() - 10;
> > +        mAsserter.dumpLog("Long Clicking at width = " + String.valueOf(width) + " and height = " + String.valueOf(height));
> > +        mSolo.clickOnScreen(width,height);
> > +        mAsserter.ok(mSolo.waitForText("Share via"), "Waiting for the share menu", "The share menu is present");
> > +        mActions.sendSpecialKey(Actions.SpecialKey.BACK); // Close the share menu
> 
> why verify share menu is present?
Because is a reader mode toolbar option and i check to see if this option is available
> 
> @@ +80,5 @@
> > +        mAsserter.ok(mSolo.waitForText("Page removed from your Reading List"), "Waiting for the page to removed from your Reading List", "The page is removed from your Reading List");
> > +
> > +        //Add page to the Reading List using reader toolbar
> > +        mSolo.clickOnScreen(width,height);
> > +        mAsserter.ok(mSolo.waitForText("Page added to your Reading List"), "Waiting for the page to be added to your Reading List", "The page was added to your Reading List");
> 
> so this clicks the same spot, I assume it was in the reading list prior and
> now we removed it and added it again?
Yes because the that is a adding/removing button.
> 
> @@ +103,5 @@
> > +        mSolo.clickLongOnView(child);
> > +        mAsserter.ok(mSolo.waitForText("Open in Reader"), "Verify if the page is present in history as a Reading List item", "The page is present in history as a Reading List item");
> > +        mActions.sendSpecialKey(Actions.SpecialKey.BACK); // Dismiss the context menu
> > +        mSolo.waitForText("Robocop Text Page");
> > +        
> 
> nit: trailing whitespace
> 
> @@ +135,5 @@
> > +        openAboutHomeTab(AboutHomeTabs.READING_LIST);
> > +        list = findListViewWithTag("reading_list");
> > +        child = list.getChildAt(chNr-1);
> > +        mAsserter.ok(child == null, "Verify if the Reading List is empty", "The Reading List is empty");
> > +    } 
> 
> nit: trailing whitespace
> 
> @@ +154,5 @@
> > +            JSONObject data = new JSONObject(eventData);
> > +                if (data.getInt("result") == 0) {
> > +                    mAsserter.ok(true, "Waiting for the page to be added to your Reading List", "The page was added to your Reading List");
> > +                }
> > +                else { 
> 
> nit: trailing whitespace

Comment 5

[Paul Feher]

Attachment: Reader Mode V1.1

Made the required changes.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 816563 [details] [diff] [review]
Reader Mode V1.1

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

thanks for explaining some of the details here and fixing the nits.

::: mobile/android/base/tests/testReaderMode.java.in
@@ +33,5 @@
> +        View child;
> +        String textUrl = getAbsoluteUrl(StringHelper.ROBOCOP_TEXT_PAGE_URL);
> +        String devType;
> +        int childNo;
> +        devType = mDevice.type;

can you just do:
String devType = mDevice.type;

@@ +96,5 @@
> +
> +        // Check if the page is present in the Reading List
> +        mAsserter.ok(mSolo.waitForText("Robocop Text Page"), "Verify if the page is added to your Reading List", "The page is present in your Reading List");
> +
> +       // Check if the page is added in History tab like a Reading List item

nit, this is one space away from aligning with the code below it.

Comment 7

[Paul Feher]

Attachment: Reader Mode V2

Done

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/8ac8f0ebecdd

Can you please update your Mercurial config to include your full name in addition to your email address? Thanks :)

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ac8f0ebecdd