Saving Reader View page yields blank page

VERIFIED FIXED in Firefox 62

Status

()

defect
P1
normal
VERIFIED FIXED
Last year
Last year

People

(Reporter: erwinm, Assigned: vinoth)

Tracking

({regression})

60 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 wontfix, firefox61 wontfix, firefox62 verified, firefox63 verified)

Details

Attachments

(1 attachment)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704003137

Steps to reproduce:

I visited a page. I entered reader view. I saved the page. I then tried to open the saved page.


Actual results:

It was blank.


Expected results:

It should have included all the info from Reader View.
Component: Untriaged → Reader Mode
Product: Firefox → Toolkit
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Version: 61 Branch → 60 Branch
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0babcbf5ff1e263916684965996092fc7600364e&tochange=c47ba61ce442211227ccf909eee6101841e5aa94

Regressed by:
c47ba61ce442	Christoph Kerschbaumer — Bug 1436808: Apply Meta CSP to content privileged about: pages. r=gijs,freddyb


:ckerschb
Your patch seems to cause the regression, could you please look into this?
Flags: needinfo?(ckerschb)
Blocks: 1436808
(In reply to Alice0775 White from comment #1)
> Regression window:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=0babcbf5ff1e263916684965996092fc7600364e&tochange=c47b
> a61ce442211227ccf909eee6101841e5aa94
> 
> Regressed by:
> c47ba61ce442	Christoph Kerschbaumer — Bug 1436808: Apply Meta CSP to content
> privileged about: pages. r=gijs,freddyb
> 
> 
> :ckerschb
> Your patch seems to cause the regression, could you please look into this?

Vino, could you please take a look at this regression? Thanks!
Flags: needinfo?(ckerschb) → needinfo?(cegvinoth)
Assignee: nobody → cegvinoth
Flags: needinfo?(cegvinoth)
Comment on attachment 8998549 [details]
Bug 1480934 - Saving Reader View page yields blank page fixed by removing inline style

Previously AboutReader.jsm added few inline styles , which was blocked by the CSP. Hence blank page was displayed.
Now fixed this by removing inline styles and used class based styling here.
Attachment #8998549 - Flags: review?(ckerschb)
This is a regression, and it looks like it's being actively worked on, so P1'ing.
Priority: -- → P1
Comment on attachment 8998549 [details]
Bug 1480934 - Saving Reader View page yields blank page fixed by removing inline style

Christoph Kerschbaumer [:ckerschb back on 9/4/2018] has approved the revision.
Attachment #8998549 - Flags: review+
Comment on attachment 8998549 [details]
Bug 1480934 - Saving Reader View page yields blank page fixed by removing inline style

As mentioned in the review, please have Gijs sign off on those changes too. Thanks for fixing!
Attachment #8998549 - Flags: review?(ckerschb)
Comment on attachment 8998549 [details]
Bug 1480934 - Saving Reader View page yields blank page fixed by removing inline style

Hey Jared Wein,

Since :gijs is on vacation, Please let me know whether you can review the patch and let me know your comments on this.

Thanks.
Attachment #8998549 - Flags: review?(jaws)
Comment on attachment 8998549 [details]
Bug 1480934 - Saving Reader View page yields blank page fixed by removing inline style

Hey Jared Wein,
Now fixed the nit changes. Please kindly review the patch and let me know your comments.

Thanks.
Comment on attachment 8998549 [details]
Bug 1480934 - Saving Reader View page yields blank page fixed by removing inline style

Jared Wein [:jaws] (please needinfo? me) has approved the revision.
Attachment #8998549 - Flags: review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7531c106b59
Saving Reader View page yields blank page fixed by removing inline style r=jaws,ckerschb
Duplicate of this bug: 1472486
This was already on file as bug 1472486...

It looks like this bounced because we also need to change the mobile stylesheet ( https://searchfox.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css ), presumably it just need the same update as the non-mobile sheet is getting in this commit.

Trypush with that:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20d22ea7eb6919b2464584f61ced928b2f43ca0a
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44fc89e957b8
Saving Reader View page yields blank page fixed by removing inline style r=jaws,ckerschb
(In reply to :Gijs (he/him) from comment #14)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=20d22ea7eb6919b2464584f61ced928b2f43ca0a

This is green, so taking the liberty of pushing this to inbound for you with the trivial fix. :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/44fc89e957b881ba35211e313f85f7a62652f529
Flags: needinfo?(cegvinoth)
https://hg.mozilla.org/mozilla-central/rev/44fc89e957b8
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(In reply to :Gijs (he/him) from comment #16)
> (In reply to :Gijs (he/him) from comment #14)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=20d22ea7eb6919b2464584f61ced928b2f43ca0a
> 
> This is green, so taking the liberty of pushing this to inbound for you with
> the trivial fix. :-)
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 44fc89e957b881ba35211e313f85f7a62652f529

Thanks for the quick action and pushing it.
Looks like 62 was marked wontfix at some point - do you want this to ride the trains?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> Looks like 62 was marked wontfix at some point - do you want this to ride
> the trains?

I guess I can request uplift.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8998549 [details]
Bug 1480934 - Saving Reader View page yields blank page fixed by removing inline style

Approval Request Comment
[Feature/Bug causing the regression]: adding CSP to reader mode, bug 1436808
[User impact if declined]: can't save reader mode pages to disk and use them
[Is this code covered by automated tests?]: reader mode yes, saving the result to disk no.
[Has the fix been verified in Nightly?]: not yet.
[Needs manual test from QE? If yes, steps to reproduce]: see comment #0
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: not really
[Why is the change risky/not risky?]: fairly straightforward fix, now reviewed by 2 peers, JS only, mostly just mechanical updating of foo.style.display to foo.classList.add/remove.
[String changes made/needed]: no.
Attachment #8998549 - Flags: approval-mozilla-beta?
FWIW, I think originally I didn't think this was such an important usecase, but I guess now that 2 people have independently filed dupes, maybe my initial assessment wasn't correct. Apparently saving reader mode versions of things to disk is a thing people do...
Looks like a pretty mechanical change. Approved for 62.0b20.
Flags: qe-verify+
Attachment #8998549 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I reproduced the initial issue on Firefox 61.0.2.
Verified fixed on Windows 7 x64, Ubuntu 16.04 x64 and macOS 10.12.6 using latest Nightly 63.0a1 (2018-08-22) and the 62.0b20 taskcluster build from 2018-08-23.

Only one question here: I saw that after opening the saved page (in reader mode), the 3 buttons from the left ("Close Reader View", "Aa-Type controls", "Narrate") aren't functional. Is this the intended behaviour?
Flags: needinfo?(cegvinoth)
(In reply to Camelia Badau [:cbadau], Release Desktop QA from comment #25)
> I reproduced the initial issue on Firefox 61.0.2.
> Verified fixed on Windows 7 x64, Ubuntu 16.04 x64 and macOS 10.12.6 using
> latest Nightly 63.0a1 (2018-08-22) and the 62.0b20 taskcluster build from
> 2018-08-23.
> 
> Only one question here: I saw that after opening the saved page (in reader
> mode), the 3 buttons from the left ("Close Reader View", "Aa-Type controls",
> "Narrate") aren't functional. Is this the intended behaviour?

I'm pretty sure those have never worked after saving the page.
Thank you, Gijs!

Marking the issue as VERIFIED FIXED.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(cegvinoth)
You need to log in before you can comment on or make changes to this bug.