Closed Bug 1266372 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Reader Mode, defect)

48 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- verified

People

(Reporter: arni2033, Assigned: timdream)

References

Details

(Keywords: regression)

Attachments

(1 file)

>>>   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
Thanks for filing.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
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 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.
Attachment #8744155 - Flags: feedback?(margaret.leibovic)
I can't add bug 1264805 to dependency because that would be circular...
See Also: → 1264805
Comment on attachment 8744155 [details]
MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

Also flagging Gijs.
Attachment #8744155 - Flags: feedback?(gijskruitbosch+bugs)
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 ?
Attachment #8744155 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
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/
Attachment #8744155 - Attachment description: MozReview Request: Bug 1266372 - WIP, Call AboutReaderListener.closeReaderMode from AboutReader, f=margaret → MozReview Request: Bug 1266372 - Call AboutReaderListener.closeReaderMode from AboutReader, r=margaret, f=gijs
Attachment #8744155 - Flags: review?(margaret.leibovic)
Attachment #8744155 - Flags: feedback?(margaret.leibovic)
Attachment #8744155 - Flags: feedback+
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 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/
Attachment #8744155 - Attachment description: MozReview Request: Bug 1266372 - Call AboutReaderListener.closeReaderMode from AboutReader, r=margaret, f=gijs → MozReview Request: Bug 1266372 - Call AboutReaderListener.closeReaderMode from AboutReader, r=gijs
Attachment #8744155 - Flags: review?(margaret.leibovic) → review?(gijskruitbosch+bugs)
Attachment #8744155 - Flags: review?(gijskruitbosch+bugs)
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?
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?
Flags: needinfo?(gijskruitbosch+bugs)
(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. :-)
Flags: needinfo?(gijskruitbosch+bugs)
(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?
(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.
(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 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/
Attachment #8744155 - Attachment description: MozReview Request: Bug 1266372 - Call AboutReaderListener.closeReaderMode from AboutReader, r=gijs → MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs
Attachment #8744155 - Flags: review?(gijskruitbosch+bugs)
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 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!
Attachment #8744155 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/a965f8da8ed1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
need uplift
Flags: needinfo?(timdream)
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.
Flags: needinfo?(timdream)
Attachment #8744155 - Flags: approval-mozilla-aurora?
Arni2033, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(arni2033)
Comment on attachment 8744155 [details]
MozReview Request: Bug 1266372 - De-dup code to ReaderMode.leaveReaderMode, r=gijs

Recent regression in Fx48, Aurora48+
Attachment #8744155 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(arni2033)
Version: unspecified → 48 Branch
You need to log in before you can comment on or make changes to this bug.