If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
Reader Mode
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: 684sigma, Assigned: timdream)

Tracking

({regression})

50 Branch
mozilla55
regression
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 wontfix, firefox52- wontfix, firefox-esr52 wontfix, firefox53+ fixed, firefox54+ fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

7 months ago
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

7 months ago
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.
Blocks: 1260276
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
Ever confirmed: true
Flags: needinfo?(timdream)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Version: 52 Branch → 50 Branch

Comment 2

7 months ago
Too late for 51 and 52 will be released this week. Mark 51 won't fix.
status-firefox51: affected → wontfix
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.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
Oh wait, I can reproduce it now. Sorry about the confusion.
Comment hidden (mozreview-request)

Comment 6

7 months ago
mozreview-review
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
Attachment #8843870 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 7

7 months ago
mozreview-review
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 :)
Attachment #8843870 - Flags: review?(jaws)
Fix might not be sufficient since the test did not pass on Try.
I can reproduce that on my Linux desktop.
Interestingly, I can only reproduce this on Linux desktop with Einstein@Home grabbing GPU in the background :P.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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
Keywords: checkin-needed

Comment 14

7 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7689bfb22d7
Tell parent the non-reader-able reader page is not readable, r=Gijs
Keywords: checkin-needed

Comment 15

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7689bfb22d7
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Track 54+ as regression.

Hi Tim,
Since this also affects 54, do you think the patch is worth uplifting to 54 at least?
tracking-firefox54: ? → +
Flags: needinfo?(timdream)
wontfix for 52.
status-firefox52: affected → wontfix
tracking-firefox52: ? → -
status-firefox-esr52: --- → affected
Keeping ni -- let's get this sit on Nightly for a few more weeks before decide if we want to uplift it.

Updated

6 months ago
tracking-firefox53: ? → +
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.
Flags: needinfo?(timdream)
Attachment #8843870 - Flags: approval-mozilla-beta?
Attachment #8843870 - Flags: approval-mozilla-aurora?
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+.
Attachment #8843870 - Flags: approval-mozilla-beta?
Attachment #8843870 - Flags: approval-mozilla-beta+
Attachment #8843870 - Flags: approval-mozilla-aurora?
Attachment #8843870 - Flags: approval-mozilla-aurora+

Comment 21

6 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/55b98dce58cb
status-firefox54: affected → fixed

Comment 22

6 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/4153103b32cc
status-firefox53: affected → fixed
Setting qe-verify- based on comment 19.
Flags: qe-verify-
This seems like enough of an edge case to me to warrant wontfix for ESR52. What do you think, Tim?
Flags: needinfo?(timdream)
I agree. Let's won't fix this on ESR52.
Flags: needinfo?(timdream)
status-firefox-esr52: affected → wontfix
You need to log in before you can comment on or make changes to this bug.