1745158 - After exiting (x) Reader view and pressing the back-arrow you return again in "Reader view"

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P2)

Description

[danilo_fossati]

Steps to reproduce:

I enter Reader view for any web page; then I exit (pressing X) Reader view; then I press the back-arrow and that makes you wrongly return again in "Reader view".

Actual results:

I am again in Reader view by pressing back-arrow.

Expected results:

I should have returned to previous normal web page by pressing back-arrow.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Toolkit::Reader Mode' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Alice0775 White]

I can reproduce the issue if Fission is enabled in Nightly97.0a1 Windows10.

Comment 3

[Patrick (he/him)]

Regression range last good build 2020-10-02, first bad build 2020-10-03. Unable to bisect further in mozregression

Comment 4

[Alice0775 White]

from comment#3, the range is

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d70d61772f1aaace64f3df54d01d4d2057eecd78&tochange=33139800aca52aac192973fe24c9b74df7c87e00

Suspect: Bug 1668357

Comment 5

[Chris Peterson [:cpeterson]]

This is a regression from SHIP. I can reproduce this bug.

This is a bug in Reader Mode. This leaveReaderMode code should probably send a message to the AboutReader Actor in the parent process:

https://searchfox.org/mozilla-central/rev/21a9b72545da06681db97c4b3a2a6be761f4aae5/toolkit/components/reader/ReaderMode.jsm#118,141-151

Comment 6

[:Gijs (he/him)]

(In reply to Chris Peterson [:cpeterson] from comment #5)

This is a regression from SHIP. I can reproduce this bug.

This is a bug in Reader Mode. This leaveReaderMode code should probably send a message to the AboutReader Actor in the parent process:

https://searchfox.org/mozilla-central/rev/21a9b72545da06681db97c4b3a2a6be761f4aae5/toolkit/components/reader/ReaderMode.jsm#118,141-151

That code was patched by the DOM team for SHIP... is someone from the DOM team able to fix this (or at least advise in more detail on how to do so)? I'm not really sure what the parent actor should be doing - or really, why we need to talk to the parent. Can we no longer determine in the child whether we can go back, and instruct the browser to navigate back? The previous code ( https://searchfox.org/mozilla-central/rev/8822e43271108eebcd09f0bf55963752377a89ea/toolkit/components/reader/ReaderMode.jsm#122-130 ) was pretty straightforward - it checks if the previous URL was the one we entered reader mode for, and if so, it navigated back.

Comment 7

[Chris Peterson [:cpeterson]]

(In reply to :Gijs (he/him) from comment #6)

(In reply to Chris Peterson [:cpeterson] from comment #5)

This is a regression from SHIP. I can reproduce this bug.

This is a bug in Reader Mode. This leaveReaderMode code should probably send a message to the AboutReader Actor in the parent process:

https://searchfox.org/mozilla-central/rev/21a9b72545da06681db97c4b3a2a6be761f4aae5/toolkit/components/reader/ReaderMode.jsm#118,141-151

That code was patched by the DOM team for SHIP... is someone from the DOM team able to fix this (or at least advise in more detail on how to do so)? I'm not really sure what the parent actor should be doing - or really, why we need to talk to the parent. Can we no longer determine in the child whether we can go back, and instruct the browser to navigate back? The previous code ( https://searchfox.org/mozilla-central/rev/8822e43271108eebcd09f0bf55963752377a89ea/toolkit/components/reader/ReaderMode.jsm#122-130 ) was pretty straightforward - it checks if the previous URL was the one we entered reader mode for, and if so, it navigated back.

Olli, can you please describe the AboutReader fix or write the patch?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Session history isn't available in the child anymore.

farre, you patched some of ReaderMode.jsm for SHIP. Would have time to take a look at this.

Comment 9

[Andreas Farre [:farre]]

This is bug 1664982.

Session history isn't available in the child any more, which means that we can't get the URI of the previous location in history. Regardless of if that is forwards of backwards. It's fully possible to determine if you can go either back or forward with ChildSHistory.canGo, but that doesn't help us here.

So to be able to use the goBack/goForward cases for leaveReaderMode/enterReaderMode we do need to coordinate with the parent. Basically what needs to happen is that the parent have to decide before sending a message to the child if there is history that can be used, and do the history navigation from there, and don't navigate in the child at all.

Comment 10

[Andreas Farre [:farre]]

After a bit more digging I've found this. So we do indeed do the same thing for reader mode and fission. But it turns out we've forgotten the close case. So it isn't actually entirely bug 1664982. But close needs to send leave to parent and let parent coordinate closing.

Comment 11

[Andreas Farre [:farre]]

Attachment: Bug 1745158 - Always leave reader mode from the parent actor. r=Gijs!

Comment 12

[Andreas Farre [:farre]]

This patch fixes the close case, and makes close behave the same as toggle, i.e. the steps:

1. enter reader mode
2. close reader mode
3. go back

behaves the same as

1. toggle reader mode (enter)
2. toggle reader mode (leave)
3. go back

The bad thing is that it looks like reader mode is behaving strange when going back or forward into reader mode. Leaving reader mode then becomes a process of pressing close or toggle several times. I wonder if this is a session history issue or a reader mode issue. Olli, do you have an idea here?

In any case, I feel that the attached patch is good enough to close this issue if you find it ok Gijs, and we can handle the other issue in a new bug?

Comment 13

[:Gijs (he/him)]

(In reply to Andreas Farre [:farre] from comment #12)

This patch fixes the close case, and makes close behave the same as toggle, i.e. the steps:

1. enter reader mode
2. close reader mode
3. go back

behaves the same as

1. toggle reader mode (enter)
2. toggle reader mode (leave)
3. go back

The bad thing is that it looks like reader mode is behaving strange when going back or forward into reader mode. Leaving reader mode then becomes a process of pressing close or toggle several times. I wonder if this is a session history issue or a reader mode issue. Olli, do you have an idea here?

In any case, I feel that the attached patch is good enough to close this issue if you find it ok Gijs, and we can handle the other issue in a new bug?

So I'm a little confused. I don't think what I'm seeing matches this explanation. In particular, if I do this:

1. open https://www.bbc.co.uk/news/business-57425161
2. enter reader mode
3. click browser back button
4. click browser forward button

I can keep repeating steps 3/4 as often as I want and things seem to work normally.

If I use the toggle reader mode button, that's when things mess up.

This is a pretty recent regression (from the last 2 weeks!), but reader mode hasn't really changed. I'll look for a regression window.

Comment 14

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #13)

This is a pretty recent regression (from the last 2 weeks!), but reader mode hasn't really changed. I'll look for a regression window.

Oh, apparently this is a fission regression, and the initial mozregression run had it disabled until very recently. Sigh. Trying to find a window again...

Comment 15

[:Gijs (he/him)]

I filed bug 1747148 for the other issue.

I agree it can be dealt with separately, though it'd be nice to do that soon.

I'm kind of sad this all broke so extensively without being picked up by tests; looks like the test in https://searchfox.org/mozilla-central/source/toolkit/components/reader/test/browser_readerMode.js does test that toggling reader mode off goes back, and leaves canGoForward truthy, but not that actually using the forward button then produces the correct result, nor that using the "exit" button inside reader mode goes back to the main article page.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c38daf09ab50
Always leave reader mode from the parent actor. r=Gijs

Comment 17

[Iulian Moraru]

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

Comment 18

[Monica Chiorean]

I was able to reproduce the issue on Win10 using build 97.0a1 (20211209155024) and with fission.autostart='true'.
Verified as fixed on Win10/Ubuntu 20.4/ Mac 10.13 using Beta 97.0b7 (20220123185805) and 98.0a1 (20220124214229). On latest versions the fission.autostart is set to 'true' by default.