pushstate for reader mode should reset browser.isArticle to false if the new page isn't readerable

VERIFIED FIXED in Firefox 38.0.5

Status

()

Firefox
General
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

38 Branch
Firefox 41
Points:
3
Bug Flags:
firefox-backlog +
in-testsuite -

Firefox Tracking Flags

(firefox38.0.5 verified, firefox39 verified, firefox40 fixed, firefox41 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment, 1 obsolete attachment)

This was originally: https://github.com/mozilla/readability/issues/227

Reproducible on:
Beta 38.0.5 beta 3 - build1 (20150518141916)
Affected platforms: Windows 7 x86, Windows 8.1 x64, Mac OS X 10.9.5 and Ubuntu 14.04 x64

Steps to reproduce:
1. Navigate to http://espn.go.com/
2. Click on any article
3. Hit Back button
Actual result: Enter Reader View button is visible in URL bar for espn.com homepage
4. Click on Enter Reader View button
Actual result: 'Failed to load article from page'

Sample pages showing this issue:

    http://www.usatoday.com/
    https://www.youtube.com/
    http://espn.go.com/



(this also affects nightly)
Flags: qe-verify+
Flags: in-testsuite-
Flags: firefox-backlog+
Created attachment 8608161 [details]
MozReview Request: bz://1166771/Gijs

/r/9127 - Bug 1166771 - force isArticle to false on pushstate on non-article pages, r?margaret

Pull down this commit:

hg pull -r dc54fbcffb8da01ac19f7204239d6ea3bebc947e https://reviewboard-hg.mozilla.org/gecko/
Attachment #8608161 - Flags: review?(margaret.leibovic)

Comment 2

3 years ago
Comment on attachment 8608161 [details]
MozReview Request: bz://1166771/Gijs

https://reviewboard.mozilla.org/r/9125/#review7761

Ship It!
Attachment #8608161 - Flags: review?(margaret.leibovic) → review+

Comment 3

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/bd4b92eb955d
Comment on attachment 8608161 [details]
MozReview Request: bz://1166771/Gijs

Approval Request Comment
[Feature/regressing bug #]: reader mode perf adjustments
[User impact if declined]: reader mode icon sticks around when it shouldn't
[Describe test coverage new/current, TreeHerder]: nope
[Risks and why]: low. This should only adjust the currently missed case where a page change doesn't cause a pagehide event but only a pushstate event where the new state of the doc doesn't need reader mode, whereas the old state of the doc *did*, which then causes the button to mistakenly hang around.
[String/UUID change made/needed]: nope

Dolske/Margaret, would love opinions about whether you think taking this for 38RC (gtb tomorrow!) is too risky or not. Re-reading the patch and the risk description above (which I think is accurate!) I think it's non-risky enough, but I appreciate it's late in the cycle.
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(dolske)
Attachment #8608161 - Flags: approval-mozilla-release?
Attachment #8608161 - Flags: approval-mozilla-beta?
Attachment #8608161 - Flags: approval-mozilla-aurora?

Comment 5

3 years ago
(In reply to :Gijs Kruitbosch from comment #4)

> Dolske/Margaret, would love opinions about whether you think taking this for
> 38RC (gtb tomorrow!) is too risky or not. Re-reading the patch and the risk
> description above (which I think is accurate!) I think it's non-risky
> enough, but I appreciate it's late in the cycle.

I think it is low-risk. Looking at the logic, in the worst-case scenario, we just end up sending a message to hide the reader button when we otherwise wouldn't send that message.

That being said, I think we should make sure to bang on a build to make sure that nothing unexpected happens, because even low-risk patches can sometimes contain surprises.
Flags: needinfo?(margaret.leibovic)
https://hg.mozilla.org/mozilla-central/rev/bd4b92eb955d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
status-firefox38.0.5: --- → affected
status-firefox39: --- → affected
status-firefox40: --- → affected
Comment on attachment 8608161 [details]
MozReview Request: bz://1166771/Gijs

It is a new feature, affect high-profile sites, should be safe. Taking it. Hopefully, it won't break everything :)
Attachment #8608161 - Flags: approval-mozilla-release?
Attachment #8608161 - Flags: approval-mozilla-release+
Attachment #8608161 - Flags: approval-mozilla-beta?
Attachment #8608161 - Flags: approval-mozilla-beta+
Attachment #8608161 - Flags: approval-mozilla-aurora?
Attachment #8608161 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/62af7310ed73
status-firefox40: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/bc222d611a7c
status-firefox39: affected → fixed
https://hg.mozilla.org/releases/mozilla-release/rev/17169e355c59
status-firefox38.0.5: affected → fixed
Verified fixed on RC 38.0.5-build3 (20150521175336), using the sample pages from comment 0 along with several other similar pages. Further Regression testing will be conducted on this area during the validation of 38.0.5-build3.
Status: RESOLVED → VERIFIED
status-firefox38.0.5: fixed → verified
Depends on: 1168101
Flags: needinfo?(dolske)
No longer depends on: 1168101

Comment 12

2 years ago
Verified fixed using 39.0b1 build 2 (20150523155636), under the following platforms: Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit.
status-firefox39: fixed → verified
Removing qe-verify+ since verification on 38.0.5 Beta and 39 Beta should suffice.
Flags: qe-verify+
Comment on attachment 8608161 [details]
MozReview Request: bz://1166771/Gijs
Attachment #8608161 - Attachment is obsolete: true
Attachment #8620340 - Flags: review+
Created attachment 8620340 [details]
MozReview Request: Bug 1166771 - force isArticle to false on pushstate on non-article pages, r?margaret
You need to log in before you can comment on or make changes to this bug.