1480934 - Saving Reader View page yields blank page

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P1)

Description

[MarjaE]

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.

Comment 1

[Alice0775 White]

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?

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

(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!

Comment 3

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: Bug 1480934 - Saving Reader View page yields blank page fixed by removing inline style

Comment 4

[Vinothkumar Nagasayanan [:vinoth]]

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.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

This is a regression, and it looks like it's being actively worked on, so P1'ing.

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

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.

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

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!

Comment 8

[Vinothkumar Nagasayanan [:vinoth]]

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.

Comment 9

[Vinothkumar Nagasayanan [:vinoth]]

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 10

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

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7531c106b59
Saving Reader View page yields blank page fixed by removing inline style r=jaws,ckerschb

Comment 12

[Andreea Pavel [:apavel]]

Backed out for failing android at mobile/android/tests/browser/chrome/test_session_scroll_position.html

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b7531c106b5947faf3edd8a951003d406c6a54dd

Failure log: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b7531c106b5947faf3edd8a951003d406c6a54dd

Backout: https://hg.mozilla.org/integration/autoland/rev/3e66cf1dad95c1594448d82bcac82711f8ba155e

Comment 14

[:Gijs (he/him)]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44fc89e957b8
Saving Reader View page yields blank page fixed by removing inline style r=jaws,ckerschb

Comment 16

[:Gijs (he/him)]

(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

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/44fc89e957b8

Comment 18

[Vinothkumar Nagasayanan [:vinoth]]

(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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Looks like 62 was marked wontfix at some point - do you want this to ride the trains?

Comment 20

[:Gijs (he/him)]

(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.

Comment 21

[:Gijs (he/him)]

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.

Comment 22

[:Gijs (he/him)]

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...

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Looks like a pretty mechanical change. Approved for 62.0b20.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4a5ae6a7911d

Comment 25

[Camelia Badau [:cbadau], Desktop Test Engineering]

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?

Comment 26

[:Gijs (he/him)]

(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.

Comment 27

[Camelia Badau [:cbadau], Desktop Test Engineering]

Thank you, Gijs!

Marking the issue as VERIFIED FIXED.