1344431 - Regression Firefox displays reader button in location bar even if it's unavailable (sometimes)

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[684sigma]

I have a problem with Firefox Beta 52. It doesn't happen on Firefox ESR 45.
Sometimes when I load pending tab with an article opened in reader mode, the article is already deleted on the site. In this case, reader mode displays text "Failed to load article from page". When I exit reader mode, reader button is displayed in location bar, even though there's no article. It may be confusing after ~30 more minutes of using browser, when I go back to that tab and the area where the article was located is not visible, so I cannot determine "fake" reader mode button. As a user, I don't want to always spend extra time to scroll the page and check that article is there.
I can't create an online example of article disappearing, nor I want to disclose sites I visit, but I found one scenario that reproduces exactly what I see.

1. Open about:reader?url=http://example.org/
2. Click reader button in location bar

Result: browser exits reader mode, and reader button is displayed in location bar
Expected: no reader button, it's confusing

Comment 1

[Loic]

Reg range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=683bf5d3c1cdd76799d7da01d3a0cd2dbb42e8f9&tochange=676b7df970b6b2f9432e421f9151e80346ee42ab

Timothy Guan-tin Chien — Bug 1260276 - Replace the dead reader page in history when we are being redirected, r=gijs

Tim, could you check this regression, please.

Comment 2

[Gerry Chang [:gchang]]

Too late for 51 and 52 will be released this week. Mark 51 won't fix.

Comment 3

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

Thanks for filing. I couldn't reproduce locally so I suspect this is a timing issue. Will look at the changes and see what's going on.

Comment 4

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

Oh wait, I can reproduce it now. Sorry about the confusion.

Comment 5

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

Attachment: Bug 1344431 - Tell parent the non-reader-able reader page is not readable,

Although regression window testing pin this to bug 1260276, I believe
this is a regression from bug 1126967. Bug 1260276 just make it more
visible because we stop automatically redirect users to the original
page.

This patch fix the bug by checking if the current page is in readerable
state (i.e. not error state), and send the message accordingly.

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

Comment 6

[:Gijs (he/him)]

Comment on attachment 8843870 [details]
Bug 1344431 - Tell parent the non-reader-able reader page is not readable,

https://reviewboard.mozilla.org/r/117492/#review119194

::: toolkit/components/reader/test/browser_readerMode.js:110
(Diff revision 1)
> +  is_element_visible(readerButton, "Reader mode button is present on about:reader");
> +
> +  // Switch page back out of reader mode.
> +  promisePageShow = BrowserTestUtils.waitForContentEvent(newTab.linkedBrowser, "pageshow");
> +  readerButton.click();
> +info('CLICKED');

Nit: indentation

Comment 7

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

Comment on attachment 8843870 [details]
Bug 1344431 - Tell parent the non-reader-able reader page is not readable,

https://reviewboard.mozilla.org/r/117492/#review119230

Gijs' review is sufficient here :)

Comment 8

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

Fix might not be sufficient since the test did not pass on Try.

Comment 9

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

I can reproduce that on my Linux desktop.

Comment 10

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

Interestingly, I can only reproduce this on Linux desktop with Einstein@Home grabbing GPU in the background :P.

Comment 11

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

Comment on attachment 8843870 [details]
Bug 1344431 - Tell parent the non-reader-able reader page is not readable,

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

Comment 12

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

Comment on attachment 8843870 [details]
Bug 1344431 - Tell parent the non-reader-able reader page is not readable,

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

Comment 13

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

It turned out I would need to wait for the error page to render before switching away.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1889a93375a7974e61675853c4cd8aaa87cbbe5e
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cbcb313b616cfe209e5bb9ebd3922e6ecb15f44

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7689bfb22d7
Tell parent the non-reader-able reader page is not readable, r=Gijs

Comment 15

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

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

Comment 16

[Gerry Chang [:gchang]]

Track 54+ as regression.

Hi Tim,
Since this also affects 54, do you think the patch is worth uplifting to 54 at least?

Comment 17

[Julien Cristau [:jcristau]]

wontfix for 52.

Comment 18

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

Keeping ni -- let's get this sit on Nightly for a few more weeks before decide if we want to uplift it.

Comment 19

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

Comment on attachment 8843870 [details]
Bug 1344431 - Tell parent the non-reader-able reader page is not readable,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1260276
[User impact if declined]: See STR, Firefox is confused and will think non-read-able page is readable.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet, only manually verified before landing.
[Needs manual test from QE? If yes, steps to reproduce]: No need to manual test.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Shouldn't be.
[Why is the change risky/not risky?]: Logic is localized and only specific to the reader mode error page.
[String changes made/needed]: None.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8843870 [details]
Bug 1344431 - Tell parent the non-reader-able reader page is not readable,

Fix a reader mode regression that Firefox will think a non-readable page is readable. Aurora54+ & Beta53+.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/55b98dce58cb

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4153103b32cc

Comment 23

[Cornel Ionce [:noni] [Hubs QA]]

Setting qe-verify- based on comment 19.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

This seems like enough of an edge case to me to warrant wontfix for ESR52. What do you think, Tim?

Comment 25

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

I agree. Let's won't fix this on ESR52.