1134501 - Add API's to move page into Reader View, and force show Reader View icon

Status: RESOLVED FIXED

(Firefox :: Tours, defect, P1)

Description

[Justin Dolske [:Dolske]]

UITour should have a function to allow a tour page to enable Reader View on itself. This allows having a snippet, shared link, or general product page that can start a simple in-product tour, with info panels pointing out salient features of Reader Mode and Reading List.

Two additional details:

* This function should ensure that the Reading List sidebar is enabled upon entering Reader View -- it is normally shown anyway upon entering Reader View for the first time, but we want to make sure it's _always_ shown for tour pages, since they'll be pointing out features in the sidebar.

* Upon entering Reader View, the first in-product tour infopanel (from bug 1134497) should be shown.

Comment 1

[Marco Mucci [:MarcoM]]

Required for Campaign Release

Comment 2

[Justin Dolske [:Dolske]]

Additional update from today's meeting: we'd like an argument to allow triggering the infopanels (from bug 1134497), even if they've previously seen them. That way opening this page, which is essentially a UITour page, can show these useful panels.

Comment 3

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Per a discussion with :MattN today, it was decided it would be more efficient for the product to force the page into Reader View rather than create API's to allow UITour to execute.

In bug1136570comment8 it was confirmed that the FTU URL will be stored in a pref.

A possible approach is that if the current URL = the FTU URL in pref, launch Reader View.

NI :Unfocused to confirm that the plan is still to store the URL in a pref (note: FTU URL settled in bug 1134286)
NI :dolske to review and bring up any concerns with this approach.

Comment 4

[Matthew N. [:MattN]]

(In reply to Cory Price [:ckprice] from comment #3)
> Per a discussion with :MattN today, it was decided it would be more
> efficient for the product to force the page into Reader View rather than
> create API's to allow UITour to execute.

The reason is that the pre-loaded reading list entry will load in reader view so it won't be able to run JS to trigger reader view via JS.

> A possible approach is that if the current URL = the FTU URL in pref, launch
> Reader View.

Note that it's not as simple as doing an exact match as the URL will redirect to include a locale and we need to handle all such locales.

Comment 5

[Justin Dolske [:Dolske]]

Comment 3/4 make sense to me.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Matthew N. [:MattN] from comment #4)
> > A possible approach is that if the current URL = the FTU URL in pref, launch
> > Reader View.
> 
> Note that it's not as simple as doing an exact match as the URL will
> redirect to include a locale and we need to handle all such locales.

Yea, having a similar issue with server-side redirects in bug 1136570.

For this bug at least, we can have a simpler fix without adding %LOCALE% to the URL, by also including a pattern matching string. I think we probably want that in bug 1136570 too, but that's harder to do for various reasons - here it (should be) much simpler.


(In reply to Cory Price [:ckprice] from comment #3)
> NI :Unfocused to confirm that the plan is still to store the URL in a pref

Yes... but for the above reasons, that ends up not being relevant here.

I think what we'll end up doing here is having a pref that stores a list of patterns. If we load a URL that matches a pattern in that list, we auto-open Reader View.

Comment 7

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

:MattN, :Unfocused

Will this override the natural Reader View detection?

We started development of the page based on the approved design/copy (bug 1132954 comment 9). Some preliminary tests show that the page as it isn't triggering Reader View. 

1. When this bug lands, will it automatically triggering Reader View when someone visits regardless of the content?
2. Is it possible to have this page 'white listed' so that the Reader View icon is always available for it?

Comment 8

[Matthew N. [:MattN]]

I'm not really surprised that that page doesn't triggered reader view since it isn't really like an article.

Margaret/Gijs, is there currently a way to force a page into Reader View? Otherwise, is there some markup that will cause the page to be considered appropriate for readability? Can the page hide some article text to trigger reader view?

(In reply to Cory Price [:ckprice] from comment #7)
> 2. Is it possible to have this page 'white listed' so that the Reader View
> icon is always available for it?

I think if we have #1 then we will get #2 for free.

Comment 9

[:Margaret Leibovic]

(In reply to Matthew N. [:MattN] from comment #8)
> I'm not really surprised that that page doesn't triggered reader view since
> it isn't really like an article.
> 
> Margaret/Gijs, is there currently a way to force a page into Reader View?
> Otherwise, is there some markup that will cause the page to be considered
> appropriate for readability? Can the page hide some article text to trigger
> reader view?

Right now we use this `isProbablyReaderable` function to decide whether or not to show the reader view button:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm?force=1#70

But that doesn't actually determine whether or not Readability would succeed or fail. You can attempt to load a page in reader view directly by loading about:reader?url=<foo>. Or if you have the power, you could try calling ReaderParent.toggleReaderMode directly:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/ReaderParent.jsm#162

> (In reply to Cory Price [:ckprice] from comment #7)
> > 2. Is it possible to have this page 'white listed' so that the Reader View
> > icon is always available for it?
> 
> I think if we have #1 then we will get #2 for free.

Yes, we could add logic to whitelist this in `isProbablyReaderable`.

Comment 10

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

:Unfocused - will the FTU page have to pass isProbablyReadable for the functionality described in this bug (forcing into Reader View) to kick in?

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v1

WiP. Needing some time off, so dropping this and attaching WiP for someone else to pick up.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8587077 [details] [diff] [review]
WiP v1

*cough*

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Cory Price [:ckprice] from comment #10)
> :Unfocused - will the FTU page have to pass isProbablyReadable for the
> functionality described in this bug (forcing into Reader View) to kick in?

I'll leave this up to whoever ends up picking this up while I'm away (if that happens before I'm back). But my initial thought is that the page won't need to do anything - this will all be handled in Firefox itself. Page just needs to ensure it looks *good* in ReaderView, see comment 9 to force it to test.

Comment 14

[:Margaret Leibovic]

I'm not sure I have enough context here about how the FTU experience works, but if we're loading a webpage in a tab, the isProbablyReaderable code will automatically kick in, as Blair suggested, as we automatically show the reader view button for any page that we detect as "probably readerable" on page load.

Comment 15

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to :Margaret Leibovic from comment #14)
> I'm not sure I have enough context here about how the FTU experience works,
> but if we're loading a webpage in a tab, the isProbablyReaderable code will
> automatically kick in, as Blair suggested, as we automatically show the
> reader view button for any page that we detect as "probably readerable" on
> page load.
See attachment 8574210 [details] for a wireframe of the tour.

Basically, this page will be a background for the tour door hangers. 

1. It's not technically an 'article'
2. Tested and looks okay in Reader View
3. It probably will not meet the criteria needed for isProbablyReadable

You can view a demo here: https://www-demo2.allizom.org/en-US/firefox/reading/start/

Note: we had to add lorem copy to trigger the Reader icon for tests.

The question is will the work being done in this bug bypass the requirement for isProbablyReadable and just move this page into Reader View.

Secondary question is: is it possible to add this page as an exception so the Reader icon will show up regardless of content.

Comment 16

[:Gijs (he/him)]

(In reply to Cory Price [:ckprice] from comment #15)
> (In reply to :Margaret Leibovic from comment #14)
> > I'm not sure I have enough context here about how the FTU experience works,
> > but if we're loading a webpage in a tab, the isProbablyReaderable code will
> > automatically kick in, as Blair suggested, as we automatically show the
> > reader view button for any page that we detect as "probably readerable" on
> > page load.
> See attachment 8574210 [details] for a wireframe of the tour.
> 
> Basically, this page will be a background for the tour door hangers. 
> 
> 1. It's not technically an 'article'
> 2. Tested and looks okay in Reader View
> 3. It probably will not meet the criteria needed for isProbablyReadable
> 
> You can view a demo here:
> https://www-demo2.allizom.org/en-US/firefox/reading/start/
> 
> Note: we had to add lorem copy to trigger the Reader icon for tests.
> 
> The question is will the work being done in this bug bypass the requirement
> for isProbablyReadable and just move this page into Reader View.
> 
> Secondary question is: is it possible to add this page as an exception so
> the Reader icon will show up regardless of content.

It's certainly possible, but I wonder if it isn't easier if you just add 5 sufficiently long paragraphs with a style="display: none" attribute, which IIRC won't be removed by reader mode, so it should stay hidden.

Comment 17

[Alex Gibson [:agibson]]

(In reply to :Gijs Kruitbosch from comment #16)
> It's certainly possible, but I wonder if it isn't easier if you just add 5
> sufficiently long paragraphs with a style="display: none" attribute, which
> IIRC won't be removed by reader mode, so it should stay hidden.

Thanks for the suggestion and something I had not yet tried. I just tested this locally, and while an inline style (style="display: none") does hide the text from a web page, it is still parsed and shown in Reader View.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Visiting the page will force Reader Mode on, but clicking the Exit button will cause Reader Mode to get re-entered since the URL still matches. Should we be only forcing entrance on the first visit to this URL?

Comment 19

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Thanks Jared - sent you an email, but we can resolve here.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> the first visit to this URL?
What is the scope here? Tab, session, user, etc?

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

We could do per session (a variable stored in memory) or per profile (a pref in about:config) if we wanted.

Comment 21

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> We could do per session (a variable stored in memory) or per profile (a pref
> in about:config) if we wanted.
Per session would be preferred. :habber thoughts?

Comment 22

[Holly Habstritt Gaal [:Habber]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> We could do per session (a variable stored in memory) or per profile (a pref
> in about:config) if we wanted.

Hi Jared. Cory, MattN, Agibson, and I discussed this a bit. Having this be Tab-based would be better than session-based. However, we want to do our best to be sure that when a user clicks on the URL (/reading/start) they receive the intended experience, which is displaying the tour. 

Going with tab-based is better, but results in 2 other cases to solve for. Could we tie this to the action that precedes it? For example, whenever they enter the experience via our URL, the tour always happens. 
- Tab-based wouldn't account for assuring the tour would start again if the user hits their back button, then returns to the page in this same tab (e.g., clicks back button to return to Facebook and clicks the campaign link in their feed). 
- Or toggles the Reader View on and off. 
(:MattN had some ideas for solving for the the first case above.)

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

The more I think about this, I don't think we should auto-enter Reader Mode. We can have a mozTour API request to put us in to reader mode, and this page can have some text that says "Click this link to start the tour" or something like that.

Then clicking on that link will just make the mozTour API call to enter Reader Mode. This seems like the simplest and less-side effect causing route.

The link will probably still be visible within Reader Mode, but as long as the text is written in such a way it shouldn't be too awkward.

Comment 24

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> The more I think about this, I don't think we should auto-enter Reader Mode.
> We can have a mozTour API request to put us in to reader mode, and this page
> can have some text that says "Click this link to start the tour" or
> something like that.
Thanks :jaws, we're considering this option. In the meantime, can you answer a few questions:

1. Does the aforementioned mozTour API request already exist? Or will this have to be developed? If it does have to be developed, will it land in RC 38.1?

2. How would this interact with the doorhangers? Would this API also kick off the tour?

3. Will the page have to meet the requirement for isProbablyReaderable to enter Reader View (e.g. x paragraphs with y characters)? Or would the mozTour API bypass this requirement?

Thanks

Comment 25

[Holly Habstritt Gaal [:Habber]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> The more I think about this, I don't think we should auto-enter Reader Mode.
> We can have a mozTour API request to put us in to reader mode, and this page
> can have some text that says "Click this link to start the tour" or
> something like that.
> 
> Then clicking on that link will just make the mozTour API call to enter
> Reader Mode. This seems like the simplest and less-side effect causing route.
> 
> The link will probably still be visible within Reader Mode, but as long as
> the text is written in such a way it shouldn't be too awkward.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> The more I think about this, I don't think we should auto-enter Reader Mode.
> We can have a mozTour API request to put us in to reader mode, and this page
> can have some text that says "Click this link to start the tour" or
> something like that.
> 
> Then clicking on that link will just make the mozTour API call to enter
> Reader Mode. This seems like the simplest and less-side effect causing route.
> 
> The link will probably still be visible within Reader Mode, but as long as
> the text is written in such a way it shouldn't be too awkward.


Does this solve all of the issues mentioned in commend 22? Just curious since it results in one downside we should consider. In the copy surrounding the promoted link (e.g., on FB or in a snippet), we'll set up the feature and give them the expectation that they can try it by clicking on the link. It presents an extra click for the user to start the tour after landing on the web page. We know with other tours, relying on a user to interact with a CTA in a web page in order to trigger a tour can reduce engagement. However, the technical/scope benefits might outweigh this. 

Your suggestion would basically look like this, correct?: 
1. User lands on our web page w/ CTA on page to start tour - http://cl.ly/image/401O3l0h3522 (w/ more padding around the button)
2. User clicks on CTA which puts page into Reader View and starts tour door hangers.

A positive aspect of your solution allows the user to see the page transition from full stylized web page into Reader View and demonstrate how it works.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Cory Price [:ckprice] from comment #24)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> > The more I think about this, I don't think we should auto-enter Reader Mode.
> > We can have a mozTour API request to put us in to reader mode, and this page
> > can have some text that says "Click this link to start the tour" or
> > something like that.
> Thanks :jaws, we're considering this option. In the meantime, can you answer
> a few questions:
> 
> 1. Does the aforementioned mozTour API request already exist? Or will this
> have to be developed? If it does have to be developed, will it land in RC
> 38.1?

The mozTour API request is part of the patch that is attached to this bug. It would have to be uplifted to 38.1.

> 2. How would this interact with the doorhangers? Would this API also kick
> off the tour?

The API would start up reader mode, which can then in turn kick off the tour based on the other work that is done to start the tour when Reader Mode is entered (via various other bugs).

> 3. Will the page have to meet the requirement for isProbablyReaderable to
> enter Reader View (e.g. x paragraphs with y characters)? Or would the
> mozTour API bypass this requirement?

Yes, it should still meet this requirement. You may be able to add the "visually-hidden" class to the text to trick reader mode in to thinking that the page is readable but not show the text in reader mode, but I'm not sure.

(In reply to Holly Habstritt Gaal [:Habber] from comment #25)
> Does this solve all of the issues mentioned in commend 22? Just curious
> since it results in one downside we should consider. In the copy surrounding
> the promoted link (e.g., on FB or in a snippet), we'll set up the feature
> and give them the expectation that they can try it by clicking on the link.
> It presents an extra click for the user to start the tour after landing on
> the web page. We know with other tours, relying on a user to interact with a
> CTA in a web page in order to trigger a tour can reduce engagement. However,
> the technical/scope benefits might outweigh this. 
> 
> Your suggestion would basically look like this, correct?: 
> 1. User lands on our web page w/ CTA on page to start tour -
> http://cl.ly/image/401O3l0h3522 (w/ more padding around the button)
> 2. User clicks on CTA which puts page into Reader View and starts tour door
> hangers.

Yeah, that's correct.
 
> A positive aspect of your solution allows the user to see the page
> transition from full stylized web page into Reader View and demonstrate how
> it works.

Another way to do it might be to draw a huge arrow pointing at the Reader Mode icon in the location bar, after all that is where the user will have to click for every other article on the internet. Using graphics to point to the Reader Mode icon (and text saying to click on the icon) might transition better to the Reader Mode view anyways.

Comment 27

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> The more I think about this, I don't think we should auto-enter Reader Mode.
> We can have a mozTour API request to put us in to reader mode, and this page
> can have some text that says "Click this link to start the tour" or
> something like that.

This was the original plan months ago and I agree that it's less complex and magical.

> Then clicking on that link will just make the mozTour API call to enter
> Reader Mode. This seems like the simplest and less-side effect causing route.

I think the main reason we ended up going with this was because we wanted to handle visiting this page directly in reader view via the reading list entry the same way. As long as we are able to implement what we desire for the tour when clicking a reading list entry which goes directly into reader view (meaning the page can't run JS to call the UITour API then this is fine with me.


> The link will probably still be visible within Reader Mode, but as long as
> the text is written in such a way it shouldn't be too awkward.

Does reader mode show <button>s? I kinda would hope not so we could look into that too.

Comment 28

[Holly Habstritt Gaal [:Habber]]

> Another way to do it might be to draw a huge arrow pointing at the Reader
> Mode icon in the location bar, after all that is where the user will have to
> click for every other article on the internet. Using graphics to point to
> the Reader Mode icon (and text saying to click on the icon) might transition
> better to the Reader Mode view anyways.

Not a bad idea, but I think we should take this off the table for the following reasons. Positioning of this kind of element is always tricky since the user's viewport is not predictable, so keeping that arrow in the proper position can be a challenge. I'd be much more in favor of the CTA/button triggering the tour. We can also measure the effectiveness/clicks of the button which is an added benefit.

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Holly Habstritt Gaal [:Habber] from comment #28)
> Positioning of this kind of element is always tricky
> since the user's viewport is not predictable

Oh, you're definitely right. Thanks for pointing that out!

Comment 30

[Matthew N. [:MattN]]

We could anchor a tour panel there with a button to start the tour.

Comment 31

[Holly Habstritt Gaal [:Habber]]

(In reply to Matthew N. [:MattN] from comment #30)
> We could anchor a tour panel there with a button to start the tour.

This is a good idea, but it would obscure the page messaging. Let's prioritize having the button embedded in the web page. If that is not possible by 38.1, we'll use a tour panel anchored on the Reader View icon in the location bar as a fallback. 

Ckprice, is this prioritization ok with you? What do we need resolved to know if the button in the web page is possible? I understand we'd need to figure out strings asap.

Comment 32

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

RE: button kicking off reader view & ftu via mozTour API

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #26)
> The mozTour API request is part of the patch that is attached to this bug.
> It would have to be uplifted to 38.1.
I'm a bit concerned with introducing more work to this process. It sounds like _some_ of the functionality for this is complete, but more API work is necessary.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #26)
> Yes, it should still meet this requirement. You may be able to add the
> "visually-hidden" class to the text to trick reader mode in to thinking that
> the page is readable but not show the text in reader mode, but I'm not sure.
It also doesn't help our reader view requirements issue, so we'll have to go back to design/copy regardless for more than just the CTA string.

Basically we're fixing the scope issue, but replacing it with more API work. There's also a possibility that the button CTA/text will show in Reader View as noted by :MattN.

NI jaws - how confident are we that the API work can land for 38.1? Is this significantly more/less work than the original ask in this bug?
NI margaret - way back in comment 9 you mentioned a whitelist. Could this be an option for 38.1?

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

How about we do it like this:
1) We add the API for mozTour to request that the page be put in to reader mode. The complexity of this is not too high for us to uplift to 38.1.
2) The webpage on first-load checks to see if a cookie (or some other mechanism) is present. If not, then it sets the cookie and requests the page be loaded in reader view.
3) If the user subsequently exits reader view and loads the page, they will not get auto-bounced in to reader view, but some form of button/link on the page can trigger loading reader-view again if the user wishes to restart the tour.

We will always start the tour if the about:reader?url=special-url is visited.

Comment 34

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #33)
> How about we do it like this:
Thank you so much for thinking this through!

If we're building the API and there's no concern with that landing in 38.1 (with some time to test), I think it'd be more appropriate (and simple) to have the user click a button to bring them into Reader View.

1. We will add a button per :habber's suggestion (http://cl.ly/image/401O3l0h3522)
2. If clicked, we'll use the API to move the user into Reader View 
** Can we hide the <button> copy?

This would avoid us having to work with cookies and avoid us having to contend with any possible race conditions/odd UX of moving into Reader View on page load.

:agibson - what do you think of this?

I'll close this bug as INVALID once we've decided a direction.

> We will always start the tour if the about:reader?url=special-url is visited.
NI jaws - Can you elaborate on this comment?

Comment 35

[Alex Gibson [:agibson]]

(In reply to Cory Price [:ckprice] from comment #34)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #33)
> > How about we do it like this:
> Thank you so much for thinking this through!
> 
> If we're building the API and there's no concern with that landing in 38.1
> (with some time to test), I think it'd be more appropriate (and simple) to
> have the user click a button to bring them into Reader View.
> 
> 1. We will add a button per :habber's suggestion
> (http://cl.ly/image/401O3l0h3522)
> 2. If clicked, we'll use the API to move the user into Reader View 
> ** Can we hide the <button> copy?
> 
> This would avoid us having to work with cookies and avoid us having to
> contend with any possible race conditions/odd UX of moving into Reader View
> on page load.
> 
> :agibson - what do you think of this?
> 
> I'll close this bug as INVALID once we've decided a direction.
> 
> > We will always start the tour if the about:reader?url=special-url is visited.
> NI jaws - Can you elaborate on this comment?

This all sounds fine to me. +1 for having the user click a button in the page to enter Reader View. I think this would be a nicer UX overall, as well as being technically simpler.

Comment 36

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

NI jaws

A couple final questions

1. What is required for us to start development on this API? Shall I create a bug and reference this bug?
2. Can we build the API in such a way that we bypass the isProbablyReaderable requirements? We have the copy passing in en-us (5 paragraphs of 100 characters each), but there are edgecases in other locales that may drop us below the minimum.

Comment 37

[:Margaret Leibovic]

(In reply to Cory Price [:ckprice] from comment #32)

> NI margaret - way back in comment 9 you mentioned a whitelist. Could this be
> an option for 38.1?

Sorry for the delay here, but it sounds like you and Jared reached a solution that won't require this.

(In reply to Cory Price [:ckprice] from comment #36)

> 2. Can we build the API in such a way that we bypass the
> isProbablyReaderable requirements? We have the copy passing in en-us (5
> paragraphs of 100 characters each), but there are edgecases in other locales
> that may drop us below the minimum.

Perhaps we can support some sort of meta tag that allows developers to specify pages as reader-able? Gijs, what do you think about this?

Comment 38

[:Gijs (he/him)]

(In reply to :Margaret Leibovic from comment #37)
> (In reply to Cory Price [:ckprice] from comment #32)
> 
> > NI margaret - way back in comment 9 you mentioned a whitelist. Could this be
> > an option for 38.1?
> 
> Sorry for the delay here, but it sounds like you and Jared reached a
> solution that won't require this.
> 
> (In reply to Cory Price [:ckprice] from comment #36)
> 
> > 2. Can we build the API in such a way that we bypass the
> > isProbablyReaderable requirements? We have the copy passing in en-us (5
> > paragraphs of 100 characters each), but there are edgecases in other locales
> > that may drop us below the minimum.
> 
> Perhaps we can support some sort of meta tag that allows developers to
> specify pages as reader-able? Gijs, what do you think about this?

I mean, it depends what we want to achieve. A meta tag would be universally usable. A UITour API (which is another relatively simple option) would only be available to the websites where we currently want this.


On the one hand, adding non-standard features to the "web" part of pages makes me feel a little worried (as well as: do we really want to give sites explicit control over the availability of this part of browser UI?), on the other, adding functionality just for our own web-page that ISTR we have heard interest in from other parties seems wrong, too.

dolske/margaret, thoughts?

Comment 39

[:Margaret Leibovic]

(In reply to :Gijs Kruitbosch from comment #38)
> (In reply to :Margaret Leibovic from comment #37)
> > (In reply to Cory Price [:ckprice] from comment #32)
> > 
> > > NI margaret - way back in comment 9 you mentioned a whitelist. Could this be
> > > an option for 38.1?
> > 
> > Sorry for the delay here, but it sounds like you and Jared reached a
> > solution that won't require this.
> > 
> > (In reply to Cory Price [:ckprice] from comment #36)
> > 
> > > 2. Can we build the API in such a way that we bypass the
> > > isProbablyReaderable requirements? We have the copy passing in en-us (5
> > > paragraphs of 100 characters each), but there are edgecases in other locales
> > > that may drop us below the minimum.
> > 
> > Perhaps we can support some sort of meta tag that allows developers to
> > specify pages as reader-able? Gijs, what do you think about this?
> 
> I mean, it depends what we want to achieve. A meta tag would be universally
> usable. A UITour API (which is another relatively simple option) would only
> be available to the websites where we currently want this.
> 
> 
> On the one hand, adding non-standard features to the "web" part of pages
> makes me feel a little worried (as well as: do we really want to give sites
> explicit control over the availability of this part of browser UI?), on the
> other, adding functionality just for our own web-page that ISTR we have
> heard interest in from other parties seems wrong, too.
> 
> dolske/margaret, thoughts?

For the Fx38 timeframe, a UITour API is probably the safer bet, but I think it would be interesting to explore ways we can let webpages "support" reader view.

I'm just not familiar with how the UITour APIs work. Is it possible to create an API to show the button without meddling with our current reader view button logic? I was just thinking that if we do need to change the reader view button logic, it could be worth considering whether we should do something more general, rather than a hack specific to UI tour.

Comment 40

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Comment 42

[:Gijs (he/him)]

(In reply to :Margaret Leibovic from comment #39)
> I'm just not familiar with how the UITour APIs work. Is it possible to
> create an API to show the button without meddling with our current reader
> view button logic?

I think now that we only send "yes" updates and "no" updates on pagehide, we can just make the uitour-content bits send a "yes" update based on a page telling uitour to do that, or, if this isn't e10s-compatible yet, to set browser.isArticle to true and call updateReaderButtonState or whatever the method is called.

So yeah, I think we can do this without meddling with the logic.

> I was just thinking that if we do need to change the
> reader view button logic, it could be worth considering whether we should do
> something more general, rather than a hack specific to UI tour.

Right. I think the approach I suggested is non-hacky enough to be workable for the issue at hand.


That said, looking at Jared's patch, I think it already does everything required here? The icon might not be visible, but if the page calls the right API / fires the right custom event, the page will go into reader mode. We might, of course, still want to ensure that the icon *is* visible... :ckprice, want to clarify?

Comment 43

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #42)
> (In reply to :Margaret Leibovic from comment #39)
> > I'm just not familiar with how the UITour APIs work. Is it possible to
> > create an API to show the button without meddling with our current reader
> > view button logic?
> 
> I think now that we only send "yes" updates and "no" updates on pagehide


Sigh, re-reading, this is confusing. What I meant is:

I think now that we only send "yes" updates, and only send "no" updates on pagehide (ie when we move from one page to the next), and therefore are guaranteed not to hide the icon again until after the page goes away, ...

Comment 44

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to :Gijs Kruitbosch from comment #42)
> That said, looking at Jared's patch, I think it already does everything
> required here? The icon might not be visible, but if the page calls the
> right API / fires the right custom event, the page will go into reader mode.
> We might, of course, still want to ensure that the icon *is* visible...
> :ckprice, want to clarify?
I think the icon not being visible _may_ be acceptable if we know that the API will still move a user into Reader View even if the page doesn't qualify.

Hopefully this will be an edgecase as we've modified copy to meet the requirement of 5 paragraphs of 100 characters or more in en-US (ref bug 1149859 comment 6). The edgecase is ja/zh where we will have 40-50% less characters. All other locales will have more than en-US.

:habber, this sounds okay to me. What do you think?

Comment 45

[Holly Habstritt Gaal [:Habber]]

(In reply to Cory Price [:ckprice] from comment #44)
> (In reply to :Gijs Kruitbosch from comment #42)
> > That said, looking at Jared's patch, I think it already does everything
> > required here? The icon might not be visible, but if the page calls the
> > right API / fires the right custom event, the page will go into reader mode.
> > We might, of course, still want to ensure that the icon *is* visible...
> > :ckprice, want to clarify?
> I think the icon not being visible _may_ be acceptable if we know that the
> API will still move a user into Reader View even if the page doesn't qualify.
> 
> Hopefully this will be an edgecase as we've modified copy to meet the
> requirement of 5 paragraphs of 100 characters or more in en-US (ref bug
> 1149859 comment 6). The edgecase is ja/zh where we will have 40-50% less
> characters. All other locales will have more than en-US.
> 
> :habber, this sounds okay to me. What do you think?

In the FTE tour we highlight the Reader View icon and extend a doorhanger from it in order to show users where it is. Therefore, the Reader View icon should be visible. (See wireframe mockup http://cl.ly/image/2D2u2K0R2y2E)  

For the edgecase locales (assuming it's only 3-6 locales that we discussed last week) how would we handle this second door hanger that points to the Reader View icon if the icon is not visible?

Comment 46

[:Gijs (he/him)]

Attachment: MozReview Request: bz://1134501/Gijs

/r/7001 - Bug 1134501 - add way for UITour'd page to force-show the reader mode button, r?margaret,jaws

Pull down this commit:

hg pull -r 03a24dd7c39af01fe1ad4363404a2f9af7eb2ce0 https://reviewboard-hg.mozilla.org/gecko/

Comment 47

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Holly Habstritt Gaal [:Habber] from comment #45)
> For the edgecase locales (assuming it's only 3-6 locales that we discussed
> last week) how would we handle this second door hanger that points to the
> Reader View icon if the icon is not visible?
Could we just remove the highlight? And have the info panel pointing the general area of where the icon would be?

Comment 48

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

^^ :habber

Comment 49

[:Gijs (he/him)]

(In reply to Cory Price [:ckprice] from comment #48)
> ^^ :habber

All of this seemed complicated so I just put up a patch that will let a UITour page trigger the icon.

Comment 50

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to :Gijs Kruitbosch from comment #49)
> (In reply to Cory Price [:ckprice] from comment #48)
> > ^^ :habber
> 
> All of this seemed complicated so I just put up a patch that will let a
> UITour page trigger the icon.
Thanks :gijs

Updated summary to something more appropriate.

Comment 51

[:Gijs (he/him)]

Comment on attachment 8591087 [details] [diff] [review]
Patch v2

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

I would like to skip using content-UITour for the page load work as it seems this could be handled fully by UITour.jsm which would simplify some of the code.

::: browser/app/profile/firefox.js
@@ +251,5 @@
>  pref("browser.uitour.requireSecure", true);
>  pref("browser.uitour.themeOrigin", "https://addons.mozilla.org/%LOCALE%/firefox/themes/");
>  pref("browser.uitour.url", "https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/tour/");
> +// This is used as a regexp match against the page's URL.
> +pref("browser.uitour.readerViewTrigger", "^about:reader?url=https://www.mozilla.org/[^/]+/firefox/reading/start/");

Shouldn't the URL be URL-escaped here or something?

::: browser/components/uitour/UITour-lib.js
@@ -284,5 @@
>  })();
>  
>  // Make this library Require-able.
>  if (typeof module !== 'undefined' && module.exports) {
> -  module.exports = Mozilla.UITour;

What changed here?

::: browser/components/uitour/content-UITour.js
@@ +76,5 @@
> +    // The ReadingList/ReaderView tour page is expected to run in Reader View,
> +    // which disables JavaScript on the page. To get around that, we
> +    // automatically start a pre-defined tour on page load.
> +    let readerViewPattern = new RegExp(Services.prefs.getCharPref(PREF_READERVIEW_TRIGGER));
> +    if (readerViewPattern.exec(content.document.documentURI)) {

.test instead of .exec please.

I think this is the wrong place for this code though... why don't we add it in UITour.jsm (parent/chrome-side) and make the browser's own load code trigger a method in UITour.jsm that checks the page URI and then calls the relevant method directly? Cuts out another messaging round-trip, too.

@@ +81,5 @@
> +      // For simplicity and avoid adding an extra API, just use the existing
> +      // mozUITour event handling API.
> +      this.handleAPIEvent({
> +        detail: {
> +          action: "startSubTour",

I can has tests?

Comment 52

[Holly Habstritt Gaal [:Habber]]

(In reply to :Gijs Kruitbosch from comment #49)
> (In reply to Cory Price [:ckprice] from comment #48)
> > ^^ :habber
> 
> All of this seemed complicated so I just put up a patch that will let a
> UITour page trigger the icon.

Thank you Gijs! It was going to be pretty confusing for some locales to see a doorhanger pointing to an empty space where the icon should be. Since the purpose of this experience is to introduce Reader View and Reading List to users, I think your solution is appropriate.

Comment 53

[:Margaret Leibovic]

Comment on attachment 8591835 [details]
MozReview Request: bz://1134501/Gijs

https://reviewboard.mozilla.org/r/6999/#review5819

::: browser/components/uitour/test/browser_UITour_forceReaderMode.js
(Diff revision 1)
> +    yield waitForConditionPromise(() => gBrowser.selectedBrowser.isArticle);

Maybe we should add a check to make sure this somehow wasn't already an article before calling forceShowReaderIcon.

Comment 54

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2.1

Hey, do you see why this is causing the following console message when I run my test?

[JavaScript Error: "UITour is not defined" {file: "chrome://browser/content/browser.js" line: 14919}]

If I use the debugger and step through the code that line works. I can only reproduce this error when running the tests, it doesn't show up in the Error Console when running the browser manually.

Comment 55

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #51)
> ::: browser/components/uitour/UITour-lib.js
> @@ -284,5 @@
> >  })();
> >  
> >  // Make this library Require-able.
> >  if (typeof module !== 'undefined' && module.exports) {
> > -  module.exports = Mozilla.UITour;
> 
> What changed here?

Nothing as far as I can tell. I re-did the same change and that line got changed again. Used BeyondCompare to see if there were any line-ending differences or whitespace differences and couldn't see them either.

Comment 56

[:Gijs (he/him)]

(In reply to (behind on reviews) Jared Wein [:jaws] (please needinfo? me) from comment #54)
> Created attachment 8591948 [details] [diff] [review]
> Patch v2.1
> 
> Hey, do you see why this is causing the following console message when I run
> my test?
> 
> [JavaScript Error: "UITour is not defined" {file:
> "chrome://browser/content/browser.js" line: 14919}]
> 
> If I use the debugger and step through the code that line works. I can only
> reproduce this error when running the tests, it doesn't show up in the Error
> Console when running the browser manually.

I bet that onLocationChange is getting called earlier in the test, or later (while/when the window is being/already torn down) and either the getter is somehow destroyed or the module is unloaded or whatever, and so the UITour object dies.

To figure out exact specifics, add a !UITour if statement in the onLocationChange handler that dumps a stack, and see where in the log it appears and with what stack.

Comment 57

[:Gijs (he/him)]

Comment on attachment 8591948 [details] [diff] [review]
Patch v2.1

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

Nice! This looks pretty good, just some minor suggestions below - that and fixing the error while running the tests and we should be good, I think.

::: browser/app/profile/firefox.js
@@ +251,5 @@
>  pref("browser.uitour.requireSecure", true);
>  pref("browser.uitour.themeOrigin", "https://addons.mozilla.org/%LOCALE%/firefox/themes/");
>  pref("browser.uitour.url", "https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/tour/");
> +// This is used as a regexp match against the page's URL.
> +pref("browser.uitour.readerViewTrigger", "^about:reader?url=https%3A%2F%2Fwww.mozilla.org%2F[^/]+%2Ffirefox%2Freading%2Fstart%2F");

Did you test this? :-)

Also, because this is a regex, if someone else owns wwwxmozilla.org they could get this to match. :-)

I wonder if it's easier/better/safer to just check for about:reader and then use ReaderMode.getOriginalURL once Margaret finishes off bug 1152121 (she has r+ on the patch, so that should be "soon") to get the original URL. Might be more convoluted / more work though. In which case, let me revert to just "please backslash-escape the '.'s in the domain" :-)

::: browser/components/uitour/UITour.jsm
@@ +348,5 @@
> +  onLocationChange: function(aLocation) {
> +    // The ReadingList/ReaderView tour page is expected to run in Reader View,
> +    // which disables JavaScript on the page. To get around that, we
> +    // automatically start a pre-defined tour on page load.
> +    let readerViewPattern = new RegExp(Services.prefs.getCharPref(PREF_READERVIEW_TRIGGER));

Nit: please cache the regexp version in a lazy getter
Nit: please pass "i" as the second param so that the resulting regex is case-insensitive.

::: browser/components/uitour/test/browser.ini
@@ +32,5 @@
>  skip-if = e10s # Bug 941428 - UITour.jsm not e10s friendly
>  [browser_UITour_sync.js]
>  skip-if = e10s # Bug 941428 - UITour.jsm not e10s friendly
> +[browser_UITour_toggleReaderMode.js]
> +skip-if = e10s # Bug 941428 - UITour.jsm not e10s friendly

After we're done here we should update all these bug references to bug 1073247 at one point. 941428 is marked fixed, and UITour should work in e10s, but we don't run tests because the tests are busted.

::: browser/components/uitour/test/browser_UITour_toggleReaderMode.js
@@ +1,1 @@
> +"use strict";

This file has weird line-ends when I look at the raw diff. Does running dos2unix on this file help?

@@ +14,5 @@
> +  taskify(function*() {
> +    ok(!gBrowser.selectedBrowser.currentURI.spec.startsWith("about:reader"), "Should not be in reader mode at start of test.");
> +    gContentAPI.toggleReaderMode();
> +    yield waitForConditionPromise(() => gBrowser.selectedBrowser.currentURI.spec.startsWith("about:reader"));
> +    ok(gBrowser.selectedBrowser.currentURI.spec.startsWith("about:reader"), "Should be in reader mode now.");

Do we not need to leave reader mode again in order to not mess up subsequent tests?

Comment 58

[:Gijs (he/him)]

(In reply to :Margaret Leibovic from comment #53)
> Comment on attachment 8591835 [details]
> MozReview Request: bz://1134501/Gijs
> 
> https://reviewboard.mozilla.org/r/6999/#review5819
> 
> ::: browser/components/uitour/test/browser_UITour_forceReaderMode.js
> (Diff revision 1)
> > +    yield waitForConditionPromise(() => gBrowser.selectedBrowser.isArticle);
> 
> Maybe we should add a check to make sure this somehow wasn't already an
> article before calling forceShowReaderIcon.

Good call!

remote:   https://hg.mozilla.org/integration/fx-team/rev/9d319ca0f937

Comment 59

[Jared Wein [:jaws] (please needinfo? me)]

https://reviewboard.mozilla.org/r/7001/#review5903

Ship It!

Comment 60

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2.2

We don't need to leave Reader View after the test ends because the UITourTest() opens a new tab and then when the test file is done the tab is closed (which implicitly exits reader mode).

Comment 61

[:Gijs (he/him)]

Comment on attachment 8593107 [details] [diff] [review]
Patch v2.2

Great, thanks!

r=me assuming you figured out the test issue.

Comment 62

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/9d319ca0f937

Comment 63

[Jared Wein [:jaws] (please needinfo? me)]

 https://hg.mozilla.org/integration/fx-team/rev/364bc6209cc5

Comment 64

[Phil Ringnalda (:philor)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #63)
>  https://hg.mozilla.org/integration/fx-team/rev/364bc6209cc5

Jetpack says... https://treeherder.mozilla.org/logviewer.html#?job_id=2702229&repo=fx-team

Backed out in https://hg.mozilla.org/integration/fx-team/rev/713fa47e79f4

Comment 65

[Phil Ringnalda (:philor)]

And also https://treeherder.mozilla.org/logviewer.html#?job_id=2702444&repo=fx-team

Comment 66

[Phil Ringnalda (:philor)]

And my favorite for the unlikelihood, https://treeherder.mozilla.org/logviewer.html#?job_id=2702788&repo=fx-team (ignore the parsed error about graphserver, that's pretty much always bullshit, it's actually some sort of a tart failure).

Comment 67

[:Gijs (he/him)]

Comment on attachment 8593107 [details] [diff] [review]
Patch v2.2

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

::: browser/components/uitour/UITour.jsm
@@ +349,5 @@
>  
> +  get _readerViewTriggerRegEx() {
> +    delete this.readerViewTriggerRegEx;
> +    let readerViewUITourTrigger = Services.prefs.getCharPref(PREF_READERVIEW_TRIGGER);
> +    return this._readerViewTriggerRegEx = new RegExp(readerViewUITourTrigger, "i");

Missed this, the deleted thing has no _ so I think this then doesn't get set (the logs have "setting property which only has a getter" errors).

And if calling this getter causes exceptions... I bet it breaks 'stuff'. :-(

Comment 68

[Pulsebot]

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

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f723ff730047

Comment 70

[:Gijs (he/him)]

Comment on attachment 8593107 [details] [diff] [review]
Patch v2.2

Approval Request Comment
[Feature/regressing bug #]: reader mode
[User impact if declined]: no way for UI tour to force-show reader mode icon
[Describe test coverage new/current, TreeHerder]: has test!
[Risks and why]: pretty low, UITour-specific addition
[String/UUID change made/needed]: nope

Comment 71

[:Gijs (he/him)]

Comment on attachment 8591835 [details]
MozReview Request: bz://1134501/Gijs

Approval Request Comment
[Feature/regressing bug #]: reader mode
[User impact if declined]: no way to show an info panel in UITour for reader mode
[Describe test coverage new/current, TreeHerder]: nope
[Risks and why]: pretty low, UITour only
[String/UUID change made/needed]: nope

(I seem to have gotten the approval requests the wrong way around for these patches. Sorry.)

Comment 72

[Liz Henry (:lizzard) (relman/hg->git project)]

From talking with Gijs on irc it sounds like uplifting this to aurora won't affect anything directly -- it will just put the code in place for when we want to turn on the UI tour.  On the other hand, I don't think we have any need to uplift these patches if we aren't intending to use them in 38 and 39. 

Dolske, what do you think?

Comment 73

[Justin Dolske [:Dolske]]

We won't be shipping Reading List in 38.1, but will be shipping Reading View, which this exposes. What we'll be doing in the tour is a little in flux (bug 1155541), but I think we should go ahead and uplift this so we have it if needed.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cebe7cad65a2

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e5d6dc48f6de

Comment 76

[Andrei Vaida [:avaida]]

I'm a little confused about whether there's something here for manual QA to look at, since the UI tour is going to change, according to Bug 1155541. Setting qe-verify- for now, as this patch is also covered by automated tests - Jared, if you think otherwise, please let me know.

Comment 77

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #58)
> (In reply to :Margaret Leibovic from comment #53)
> > Comment on attachment 8591835 [details]
> > MozReview Request: bz://1134501/Gijs
> > 
> > https://reviewboard.mozilla.org/r/6999/#review5819
> > 
> > ::: browser/components/uitour/test/browser_UITour_forceReaderMode.js
> > (Diff revision 1)
> > > +    yield waitForConditionPromise(() => gBrowser.selectedBrowser.isArticle);
> > 
> > Maybe we should add a check to make sure this somehow wasn't already an
> > article before calling forceShowReaderIcon.
> 
> Good call!
> 
> remote:   https://hg.mozilla.org/integration/fx-team/rev/9d319ca0f937

It looks like this didn't get uplifted. I don't see it in mozilla-release but it is in mozilla-central.

Comment 78

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Just a note that we will not need these API's for 38.0.5 (June 2).

The UITour has changed a bit for this launch. The required bugs are in bug 1155541.

Comment 79

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #77)
> (In reply to :Gijs Kruitbosch from comment #58)
> > (In reply to :Margaret Leibovic from comment #53)
> > > Comment on attachment 8591835 [details]
> > > MozReview Request: bz://1134501/Gijs
> > > 
> > > https://reviewboard.mozilla.org/r/6999/#review5819
> > > 
> > > ::: browser/components/uitour/test/browser_UITour_forceReaderMode.js
> > > (Diff revision 1)
> > > > +    yield waitForConditionPromise(() => gBrowser.selectedBrowser.isArticle);
> > > 
> > > Maybe we should add a check to make sure this somehow wasn't already an
> > > article before calling forceShowReaderIcon.
> > 
> > Good call!
> > 
> > remote:   https://hg.mozilla.org/integration/fx-team/rev/9d319ca0f937
> 
> It looks like this didn't get uplifted. I don't see it in mozilla-release
> but it is in mozilla-central.

OK. Per comment #78, do we need to?

Comment 80

[:Gijs (he/him)]

Comment on attachment 8591835 [details]
MozReview Request: bz://1134501/Gijs

per IRC discussion

Approval Request Comment
[Feature/regressing bug #]: reader mode and so on
[User impact if declined]: if we change UITour plans again it won't be on mozilla-release; it makes uplifting other patches more difficult.
[Describe test coverage new/current, TreeHerder]: has test
[Risks and why]: low risk because of the test and extended baking
[String/UUID change made/needed]: no.

Comment 81

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8591835 [details]
MozReview Request: bz://1134501/Gijs

As stated, this fix has been on m-c for a while. We'll take it in 38.0.5.

Comment 82

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-release/rev/5741ccc7bb74

Comment 83

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/843ba44b7968

Comment 85

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1134501 - add way for UITour'd page to force-show the reader mode button, r?margaret,jaws