1266372 - Reader view button in urlbar and back button in reader view work differently on facebook articles

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160416030220
STR_1:
1. Open https://www.facebook.com/permalink.php?story_fbid=209620482737575&id=100010688735430
2. Click reader view button in urlbar
3. Click reader view button in urlbar

STR_2: 
1. Open https://www.facebook.com/permalink.php?story_fbid=209620482737575&id=100010688735430
2. Click reader view button in urlbar
3. Click back button in reader view

AR:  STR_1 - performs history.go(-1).  STR_2 - adds new item in tab history.
ER:  The result should be the same in both cases.

I'm marking it as regression for now. It was caused by bug 1184950. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=6f8f469a45c08371398d2a98bb7fb7b2a5d07355&tochange=759244c242e1c96a2fad70608e4a646aa9a5761c

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thanks for filing.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

This patch sends the reference of AboutReaderListener to the created
AboutReader instance, so it would be able to call closeReaderMode when
the user hits the close button.

Review commit: https://reviewboard.mozilla.org/r/48335/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48335/

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8744155 [details]
MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

I've spent some time thinking about how to reuse the go-back-or-not logic when we need to leave the reader mode. This is my conclusion for now.

The alternative would be to expose or send content/docShell to either AboutReader instance or ReaderMode object, or keep a reference of AboutReader instance in AboutReaderListener (which is bad because we would be creating many instances during the life time of the tab).

This is a WIP, because the Fennec part would need to be done on top of bug 1264805.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I can't add bug 1264805 to dependency because that would be circular...

Comment 5

[:Margaret Leibovic]

Comment on attachment 8744155 [details]
MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

Also flagging Gijs.

Comment 6

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/48335/#review45371

::: browser/base/content/tab-content.js:254
(Diff revision 1)
>    init: function() {
>      addEventListener("AboutReaderContentLoaded", this, false, true);
>      addEventListener("DOMContentLoaded", this, false);
>      addEventListener("pageshow", this, false);
>      addEventListener("pagehide", this, false);
> +    addEventListener("pageshow", this, false);

There's already a pageshow listener ?

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8744155 [details]
MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48335/diff/1-2/

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8744155 [details]
MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48335/diff/2-3/

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8744155 [details]
MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48335/diff/3-4/

Comment 10

[:Gijs (he/him)]

Comment on attachment 8744155 [details]
MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

https://reviewboard.mozilla.org/r/48335/#review45503

So... I feel really bad now - when I said I could take care of the review, I thought that iteration 1 that I f+'d looked OK apart from the double addEventListener call I commented on. But now there's the mobile addition as well, which I hadn't realized.

Looking at it this way, we're duplicating code. AboutReader.jsm has access to the docShell and document state, so closeReaderMode could be implemented there instead, so we'd have the implementation just once. Could we have the closeReaderMode method live there, and avoid passing in the listener (keeping the same method of passing the promise as we have now? Or is there a reason we're nulling it out on the listener as well (see below) or a subtle difference in the implementation in mobile vs. desktop that I've missed?

::: toolkit/components/reader/AboutReader.jsm:50
(Diff revision 4)
>  
> -  if (articlePromise) {
> -    this._articlePromise = articlePromise;
> +  this._listener = listener;
> +
> +  if (listener.articlePromise) {
> +    this._articlePromise = listener.articlePromise;
> +    listener.articlePromise = null;

Sorry, I should have thought about this before. Why are we nulling it out here?

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The problem is, AboutReaderListener does not keep AboutReader instances, so when it receives Reader:ToggleReaderMode message, it would not be able to call the specific AboutReader#closeReaderMode().

Are you suggesting I should implement a static method AboutReader.closeReaderMode(docShell, global) in AboutReader.jsm?

Comment 12

[:Gijs (he/him)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #11)
> The problem is, AboutReaderListener does not keep AboutReader instances, so
> when it receives Reader:ToggleReaderMode message, it would not be able to
> call the specific AboutReader#closeReaderMode().

Ugh, I'd missed that. Something about not seeing the trees for the forest. Sorry.

> Are you suggesting I should implement a static method
> AboutReader.closeReaderMode(docShell, global) in AboutReader.jsm?

I hadn't thought about it, but that would definitely be one solution. Another alternative would be to make AboutReader.jsm listen for the ToggleReaderMode message itself. Depends how ugly we think it is to handle one part (closing) in AboutReader.jsm and one part (opening) in the desktop/android content scripts... I could live with either of these solutions - do what you think makes the most sense. :-)

Comment 13

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #12)
> I hadn't thought about it, but that would definitely be one solution.
> Another alternative would be to make AboutReader.jsm listen for the
> ToggleReaderMode message itself. Depends how ugly we think it is to handle
> one part (closing) in AboutReader.jsm and one part (opening) in the
> desktop/android content scripts... I could live with either of these
> solutions - do what you think makes the most sense. :-)

Hm, though in fact, we could push the opening part into AboutReader.jsm as well, if we wanted to do that... Could leave unification to bug 1153485 as the android comment suggests?

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to :Gijs Kruitbosch from comment #13)
> Hm, though in fact, we could push the opening part into AboutReader.jsm as
> well, if we wanted to do that... Could leave unification to bug 1153485 as
> the android comment suggests?

I don't think we should move the open part into AboutReader.jsm too. That implies defineLazyModuleGetter always loads and run the module (for the opening listener) even if the user don't use reader mode for the entire life time of the process.

Also, the user should be able to turn off the reader mode even before the "AboutReaderContentLoaded" event and the existance of AboutReader instance. So, even though a static method is ugly and strange, it's probably the safest approach.

Comment 15

[:Gijs (he/him)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > Hm, though in fact, we could push the opening part into AboutReader.jsm as
> > well, if we wanted to do that... Could leave unification to bug 1153485 as
> > the android comment suggests?
> 
> I don't think we should move the open part into AboutReader.jsm too. That
> implies defineLazyModuleGetter always loads and run the module (for the
> opening listener) even if the user don't use reader mode for the entire life
> time of the process.

We could potentially add a separate JSM if we're concerned about this, but fair enough to not want to open that can of worms in this bug.

> Also, the user should be able to turn off the reader mode even before the
> "AboutReaderContentLoaded" event and the existance of AboutReader instance.
> So, even though a static method is ugly and strange, it's probably the
> safest approach.

Fair!

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8744155 [details]
MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48335/diff/4-5/

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://reviewboard.mozilla.org/r/48335/#review45745

::: browser/base/content/tab-content.js:266
(Diff revision 5)
>          let url = content.document.location.href;
>          if (!this.isAboutReader) {
>            this._articlePromise = ReaderMode.parseDocument(content.document).catch(Cu.reportError);
> -          content.document.location = "about:reader?url=" + encodeURIComponent(url);
> +          ReaderMode.enterReaderMode(docShell, content);
>          } else {
> -          let originalURL = ReaderMode.getOriginalUrl(url);
> +          ReaderMode.leaveReaderMode(docShell, content);

ReaderMode is the better place to put the functions, since it's already a singleton, util-like object.

::: toolkit/components/reader/ReaderMode.jsm:94
(Diff revision 5)
>  
>    /**
> +   * Enter the reader mode by going forward one step in history if applicable,
> +   * if not, append the about:reader page in the history instead.
> +   */
> +  enterReaderMode: function(docShell, win) {

Also implements forward history check per the original patch in bug 1184950 comment 1

Comment 18

[:Gijs (he/him)]

Comment on attachment 8744155 [details]
MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

https://reviewboard.mozilla.org/r/48335/#review45791

Nice, thanks!

Comment 19

[Pulsebot]

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

Comment 20

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

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

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

need uplift

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8744155 [details]
MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

Approval Request Comment
[Feature/regressing bug #]: bug 1184950
[User impact if declined]: Inconsistent behavior between the back button on Reader mode page and the same button on address bar
[Describe test coverage new/current, TreeHerder]: Existing tests covers the address bar button. Manually tested the close button on the page.
[Risks and why]: Risk should be slow and confined in the reader mode itself.
[String/UUID change made/needed]: None.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Arni2033, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8744155 [details]
MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

Recent regression in Fx48, Aurora48+

Comment 25

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/13fda2d4925e

Comment 26

[arni2033]

Fixed on:   Win7_64, Nightly 49, 32bit, ID 20160504043118

I checked it with the article in comment 0, and articles on russian and english versions of Wikipedia