962433 - Use Reader Mode for printing articles

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

So, here's a crazy idea: Lets use Reader Mode to to provide a better printing experience.

The usual problem with printing articles (yes, people do this, and we have data to back this up) is that you end up printing a lot of extra crap from the webpage that you don't want printed. Most sites just don't provide the proper @media queries to hide all that cruft when printing. Reader Mode gives us a nice way to handle that.

Obviously, there needs to be a way to toggle this off. And, uh, we don't want dark mode.

Comment 1

[Eklavya Mirani]

Awesome! My dad will be happy with this!

Ouch, dark mode ;)

Though, our readability.js misses article images (or shows the author image instead of main article image)

Comment 2

[Daniel Holbert [:dholbert]]

[mlongaray expressed interest in working on this bug in IRC. I've assigned it to him, so that he can post patches, and I've CC'd mconley, since I suspect he might be a good reviewer both from a frontend-code-knowledge & printing-knowledge perspective.]

Comment 3

[Matheus Longaray (:mlongaray)]

Attachment: WIP1: Add checkbox to change print preview layout for easier reading.

This patch adds a new checkbox on print preview UI that allows the user to change the layout for easier reading. It uses reader-mode infrastructure internally.

Comment 4

[Matheus Longaray (:mlongaray)]

Thanks for doing this, Daniel. I've CC'd philipp because we've been talking through e-mail for a couple of weeks now. He has tested and reviewed couple of builds, and also gave some useful feedback to it.

Comment 5

[Matheus Longaray (:mlongaray)]

Attachment: WIP2: Small tooltip refactor and checkbox disabling

Comment 6

[Christopher Arnold]

Thanks for this Matheus. Can you attach screenshots of the output you achieve with this patch?

Comment 7

[Matheus Longaray (:mlongaray)]

I sure can, Christopher. I'll grab those for you and also attach a few more examples to provide broader understanding.

Comment 8

[Matheus Longaray (:mlongaray)]

Attachment: Scenario 1: Home page tab

Comment 9

[Matheus Longaray (:mlongaray)]

Attachment: Scenario 1: Home page tab when print previewing

See tooltip of simplify page checkbox.

Comment 10

[Matheus Longaray (:mlongaray)]

Attachment: Scenario 2: Techcrunch page used for testing

Comment 11

[Matheus Longaray (:mlongaray)]

Attachment: Scenario 2: Techcrunch page when print previewing

See tooltip of simplify page checkbox

Comment 12

[Matheus Longaray (:mlongaray)]

Attachment: Scenario 2: Techcrunch page print preview after changing layout

Comment 13

[Matheus Longaray (:mlongaray)]

Attachment: Scenario 3: Techcrunch page under reader mode

Comment 14

[Matheus Longaray (:mlongaray)]

Attachment: Scenario 3: Techcrunch page when printing from reader mode

See tooltip of simplify page checkbox

Comment 15

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

Comment on attachment 8725760 [details] [diff] [review]
WIP2: Small tooltip refactor and checkbox disabling

Review of attachment 8725760 [details] [diff] [review]:
-----------------------------------------------------------------

This is a great proof of concept, and I think this is a cool feature.

I have some suggestions about code organization, however. For one thing, I suggest that instead of opening two preview tabs (one "simplified" and one "normal"), that we use the same preview tab we always have, but when the user chooses to "simplify" the print preview, that we send a message down to the content process which switches the document out of print preview mode, and then enters it into reader mode.

Here's the code that listens to messages from the parent - this is what can enter us and exit us from print preview: https://dxr.mozilla.org/mozilla-central/rev/eb25b90a05c194bfd4f498ff3ffee7440f85f1cd/toolkit/content/browser-content.js#364

Here's the code that runs in the parent that does the messaging: https://dxr.mozilla.org/mozilla-central/rev/eb25b90a05c194bfd4f498ff3ffee7440f85f1cd/toolkit/components/printing/content/printUtils.js#8

Here's the code that runs in the content process to enter us into ReaderMode when the user triggers it from the icon in the URL bar: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tab-content.js#275

So here's what I envision:

The user opens print preview, and the content process reads the pref (since it has access to the Preferences service) to determine whether or not to default to ReaderMode. It goes into either ReaderMode or normal print preview as its initial state. When the user hits the checkbox in the Print Preview toolbar, the pref gets flipped to the other state. The content process needs to add a preference observer, since it will be notified when the parent process updates a preference. When that occurs, it will flip to the other state.

I also think we can get rid of a lot of the stuff that's been added here to add the pref to nsIPrintSettings / nsPrintOptions, etc. I think just a raw pref is fine here.

Does that make sense? Let me know if any of that is unclear, and I'll happily elaborate.

Comment 16

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #15)
> Comment on attachment 8725760 [details] [diff] [review]
> WIP2: Small tooltip refactor and checkbox disabling
> 
> Review of attachment 8725760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a great proof of concept, and I think this is a cool feature.
> 
> I have some suggestions about code organization, however. For one thing, I
> suggest that instead of opening two preview tabs (one "simplified" and one
> "normal"), that we use the same preview tab we always have, but when the
> user chooses to "simplify" the print preview, that we send a message down to
> the content process which switches the document out of print preview mode,
> and then enters it into reader mode.
> 
> Here's the code that listens to messages from the parent - this is what can
> enter us and exit us from print preview:
> https://dxr.mozilla.org/mozilla-central/rev/
> eb25b90a05c194bfd4f498ff3ffee7440f85f1cd/toolkit/content/browser-content.
> js#364
> 
> Here's the code that runs in the parent that does the messaging:
> https://dxr.mozilla.org/mozilla-central/rev/
> eb25b90a05c194bfd4f498ff3ffee7440f85f1cd/toolkit/components/printing/content/
> printUtils.js#8
> 
> Here's the code that runs in the content process to enter us into ReaderMode
> when the user triggers it from the icon in the URL bar:
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tab-
> content.js#275
> 
> So here's what I envision:
> 
> The user opens print preview, and the content process reads the pref (since
> it has access to the Preferences service) to determine whether or not to
> default to ReaderMode. It goes into either ReaderMode or normal print
> preview as its initial state. When the user hits the checkbox in the Print
> Preview toolbar, the pref gets flipped to the other state. The content
> process needs to add a preference observer, since it will be notified when
> the parent process updates a preference. When that occurs, it will flip to
> the other state.
> 
> I also think we can get rid of a lot of the stuff that's been added here to
> add the pref to nsIPrintSettings / nsPrintOptions, etc. I think just a raw
> pref is fine here.
> 
> Does that make sense? Let me know if any of that is unclear, and I'll
> happily elaborate.

Thanks for the quick feedback, Mike! I really appreciate. I’ll start investigating how to better elaborate this refactor.

A couple of things are still unclear to me however. It would be great if you could elaborate more on the following questions I have.

From what you envisioned, when the user opens print preview and the content process reads the pref service, we would still have to check if the current tab is an article or not (reader-mode-able), otherwise we would not get a simplified version of it (since triggering reader mode for non-readable pages, the tab gets redirected to the original url/page). With that, would we change the users pref (uncheck simplify page checkbox)?

When exiting from preview, should we worry about the current state of the document? I suppose we would need to send another message down to the content process in order to switch to its original state. Is that correct?

I believe we would gain performance efficiency refactoring it. Is that also correct?

Thanks again for your time. Let’s ship it :)

Comment 17

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

(In reply to Matheus Longaray from comment #16)
> 
> From what you envisioned, when the user opens print preview and the content
> process reads the pref service, we would still have to check if the current
> tab is an article or not (reader-mode-able), otherwise we would not get a
> simplified version of it (since triggering reader mode for non-readable
> pages, the tab gets redirected to the original url/page). With that, would
> we change the users pref (uncheck simplify page checkbox)?

That's a good point. I failed to account for the possibility that the user is looking at a page that ReaderMode can't render properly.

Suppose we're looking at such a page. There are two cases: the user has Simplify enabled by default, or not.

Let's suppose that by default, when print preview is opened, the checkbox for enabling simplified mode is disabled (since we cannot know until the page is loaded whether or not we can use ReaderMode).

If the user has it enabled by default, we load the page, and try to get ReaderMode to consume it. When it rejects, we flip to print preview, and tell the parent. The parent will then uncheck and disable the checkbox (perhaps also putting a tooltip on it to indicate why it's disabled).

If the user has it disabled by default, we show print preview. We also wait for ReaderMode to consume it. When it rejects, we send a message to the parent that will perhaps update the tooltip on the checkbox to indicate that simplified mode will not work on this page.

One thing to consider is that the original browser that has the page will only allow entering print preview will also be checking (or will have already checked) to see if the page can be consumed by ReaderMode. We should take advantage of that instead of re-processing the page.

> 
> When exiting from preview, should we worry about the current state of the
> document? I suppose we would need to send another message down to the
> content process in order to switch to its original state. Is that correct?

When we exit print preview, the browser that print preview is displayed in is destroyed, so we don't have to worry about it.

> 
> I believe we would gain performance efficiency refactoring it. Is that also
> correct?

Yes, I believe so. Currently, you're opening two tabs - one with reader mode, and the other with normal print preview. My suggestion is to have a single browser do the work.

> 
> Thanks again for your time. Let’s ship it :)

Thanks for working on this!

Comment 18

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #17)
> (In reply to Matheus Longaray from comment #16)
> > 
> > From what you envisioned, when the user opens print preview and the content
> > process reads the pref service, we would still have to check if the current
> > tab is an article or not (reader-mode-able), otherwise we would not get a
> > simplified version of it (since triggering reader mode for non-readable
> > pages, the tab gets redirected to the original url/page). With that, would
> > we change the users pref (uncheck simplify page checkbox)?
> 
> That's a good point. I failed to account for the possibility that the user
> is looking at a page that ReaderMode can't render properly.
> 
> Suppose we're looking at such a page. There are two cases: the user has
> Simplify enabled by default, or not.
> 
> Let's suppose that by default, when print preview is opened, the checkbox
> for enabling simplified mode is disabled (since we cannot know until the
> page is loaded whether or not we can use ReaderMode).
> 
> If the user has it enabled by default, we load the page, and try to get
> ReaderMode to consume it. When it rejects, we flip to print preview, and
> tell the parent. The parent will then uncheck and disable the checkbox
> (perhaps also putting a tooltip on it to indicate why it's disabled).
> 
> If the user has it disabled by default, we show print preview. We also wait
> for ReaderMode to consume it. When it rejects, we send a message to the
> parent that will perhaps update the tooltip on the checkbox to indicate that
> simplified mode will not work on this page.
> 
> One thing to consider is that the original browser that has the page will
> only allow entering print preview will also be checking (or will have
> already checked) to see if the page can be consumed by ReaderMode. We should
> take advantage of that instead of re-processing the page.
> 

Quick thought: say it's an article page (reader-mode-able) and we enter on print preview and we've got an unchecked & disabled simplify checkbox. Then, content process tells the parent process to enable it. Once the user clicks on the checkbox, we flip to reader mode on the content process and show it on the preview. If the user clicks again, we will have to flip back to preview mode again.

What I wonder is, when you exit reader mode, when you flip back to the original page/browser, the tab gets reloaded (w/ all network requests). If that’s correct, would that be a problem for us?

> > 
> > When exiting from preview, should we worry about the current state of the
> > document? I suppose we would need to send another message down to the
> > content process in order to switch to its original state. Is that correct?
> 
> When we exit print preview, the browser that print preview is displayed in
> is destroyed, so we don't have to worry about it.
> 
> > 
> > I believe we would gain performance efficiency refactoring it. Is that also
> > correct?
> 
> Yes, I believe so. Currently, you're opening two tabs - one with reader
> mode, and the other with normal print preview. My suggestion is to have a
> single browser do the work.
> 
> > 
> > Thanks again for your time. Let’s ship it :)
> 
> Thanks for working on this!

Comment 19

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

(In reply to Matheus Longaray from comment #18)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #17)
> > (In reply to Matheus Longaray from comment #16)
> > > 
> > > From what you envisioned, when the user opens print preview and the content
> > > process reads the pref service, we would still have to check if the current
> > > tab is an article or not (reader-mode-able), otherwise we would not get a
> > > simplified version of it (since triggering reader mode for non-readable
> > > pages, the tab gets redirected to the original url/page). With that, would
> > > we change the users pref (uncheck simplify page checkbox)?
> > 
> > That's a good point. I failed to account for the possibility that the user
> > is looking at a page that ReaderMode can't render properly.
> > 
> > Suppose we're looking at such a page. There are two cases: the user has
> > Simplify enabled by default, or not.
> > 
> > Let's suppose that by default, when print preview is opened, the checkbox
> > for enabling simplified mode is disabled (since we cannot know until the
> > page is loaded whether or not we can use ReaderMode).
> > 
> > If the user has it enabled by default, we load the page, and try to get
> > ReaderMode to consume it. When it rejects, we flip to print preview, and
> > tell the parent. The parent will then uncheck and disable the checkbox
> > (perhaps also putting a tooltip on it to indicate why it's disabled).
> > 
> > If the user has it disabled by default, we show print preview. We also wait
> > for ReaderMode to consume it. When it rejects, we send a message to the
> > parent that will perhaps update the tooltip on the checkbox to indicate that
> > simplified mode will not work on this page.
> > 
> > One thing to consider is that the original browser that has the page will
> > only allow entering print preview will also be checking (or will have
> > already checked) to see if the page can be consumed by ReaderMode. We should
> > take advantage of that instead of re-processing the page.
> > 
> 
> Quick thought: say it's an article page (reader-mode-able) and we enter on
> print preview and we've got an unchecked & disabled simplify checkbox. Then,
> content process tells the parent process to enable it. Once the user clicks
> on the checkbox, we flip to reader mode on the content process and show it
> on the preview. If the user clicks again, we will have to flip back to
> preview mode again.
> 
> What I wonder is, when you exit reader mode, when you flip back to the
> original page/browser, the tab gets reloaded (w/ all network requests). If
> that’s correct, would that be a problem for us?
> 

Yeah, that's not amazing. I can confirm that when I exit Reader Mode, I hit the network again and re-retrieve the page. I'm actually surprised Reader Mode does this - I wonder why we can't just retrieve the page off the network cache like we do for things like View Source or Save Page As.

Hey margaret, I think you worked on Reader Mode - do you know if re-downloading the page when exiting Reader Mode was an intentional decision?

Comment 20

[:Margaret Leibovic]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #19)
> (In reply to Matheus Longaray from comment #18)
> > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #17)
> > > (In reply to Matheus Longaray from comment #16)
> > > > 
> > > > From what you envisioned, when the user opens print preview and the content
> > > > process reads the pref service, we would still have to check if the current
> > > > tab is an article or not (reader-mode-able), otherwise we would not get a
> > > > simplified version of it (since triggering reader mode for non-readable
> > > > pages, the tab gets redirected to the original url/page). With that, would
> > > > we change the users pref (uncheck simplify page checkbox)?
> > > 
> > > That's a good point. I failed to account for the possibility that the user
> > > is looking at a page that ReaderMode can't render properly.
> > > 
> > > Suppose we're looking at such a page. There are two cases: the user has
> > > Simplify enabled by default, or not.
> > > 
> > > Let's suppose that by default, when print preview is opened, the checkbox
> > > for enabling simplified mode is disabled (since we cannot know until the
> > > page is loaded whether or not we can use ReaderMode).
> > > 
> > > If the user has it enabled by default, we load the page, and try to get
> > > ReaderMode to consume it. When it rejects, we flip to print preview, and
> > > tell the parent. The parent will then uncheck and disable the checkbox
> > > (perhaps also putting a tooltip on it to indicate why it's disabled).
> > > 
> > > If the user has it disabled by default, we show print preview. We also wait
> > > for ReaderMode to consume it. When it rejects, we send a message to the
> > > parent that will perhaps update the tooltip on the checkbox to indicate that
> > > simplified mode will not work on this page.
> > > 
> > > One thing to consider is that the original browser that has the page will
> > > only allow entering print preview will also be checking (or will have
> > > already checked) to see if the page can be consumed by ReaderMode. We should
> > > take advantage of that instead of re-processing the page.
> > > 
> > 
> > Quick thought: say it's an article page (reader-mode-able) and we enter on
> > print preview and we've got an unchecked & disabled simplify checkbox. Then,
> > content process tells the parent process to enable it. Once the user clicks
> > on the checkbox, we flip to reader mode on the content process and show it
> > on the preview. If the user clicks again, we will have to flip back to
> > preview mode again.
> > 
> > What I wonder is, when you exit reader mode, when you flip back to the
> > original page/browser, the tab gets reloaded (w/ all network requests). If
> > that’s correct, would that be a problem for us?
> > 
> 
> Yeah, that's not amazing. I can confirm that when I exit Reader Mode, I hit
> the network again and re-retrieve the page. I'm actually surprised Reader
> Mode does this - I wonder why we can't just retrieve the page off the
> network cache like we do for things like View Source or Save Page As.
> 
> Hey margaret, I think you worked on Reader Mode - do you know if
> re-downloading the page when exiting Reader Mode was an intentional decision?

Not intentional, mostly a side effect of wanting reader mode to not depend on an original DOM having been loaded first. Because of the way reading list was implemented, we wanted the ability to directly load a reader view page if you opened a reading list item, so when you load about:reader?url=..., we download the response from that URL and immediately parse it with Readability. In that case, when you exit reader view, we don't have an original DOM to fall back to.

We could be smarter and restore a cached version of the DOM if we have it, it just requires work to fix it :)

Comment 21

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

Okay, thanks margaret.

So I think the "hitting the network again" problem is a Reader Mode problem in general, Matheus, and will probably need to be fixed before this feature ships (since I would imagine a number of things that people would want to print, like tickets for things, might not be friendly for re-retrieval off the network).

Comment 22

[Matheus Longaray (:mlongaray)]

Attachment: WIP3: Partial Refactor

As Mike Conley asked, here's a partial WIP of requested refactor. We've discussing some ideas through IRC but we're still seeing some funky behavior. Got same results on both OS X and Linux.

REM: perhaps make sure e10s is disabled when testing (https://bugzil.la/1257993).

Comment 23

[Matheus Longaray (:mlongaray)]

As mconley's requested.

STR:

1) Apply WIP3 diff file.
2) After importing diff, feel free to add "this._listener.onExit();" at "toolkit/components/printing/content/printUtils.js#509" so it becomes easier to visualize what happens on preview tab.
3) Notice that reader mode does not get painted on preview tab.

REM:
* Make sure e10s is disabled when testing (https://bugzilla.mozilla.org/show_bug.cgi?id=1257993).

Resource:
* https://s3.amazonaws.com/uploads.hipchat.com/167767/2106681/uwR3TuJUb6Z0xj8/Bug-962433-WIP3-ScreenRecording.mov

Comment 24

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

Attachment: PoC reader mode in print preview browser

MozReview-Commit-ID: F0BztDWMJUn

Comment 25

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

Comment on attachment 8734478 [details] [diff] [review]
WIP3: Partial Refactor

Hey Matheus,

I had some difficulty in reviewing your patch - I think there were a lot of leftovers from your original patch, and it was kind of hard to disentangle it.

I hope it's okay, but I took a stab at a proof-of-concept that you might be able to use as a basic framework to get you on your way. See attached.

Comment 26

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #25)
> Comment on attachment 8734478 [details] [diff] [review]
> WIP3: Partial Refactor
> 
> Hey Matheus,
> 
> I had some difficulty in reviewing your patch - I think there were a lot of
> leftovers from your original patch, and it was kind of hard to disentangle
> it.
> 
> I hope it's okay, but I took a stab at a proof-of-concept that you might be
> able to use as a basic framework to get you on your way. See attached.

Hi Mike,

Thank you for taking time looking through this one. It's absolutely okay that you wrote this basic framework for us. It helps us to sync up on things. I'll comment on the patch itself some points I already have to discuss.

Comment 27

[Matheus Longaray (:mlongaray)]

Attachment: PoC: scenario 1

Comment 28

[Matheus Longaray (:mlongaray)]

Attachment: PoC: scenario 2

Comment 29

[Matheus Longaray (:mlongaray)]

Comment on attachment 8737422 [details] [diff] [review]
PoC reader mode in print preview browser

Review of attachment 8737422 [details] [diff] [review]:
-----------------------------------------------------------------

For better understanding, please see attachments when reading feedback.

::: toolkit/content/browser-content.js
@@ +446,5 @@
> +
> +      let webNav = docShell.QueryInterface(Ci.nsIWebNavigation);
> +      let loadFlags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
> +      let referrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT;
> +      let uri = "about:reader?url=" + contentWindow.location.href;

Only flipping to reader mode is enough in our case? Would UX team be happy with it? I thought we wanted to generate the pdf for reader mode while taking into account all print settings. I attached a screenshot with the current PoC behavior (see file PoC: scenario 1).

@@ +448,5 @@
> +      let loadFlags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
> +      let referrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT;
> +      let uri = "about:reader?url=" + contentWindow.location.href;
> +      webNav.loadURIWithOptions(uri, loadFlags, null, referrerPolicy,
> +                                null, null, null);

I went ahead and added the following code to get the content window and pass it to enterPrintPreview() so it could generate the pdf for the browser. Please mind setTimeout() was only used to make sure reader mode was finished loading.

var self = this;
setTimeout(function(){
  let contentWin = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
                           .getInterface(Ci.nsIDOMWindow);
  self.enterPrintPreview(contentWin);
}, 4000);

It did generate the pdf, but it generated a blank one, better than this, it generated an about:blank pdf. This is the exact same problem that I'm already facing on the current refactor. I flip to reader mode on the background, but when I request the pdf, I get a blank one. I attached a screenshot with the modified PoC behavior (see file PoC: scenario 2).

Do you have any ideas on why this is happening? Do you think maybe because we're exiting print preview and when we flip it to reader mode, it doesn't update the document for it (docShell)?

Comment 30

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

(In reply to Matheus Longaray from comment #29)
> Comment on attachment 8737422 [details] [diff] [review]
> PoC reader mode in print preview browser
> 
> Review of attachment 8737422 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For better understanding, please see attachments when reading feedback.
> 
> ::: toolkit/content/browser-content.js
> @@ +446,5 @@
> > +
> > +      let webNav = docShell.QueryInterface(Ci.nsIWebNavigation);
> > +      let loadFlags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
> > +      let referrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT;
> > +      let uri = "about:reader?url=" + contentWindow.location.href;
> 
> Only flipping to reader mode is enough in our case? Would UX team be happy
> with it? I thought we wanted to generate the pdf for reader mode while
> taking into account all print settings. I attached a screenshot with the
> current PoC behavior (see file PoC: scenario 1).

Sorry, I don't think I understand. What PDF are we generating? I thought this was strictly about offering a new mode for printing content.

> 
> @@ +448,5 @@
> > +      let loadFlags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
> > +      let referrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT;
> > +      let uri = "about:reader?url=" + contentWindow.location.href;
> > +      webNav.loadURIWithOptions(uri, loadFlags, null, referrerPolicy,
> > +                                null, null, null);
> 
> I went ahead and added the following code to get the content window and pass
> it to enterPrintPreview() so it could generate the pdf for the browser.
> Please mind setTimeout() was only used to make sure reader mode was finished
> loading.
> 
> var self = this;
> setTimeout(function(){
>   let contentWin = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
>                            .getInterface(Ci.nsIDOMWindow);
>   self.enterPrintPreview(contentWin);
> }, 4000);
> 
> It did generate the pdf, but it generated a blank one, better than this, it
> generated an about:blank pdf. This is the exact same problem that I'm
> already facing on the current refactor. I flip to reader mode on the
> background, but when I request the pdf, I get a blank one. I attached a
> screenshot with the modified PoC behavior (see file PoC: scenario 2).
> 
> Do you have any ideas on why this is happening? Do you think maybe because
> we're exiting print preview and when we flip it to reader mode, it doesn't
> update the document for it (docShell)?

I don't think I understand what you're doing with your code fragment here. Are you attempting to enter print preview while the about:reader page is loaded?

Comment 31

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - Catching up on reviews / needinfos from comment #30)
> (In reply to Matheus Longaray from comment #29)
> > Comment on attachment 8737422 [details] [diff] [review]
> > PoC reader mode in print preview browser
> > 
> > Review of attachment 8737422 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > For better understanding, please see attachments when reading feedback.
> > 
> > ::: toolkit/content/browser-content.js
> > @@ +446,5 @@
> > > +
> > > +      let webNav = docShell.QueryInterface(Ci.nsIWebNavigation);
> > > +      let loadFlags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
> > > +      let referrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT;
> > > +      let uri = "about:reader?url=" + contentWindow.location.href;
> > 
> > Only flipping to reader mode is enough in our case? Would UX team be happy
> > with it? I thought we wanted to generate the pdf for reader mode while
> > taking into account all print settings. I attached a screenshot with the
> > current PoC behavior (see file PoC: scenario 1).
> 
> Sorry, I don't think I understand. What PDF are we generating? I thought
> this was strictly about offering a new mode for printing content.

Perhaps using the word "pdf" wasn't the right call here, sorry for that. What I meant was generating the "expected print output", the docShell under print preview mode, and not reader mode UI. Please see the following link https://bug962433.bmoattachments.org/attachment.cgi?id=8725901.

 
> > @@ +448,5 @@
> > > +      let loadFlags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
> > > +      let referrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT;
> > > +      let uri = "about:reader?url=" + contentWindow.location.href;
> > > +      webNav.loadURIWithOptions(uri, loadFlags, null, referrerPolicy,
> > > +                                null, null, null);
> > 
> > I went ahead and added the following code to get the content window and pass
> > it to enterPrintPreview() so it could generate the pdf for the browser.
> > Please mind setTimeout() was only used to make sure reader mode was finished
> > loading.
> > 
> > var self = this;
> > setTimeout(function(){
> >   let contentWin = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> >                            .getInterface(Ci.nsIDOMWindow);
> >   self.enterPrintPreview(contentWin);
> > }, 4000);
> > 
> > It did generate the pdf, but it generated a blank one, better than this, it
> > generated an about:blank pdf. This is the exact same problem that I'm
> > already facing on the current refactor. I flip to reader mode on the
> > background, but when I request the pdf, I get a blank one. I attached a
> > screenshot with the modified PoC behavior (see file PoC: scenario 2).
> > 
> > Do you have any ideas on why this is happening? Do you think maybe because
> > we're exiting print preview and when we flip it to reader mode, it doesn't
> > update the document for it (docShell)?
> 
> I don't think I understand what you're doing with your code fragment here.
> Are you attempting to enter print preview while the about:reader page is
> loaded?

I only added this code fragment for testing purposes. Note that setTimeout() would certainly not be the right way of doing this. I set an arbitrary time so it could wait for reader mode finish loading.

So, in order to get the "expected print output" (docShell under print preview mode), I tried to request from docShell its respective content window, and passed it to enterPrintPreview() function. (note that this request happens after about:reader is finished loading)

However, the result is a blank output. See https://bug962433.bmoattachments.org/attachment.cgi?id=8738166. This behavior is the exact same I was already facing refactoring the code's architecture. I flip to reader mode on the background, but when I request the content window, I get a blank result.

Do you have any ideas on why this is happening? Do you think maybe because we're exiting print preview and when we flip it to reader mode, it doesn't update the document for it (docShell)?

Let me know if I clarified your questions.

Comment 32

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

(In reply to Matheus Longaray from comment #31)
> (In reply to Mike Conley (:mconley) - Catching up on reviews / needinfos
> from comment #30)
> > (In reply to Matheus Longaray from comment #29)
> > > Comment on attachment 8737422 [details] [diff] [review]
> > > PoC reader mode in print preview browser
> > > 
> > > Review of attachment 8737422 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > For better understanding, please see attachments when reading feedback.
> > > 
> > > ::: toolkit/content/browser-content.js
> > > @@ +446,5 @@
> > > > +
> > > > +      let webNav = docShell.QueryInterface(Ci.nsIWebNavigation);
> > > > +      let loadFlags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
> > > > +      let referrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT;
> > > > +      let uri = "about:reader?url=" + contentWindow.location.href;
> > > 
> > > Only flipping to reader mode is enough in our case? Would UX team be happy
> > > with it? I thought we wanted to generate the pdf for reader mode while
> > > taking into account all print settings. I attached a screenshot with the
> > > current PoC behavior (see file PoC: scenario 1).
> > 
> > Sorry, I don't think I understand. What PDF are we generating? I thought
> > this was strictly about offering a new mode for printing content.
> 
> Perhaps using the word "pdf" wasn't the right call here, sorry for that.
> What I meant was generating the "expected print output", the docShell under
> print preview mode, and not reader mode UI. Please see the following link
> https://bug962433.bmoattachments.org/attachment.cgi?id=8725901.
> 
>  
> > > @@ +448,5 @@
> > > > +      let loadFlags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
> > > > +      let referrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT;
> > > > +      let uri = "about:reader?url=" + contentWindow.location.href;
> > > > +      webNav.loadURIWithOptions(uri, loadFlags, null, referrerPolicy,
> > > > +                                null, null, null);
> > > 
> > > I went ahead and added the following code to get the content window and pass
> > > it to enterPrintPreview() so it could generate the pdf for the browser.
> > > Please mind setTimeout() was only used to make sure reader mode was finished
> > > loading.
> > > 
> > > var self = this;
> > > setTimeout(function(){
> > >   let contentWin = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> > >                            .getInterface(Ci.nsIDOMWindow);
> > >   self.enterPrintPreview(contentWin);
> > > }, 4000);
> > > 
> > > It did generate the pdf, but it generated a blank one, better than this, it
> > > generated an about:blank pdf. This is the exact same problem that I'm
> > > already facing on the current refactor. I flip to reader mode on the
> > > background, but when I request the pdf, I get a blank one. I attached a
> > > screenshot with the modified PoC behavior (see file PoC: scenario 2).
> > > 
> > > Do you have any ideas on why this is happening? Do you think maybe because
> > > we're exiting print preview and when we flip it to reader mode, it doesn't
> > > update the document for it (docShell)?
> > 
> > I don't think I understand what you're doing with your code fragment here.
> > Are you attempting to enter print preview while the about:reader page is
> > loaded?
> 
> I only added this code fragment for testing purposes. Note that setTimeout()
> would certainly not be the right way of doing this. I set an arbitrary time
> so it could wait for reader mode finish loading.
> 
> So, in order to get the "expected print output" (docShell under print
> preview mode), I tried to request from docShell its respective content
> window, and passed it to enterPrintPreview() function. (note that this
> request happens after about:reader is finished loading)
> 
> However, the result is a blank output. See
> https://bug962433.bmoattachments.org/attachment.cgi?id=8738166. This
> behavior is the exact same I was already facing refactoring the code's
> architecture. I flip to reader mode on the background, but when I request
> the content window, I get a blank result.
> 
> Do you have any ideas on why this is happening? Do you think maybe because
> we're exiting print preview and when we flip it to reader mode, it doesn't
> update the document for it (docShell)?
> 
> Let me know if I clarified your questions.

Ah, I think I understand.

So, the problem here, I think, is that enterPrintPreview is expecting the contentWindow not of itself, but of the contentWindow we expect to show a print preview of. So any time we send down an instruction for showing a print preview, it's important that we send down the "outerWindowID" of the frame we're interested in printing so that we can get at it in the browser-content.js script using Services.wm.getOuterWindowWithId, and passing _that_ to enterPrintPreview.

I'm pretty sure passing print preview a reference to its own window is undefined behaviour, or it might be throwing an exception somewhere.

Comment 33

[Matheus Longaray (:mlongaray)]

Attachment: WIP4: Lazy approach

This patch adds a new checkbox on print preview UI that allows the user to change the layout for easier reading. It uses reader-mode infrastructure internally.

After a few discussions with Mike Conley on the IRC channel, we decided to go back to our original idea. The main difference is that we want to make it lazy, only load a second tab for reader mode if the user clicks on the checkbox. Also, got rid of a lot of stuff that had been added to add the pref to nsIPrintSettings/nsPrintOptions.

Comment 34

[Matheus Longaray (:mlongaray)]

(In reply to Matheus Longaray from comment #33)
> Created attachment 8743505 [details] [diff] [review]
> WIP4: Lazy approach
> 
> This patch adds a new checkbox on print preview UI that allows the user to
> change the layout for easier reading. It uses reader-mode infrastructure
> internally. 

To put it another way, this patch adds a new checkbox on print preview UI that allows the user to change the output representation using reader mode. Leveraging reader mode's infrastructure, we can provide a better printing experience when printing articles by removing all clutter and unnecessary content.

> After a few discussions with Mike Conley on the IRC channel, we decided to
> go back to our original idea. The main difference is that we want to make it
> lazy, only load a second tab for reader mode if the user clicks on the
> checkbox. Also, got rid of a lot of stuff that had been added to add the
> pref to nsIPrintSettings/nsPrintOptions.

Comment 35

[Matheus Longaray (:mlongaray)]

Comment on attachment 8743505 [details] [diff] [review]
WIP4: Lazy approach

Set flag "feedback?" to "review?". Added Mike Conley and Margaret Leibovic as reviewers.

Comment 36

[Matheus Longaray (:mlongaray)]

Comment on attachment 8725894 [details]
Scenario 1: Home page tab when print previewing

Early on February, we had an e-mail thread with Christopher Arnold, Philipp Sackl and Stephen Horlander in order to evaluate the proposed feature. We provided some early build spins and got green light on UX. We added required tooltips when hovering over the checkbox, and added print media queries to fix typographic issues when printing. Philipp is on FTO right now, so I'm only adding Stephen for ui-review. Please advise.

Comment 37

[Matheus Longaray (:mlongaray)]

Attachment: wip5_spcheckbox_lazy_polish_26-04.diff

Small refactor for better and concise messaging.

Comment 38

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

Comment on attachment 8745538 [details] [diff] [review]
wip5_spcheckbox_lazy_polish_26-04.diff

Review of attachment 8745538 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Matheus.

I've had a think about this, and I've written my concerns below. I think we're approaching the right implementation, but I think there's some further work to do (mainly, I'm concerned about re-loading the document unnecessarily).

::: browser/base/content/browser.js
@@ +3206,5 @@
>      }
>      return gBrowser.getBrowserForTab(this._printPreviewTab);
>    },
> +  loadReaderMode: function () {
> +    this._simplifyPageTab = gBrowser.loadOneTab("about:reader?url=" +

One thing that worries me about this is that we're going to cause the page to be re-requested off of the network in order to show ReaderMode.

There might be some trickery we can do with nsIWebPageDescriptor's, to try to load the document out of the network cache if possible (that's what we do for view source, for example).

What I'm suggesting is that instead of loading this page at about:reader, we send it to about:blank and stop it immediately. Then, we send a message down to it with the outerWindowID of the tab to be previewed.

We'll need a handler for that message - probably in browser-content.js, in roughly the same region you've been working in. That handler should take the outerWindowID and try to resolve it to the "current descriptor" of the document to be previewed. The "current descriptor" is a token that allows us to retrieve the document from the cache.

Here's an example of us getting at the current descriptor for view source, having been handed an outerWindowID: https://dxr.mozilla.org/mozilla-central/rev/77cead2cd20300623eea2416bc9bce4d5021df09/toolkit/components/viewsource/content/viewSource-content.js#223

And here's how the page descriptor is used: https://dxr.mozilla.org/mozilla-central/rev/77cead2cd20300623eea2416bc9bce4d5021df09/toolkit/components/viewsource/content/viewSource-content.js#319

Though you'll likely want to use DISPLAY_NORMAL instead of DISPLAY_AS_SOURCE.

When the page loads, we'll probably then want to run it through the ReaderMode stuff to convert it, and then when it's ready, send a message to the parent saying that it's okay to present the simplified browser tab.

@@ +3230,5 @@
>      gBrowser.selectedTab = this._tabBeforePrintPreview;
>      this._tabBeforePrintPreview = null;
>      gInPrintPreviewMode = false;
>      this._toggleAffectedChrome();
> +    if (this._simplifyPageTab != null) {

This can just be:

```
if (this._simplifyPageTab) {
  // ...
}
```

I guess.

::: toolkit/content/browser-content.js
@@ +387,5 @@
> +    // We need to add an observer to know when reader mode document has been
> +    // loaded. If we receive the appropriate message, we tell the parent that
> +    // reader mode document is ready and we are able to enter on preview mode.
> +    Services.obs.addObserver( function (subject, topic, data) {
> +      if (topic === "AboutReader:Ready") {

I don't recommend using this observer like this - this will be registered for every tab, and every time any ReaderMode is rendered in the content process, every one of these tabs will be notified, and all will send a message, which will result in a lot of unnecessary chatter.

Instead, I think we should try to use the ReaderMode module directly instead of relying on about:reader.

Comment 39

[:Margaret Leibovic]

Comment on attachment 8745538 [details] [diff] [review]
wip5_spcheckbox_lazy_polish_26-04.diff

Review of attachment 8745538 [details] [diff] [review]:
-----------------------------------------------------------------

mconley is more qualified to review this than I am.

Comment 40

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

(In reply to :Margaret Leibovic from comment #39)
> Comment on attachment 8745538 [details] [diff] [review]
> wip5_spcheckbox_lazy_polish_26-04.diff
> 
> Review of attachment 8745538 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> mconley is more qualified to review this than I am.

You're okay with me reviewing the aboutReader* CSS stuff? I just figured you might want to be in the loop on that.

Comment 41

[:Margaret Leibovic]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #40)
> (In reply to :Margaret Leibovic from comment #39)
> > Comment on attachment 8745538 [details] [diff] [review]
> > wip5_spcheckbox_lazy_polish_26-04.diff
> > 
> > Review of attachment 8745538 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > mconley is more qualified to review this than I am.
> 
> You're okay with me reviewing the aboutReader* CSS stuff? I just figured you
> might want to be in the loop on that.

I appreciate being informed, but I trust you to be able to review it :)

I would just look out for our existing print styles, but are just to hide the toolbar:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/aboutReaderControls.css#343

Comment 42

[Matheus Longaray (:mlongaray)]

Attachment: WIP6: Load simplified tab using nsIWebPageDescriptor

After Mike's review and some interaction on IRC, we're sending over a new patch that uses nsIWebPageDescriptor to load the document out of the network cache.

Comment 43

[Matheus Longaray (:mlongaray)]

Attachment: Preview/Reader Mode IPC messaging sketch

Sharing a model to better understand all IPC messaging, between parent and child process, related to print preview and reader mode.

Comment 44

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

Comment on attachment 8750492 [details] [diff] [review]
WIP6: Load simplified tab using nsIWebPageDescriptor

Review of attachment 8750492 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good, Matheus! I'm going to give this some testing tomorrow, but I'm pretty pleased with what we've got here.

Comment 45

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #44)
> Comment on attachment 8750492 [details] [diff] [review]
> WIP6: Load simplified tab using nsIWebPageDescriptor
> 
> Review of attachment 8750492 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks really good, Matheus! I'm going to give this some testing
> tomorrow, but I'm pretty pleased with what we've got here.

That's great! Look forward to hearing more from you.

Comment 46

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

Comment on attachment 8750492 [details] [diff] [review]
WIP6: Load simplified tab using nsIWebPageDescriptor

Review of attachment 8750492 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good!

I have two final concerns:

1) We need to make sure we don't accidentally pollute Thunderbird and SeaMonkey with this stuff. We should let them opt in if they want it. I have instructions on how to do that below.
2) I want to make absolutely sure that we really require the AboutReaderListener stuff. It seems a bit strange to recycle it, considering how tailored it is to about:reader. ReaderMode is a separate module for a reason, and I wonder if we can use it directly instead of using that intermediary. Have you tried that?

::: toolkit/components/printing/content/printPreviewBindings.xml
@@ +87,5 @@
>            oncommand="parentNode.parentNode.orient('landscape');"/>
>        </xul:hbox>
>  
>        <xul:toolbarseparator class="toolbarseparator-primary"/>
> +      <xul:checkbox label="&simplifyPage.label;" checked="false" disabled="true"

It's come to my attention that this is going to impact non-Firefox print-preview as well (in Thunderbird and SeaMonkey, for example). We might want to hide this checkbox by default, and only reveal it in the construction of the binding of a pref is set.

Specifically, what I'd suggest is in the initialize function of this binding: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/printing/content/printPreviewBindings.xml#139

That we get a hold of the preferences service, via:

let {Services} = Components.utils.import("resource://gre/modules/Services.jsm", {});
if (!Services.prefs.getBoolPref("print.use_simplification")) {
  this.mSimplifyPageCheckbox.hidden = true;
  // We'll need to hide the new separator too.
}

You'll need to add the pref default to modules/libpref/init/all.js. Default it to false, and then in browser/app/profile/firefox.js, set that same pref to true so that it's activated for Firefox only.

::: toolkit/components/printing/content/printUtils.js
@@ +517,5 @@
> +        this._listener.createSimplifiedBrowser();
> +
> +        let spMM = this._listener.getSimplifiedSourceBrowser().messageManager;
> +        spMM.addMessageListener("Printing:Preview:ReaderModeReady", function onReaderReady() {
> +          spMM.removeMessageListener("Printing:Preview:ReaderModeReady", onReaderReady);

This is actually not going to remove the message listener, because the signature doesn't match (onReaderReady is being removed, onReadyReady.bind(this) is what was added).

What I'd suggest instead is the following:

1) Have createSimplifiedBrowser return the browser it just created. Assign that value to some variable in this scope.
2) Don't use "this" inside your onReadyReady - just use the variable in the closure you defined above.

@@ +525,5 @@
> +
> +        // We add a listener to know when we are able to toggle to reader mode
> +        // after loading the page from cache.
> +        spMM.addMessageListener("Reader:UpdateReaderButton", function onReady(message) {
> +          if (message.data.isArticle == true) {

if (message.data.isArticle) {

@@ +526,5 @@
> +        // We add a listener to know when we are able to toggle to reader mode
> +        // after loading the page from cache.
> +        spMM.addMessageListener("Reader:UpdateReaderButton", function onReady(message) {
> +          if (message.data.isArticle == true) {
> +            spMM.removeMessageListener("Reader:UpdateReaderButton", onReady);

We should probably remove this after the first update, unless there's a good reason to keep it around (if we never get isArticle, then this will never get removed...)

@@ +527,5 @@
> +        // after loading the page from cache.
> +        spMM.addMessageListener("Reader:UpdateReaderButton", function onReady(message) {
> +          if (message.data.isArticle == true) {
> +            spMM.removeMessageListener("Reader:UpdateReaderButton", onReady);
> +            spMM.sendAsyncMessage("Reader:ToggleReaderMode");

Out of curiosity, is the ReaderMode (not the AboutReaderListener) API sufficient enough that we could communicate to it directly instead of going through AboutReaderListener? I ask, because it seems kinda funny that we have to do this bit of messaging when really the content should be able to switch to reader mode after figuring out that it has the capability to. What we really want is just a single message from the content that says, "I can be in reader mode".

::: toolkit/content/browser-content.js
@@ +436,5 @@
> +      case "Printing:Preview:LoadFromCache": {
> +        // We need to add an observer to know when reader mode document has been
> +        // loaded. If we receive the appropriate message, we tell the parent that
> +        // reader mode document is ready and we are able to enter on preview mode.
> +        Services.obs.addObserver( function (subject, topic, data) {

This is another thing I'm wondering if we can drop, if we talk to ReaderMode directly.

@@ +527,5 @@
> +      let pageDescriptor = otherWebNav.QueryInterface(Ci.nsIWebPageDescriptor)
> +                                      .currentDescriptor;
> +      let pageLoader = docShell.QueryInterface(Ci.nsIWebPageDescriptor);
> +      pageLoader.loadPage(pageDescriptor,
> +                            Ci.nsIWebPageDescriptor.DISPLAY_AS_NORMAL);

Nit - indentation

@@ +529,5 @@
> +      let pageLoader = docShell.QueryInterface(Ci.nsIWebPageDescriptor);
> +      pageLoader.loadPage(pageDescriptor,
> +                            Ci.nsIWebPageDescriptor.DISPLAY_AS_NORMAL);
> +
> +      let shEntrySource = pageDescriptor.QueryInterface(Ci.nsISHEntry);

I don't think it's necessary to do the shEntry stuff from lines 533-543... I don't think we allow the user to navigate the print preview browser or the background browser tabs.

::: toolkit/locales/en-US/chrome/global/printPreview.dtd
@@ +33,5 @@
>  <!ENTITY ShrinkToFit.label    "Shrink To Fit">
>  <!ENTITY customPrompt.title   "Custom Scale…">
> +<!ENTITY simplifyPage.label   "Simplify Page">
> +<!ENTITY simplifyPage.enabled.tooltip  "Change layout for easier reading">
> +<!ENTITY simplifyPage.disabled.tooltip "This page layout cannot be changed">

Perhaps this should be "This page cannot be automatically simplified"

Comment 47

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #46)
> Comment on attachment 8750492 [details] [diff] [review]
> WIP6: Load simplified tab using nsIWebPageDescriptor
> 
> Review of attachment 8750492 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks really good!
> 
> I have two final concerns:
> 
> 1) We need to make sure we don't accidentally pollute Thunderbird and
> SeaMonkey with this stuff. We should let them opt in if they want it. I have
> instructions on how to do that below.

Done.

> 2) I want to make absolutely sure that we really require the
> AboutReaderListener stuff. It seems a bit strange to recycle it, considering
> how tailored it is to about:reader. ReaderMode is a separate module for a
> reason, and I wonder if we can use it directly instead of using that
> intermediary. Have you tried that?
> 

Yes, that's what I wondered too. I tried to use "only" ReaderMode, after loading the page out of network cache, but we ended up having an exception. The exception was happening right here (https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm#73), apparently "dropdownToggle" is null.

From what I could see, https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/content/aboutReader.html was not loaded at the time and that makes me wonder if ReaderMode and AboutReaderListener are actually tied together. Maybe we can ping someone that has more knowledge about it.

What I tried to do was something like the following code. After firing up a promise and getting it resolved, I'd construct AboutReader with the JS object that represents the document parsed. Like this:

let articlePromise = ReaderMode.parseDocument(content.document).catch(Cu.reportError);
articlePromise.then( function() {
	new AboutReader(global, content, articlePromise);
});

The reason why I tested this was due the structure of the code, at least of how I interpreted it. From what I can see, after tab-content tells parent to update the reader button (https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tab-content.js#368), parent will update the browser accordingly (https://dxr.mozilla.org/mozilla-central/source/browser/modules/ReaderParent.jsm#80).

When the user clicks on it, parent sends a message down to content to toggle to reader mode (https://dxr.mozilla.org/mozilla-central/source/browser/modules/ReaderParent.jsm#136). Content will then receive this message, fire up a promise for the document parsed, and enter on reader mode (https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tab-content.js#262).

When it enters on reader mode (https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm#94), I think it loads the AboutReader stuff (correct me if I'm wrong). After content receives "AboutReaderContentLoaded" (https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tab-content.js#292), it updates the toolbar icon to show the "reader active" icon and constructs AboutReader with the promise it fired up before.

What do you think? Can you suggest something?

> ::: toolkit/components/printing/content/printUtils.js
> @@ +526,5 @@
> > +        // We add a listener to know when we are able to toggle to reader mode
> > +        // after loading the page from cache.
> > +        spMM.addMessageListener("Reader:UpdateReaderButton", function onReady(message) {
> > +          if (message.data.isArticle == true) {
> > +            spMM.removeMessageListener("Reader:UpdateReaderButton", onReady);
> 
> We should probably remove this after the first update, unless there's a good
> reason to keep it around (if we never get isArticle, then this will never
> get removed...)
> 

The reason why it wasn't removed after the first update, is because we receive a first message with "message.data.isArticle == false" and so we cannot toggle to reader mode yet. After receiving isArticle == true, we remove the listener and toggle to reader mode.

I can be wrong about this, but I believe we will indeed get isArticle == true since we enabled the simplify page checkbox in the first place (when the page is an article).

What should we do?

> @@ +527,5 @@
> > +        // after loading the page from cache.
> > +        spMM.addMessageListener("Reader:UpdateReaderButton", function onReady(message) {
> > +          if (message.data.isArticle == true) {
> > +            spMM.removeMessageListener("Reader:UpdateReaderButton", onReady);
> > +            spMM.sendAsyncMessage("Reader:ToggleReaderMode");
> 
> Out of curiosity, is the ReaderMode (not the AboutReaderListener) API
> sufficient enough that we could communicate to it directly instead of going
> through AboutReaderListener? I ask, because it seems kinda funny that we
> have to do this bit of messaging when really the content should be able to
> switch to reader mode after figuring out that it has the capability to. What
> we really want is just a single message from the content that says, "I can
> be in reader mode".
> 

If we manage to talk to ReaderMode directly, we could get rid of this messaging as well as the observer added on browser-content.

Comment 48

[Matheus Longaray (:mlongaray)]

Attachment: WIP7: Simplified tab using nsIWebPageDescriptor refactor

This patch applies a small refactor given Mike Conley's review as well as addresses nits and unnecessary code.

Comment 49

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

(In reply to Matheus Longaray from comment #47)
> 
> Yes, that's what I wondered too. I tried to use "only" ReaderMode, after
> loading the page out of network cache, but we ended up having an exception.
> The exception was happening right here
> (https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/
> AboutReader.jsm#73), apparently "dropdownToggle" is null.
> 
> From what I could see,
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/
> content/aboutReader.html was not loaded at the time and that makes me wonder
> if ReaderMode and AboutReaderListener are actually tied together. Maybe we
> can ping someone that has more knowledge about it.

Hrm. I'd like to know more about that exception. Do you have a stack for it?

I do think that ReaderMode is supposed to be mostly (if not entirely) decoupled from AboutReader...

> 
> What I tried to do was something like the following code. After firing up a
> promise and getting it resolved, I'd construct AboutReader with the JS
> object that represents the document parsed. Like this:
> 
> let articlePromise =
> ReaderMode.parseDocument(content.document).catch(Cu.reportError);
> articlePromise.then( function() {
> 	new AboutReader(global, content, articlePromise);
> });

Do you mean you'd construct ReaderMode? You're not using AboutReader in the code block above, and I think we need to be crystal clear on our terminology here in order to avoid confusion.

Comment 50

[Matheus Longaray (:mlongaray)]

Attachment: About Reader Exception

Here's a screenshot for an exception caused by calling About Reader.

Comment 51

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #49)
> (In reply to Matheus Longaray from comment #47)
> > 
> > Yes, that's what I wondered too. I tried to use "only" ReaderMode, after
> > loading the page out of network cache, but we ended up having an exception.
> > The exception was happening right here
> > (https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/
> > AboutReader.jsm#73), apparently "dropdownToggle" is null.
> > 
> > From what I could see,
> > https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/
> > content/aboutReader.html was not loaded at the time and that makes me wonder
> > if ReaderMode and AboutReaderListener are actually tied together. Maybe we
> > can ping someone that has more knowledge about it.
> 
> Hrm. I'd like to know more about that exception. Do you have a stack for it?
> 
> I do think that ReaderMode is supposed to be mostly (if not entirely)
> decoupled from AboutReader...
> 

I do have a stack for that. See attachment "About Reader Exception".

Yes, I also do think ReaderMode is decoupled from AboutReader, but from what I can see, ReaderMode only returns a JS object representing the document parsed (https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm#198). AboutReader would then use that to present the document on its UI (side toolbar, CSS, etc).

> > 
> > What I tried to do was something like the following code. After firing up a
> > promise and getting it resolved, I'd construct AboutReader with the JS
> > object that represents the document parsed. Like this:
> > 
> > let articlePromise =
> > ReaderMode.parseDocument(content.document).catch(Cu.reportError);
> > articlePromise.then( function() {
> > 	new AboutReader(global, content, articlePromise);
> > });
> 
> Do you mean you'd construct ReaderMode? You're not using AboutReader in the
> code block above, and I think we need to be crystal clear on our terminology
> here in order to avoid confusion.

See line 4 in the code snippet. After the promise gets resolved, that line would construct AboutReader and present the document (given what I understood from ReaderMode and About Reader's architecture).

Comment 52

[:Gijs (he/him)]

Comment on attachment 8754032 [details] [diff] [review]
WIP7: Simplified tab using nsIWebPageDescriptor refactor

Review of attachment 8754032 [details] [diff] [review]:
-----------------------------------------------------------------

So, FWIW, I have a reasonable grip on reader mode code, but I haven't looked at this patch in detail and I'm not sure I follow what it's trying to do. It kind of seems like it's jumping through hoops trying to load reader mode content without actually using the existing enter/exit reader mode functionality, in order to ensure we load the content from cache or something like that?

One thing I noticed from a quick look is below. I can investigate in more detail on Monday if you can provide steps that reproduce the error you're quoting? That's assuming the issue below doesn't fix it. In which case, please flag me for needinfo/review. Thanks!

::: toolkit/content/browser-content.js
@@ +438,5 @@
> +        // loaded. If we receive the appropriate message, we tell the parent that
> +        // reader mode document is ready and we are able to enter on preview mode.
> +        Services.obs.addObserver( function (subject, topic, data) {
> +          if (topic === "AboutReader:Ready") {
> +            Services.obs.removeObserver(this, "AboutReader:Ready");

Nit: this doesn't work - you've added a function as the observer, and you're removing |this|, which is going to be the global. Perhaps that's why you're seeing exceptions, because you're sending down this message to this particular tab all the time, even when other tabs load about:reader (either for this print thing or because you're manually loading about:reader ) ?

Also, you should probably check that this is happening in the right window, so that about:reader loading in a separate tab does not cause a race condition here.

Comment 53

[Matheus Longaray (:mlongaray)]

(In reply to :Gijs Kruitbosch from comment #52)
> Comment on attachment 8754032 [details] [diff] [review]
> WIP7: Simplified tab using nsIWebPageDescriptor refactor
> 
> Review of attachment 8754032 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, FWIW, I have a reasonable grip on reader mode code, but I haven't looked
> at this patch in detail and I'm not sure I follow what it's trying to do. It
> kind of seems like it's jumping through hoops trying to load reader mode
> content without actually using the existing enter/exit reader mode
> functionality, in order to ensure we load the content from cache or
> something like that?
> 
> One thing I noticed from a quick look is below. I can investigate in more
> detail on Monday if you can provide steps that reproduce the error you're
> quoting? That's assuming the issue below doesn't fix it. In which case,
> please flag me for needinfo/review. Thanks!
> 

Hi Gijs, thanks for your feedback.

In a nutshell, what this patch does is to add a check-box on print preview UI that will allow the user to clean up print output leveraging reader mode. See https://bug962433.bmoattachments.org/attachment.cgi?id=8725901.

Once the user clicks on it, we lazily instantiate a new tab (in background) that loads the page out from cache given the original window ID (the one that's being printed out). After this new tab is ready to toggle to reader mode, we toggle it, and pass this window as reference to print preview browser.

> ::: toolkit/content/browser-content.js
> @@ +438,5 @@
> > +        // loaded. If we receive the appropriate message, we tell the parent that
> > +        // reader mode document is ready and we are able to enter on preview mode.
> > +        Services.obs.addObserver( function (subject, topic, data) {
> > +          if (topic === "AboutReader:Ready") {
> > +            Services.obs.removeObserver(this, "AboutReader:Ready");
> 
> Nit: this doesn't work - you've added a function as the observer, and you're
> removing |this|, which is going to be the global. Perhaps that's why you're
> seeing exceptions, because you're sending down this message to this
> particular tab all the time, even when other tabs load about:reader (either
> for this print thing or because you're manually loading about:reader ) ?
> 
> Also, you should probably check that this is happening in the right window,
> so that about:reader loading in a separate tab does not cause a race
> condition here.

This patch was not getting any exception. The exception that was mentioned was related to a concern Mike Conley had, if we could manage to take a different approach. Please see https://bugzilla.mozilla.org/show_bug.cgi?id=962433#c46 & https://bugzilla.mozilla.org/show_bug.cgi?id=962433#c47.

If I can help with anything, that message is sent down to child only once and to the right window. And it only happens on print preview, if the user selects the simplify page checkbox.

Mike, can you confirm on this?

Comment 54

[:Gijs (he/him)]

(In reply to Matheus Longaray from comment #53)
> > ::: toolkit/content/browser-content.js
> > @@ +438,5 @@
> > > +        // loaded. If we receive the appropriate message, we tell the parent that
> > > +        // reader mode document is ready and we are able to enter on preview mode.
> > > +        Services.obs.addObserver( function (subject, topic, data) {
> > > +          if (topic === "AboutReader:Ready") {
> > > +            Services.obs.removeObserver(this, "AboutReader:Ready");
> > 
> If I can help with anything, that message is sent down to child only once
> and to the right window. And it only happens on print preview, if the user
> selects the simplify page checkbox.

I could be wrong - it has been known to happen! - but it would surprise me if I was. Here are some steps:

1. open a random article from the BBC.
2. in a new window (not tab, window!), open something else, and invoke print preview on it.
3. invoke the 'simplify' stuff so the tab gets opened in reader mode to be printed with that instead.
4. go back to your other window. Invoke reader mode using the button in the location bar.

Reader Mode dispatches an "AboutReader:Ready" notification via the observer service when it's loaded (here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm#772 ). Your code adds an observer for this notification, and the removeObserver call passes something other than the function that you added as the observer, and so the observer you've added is never removed. As a result, I believe it will keep being called for any other reader mode tabs being loaded.

Whether that is related to the exception you're seeing I cannot say, but it should still be fixed, and it might have been related depending on the other code you had (which I can't see, of course...).

Comment 55

[:Gijs (he/him)]

Mike, is there a reason not to just change the existing tab/docshell to enter/leave reader mode ? Then you don't need to care about the internals of it so much ... or are we not allowed to navigate the docshell/tab that's in print preview mode or something?

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tab-content.js#260-266

responds to the user toggling reader mode, and will reuse the document that's currently loaded in the docshell/browser to enter reader mode, and do all the hard work for you...

Comment 56

[Matheus Longaray (:mlongaray)]

(In reply to :Gijs Kruitbosch from comment #54)
> (In reply to Matheus Longaray from comment #53)
> > > ::: toolkit/content/browser-content.js
> > > @@ +438,5 @@
> > > > +        // loaded. If we receive the appropriate message, we tell the parent that
> > > > +        // reader mode document is ready and we are able to enter on preview mode.
> > > > +        Services.obs.addObserver( function (subject, topic, data) {
> > > > +          if (topic === "AboutReader:Ready") {
> > > > +            Services.obs.removeObserver(this, "AboutReader:Ready");
> > > 
> > If I can help with anything, that message is sent down to child only once
> > and to the right window. And it only happens on print preview, if the user
> > selects the simplify page checkbox.
> 
> I could be wrong - it has been known to happen! - but it would surprise me
> if I was. Here are some steps:
> 
> 1. open a random article from the BBC.
> 2. in a new window (not tab, window!), open something else, and invoke print
> preview on it.
> 3. invoke the 'simplify' stuff so the tab gets opened in reader mode to be
> printed with that instead.
> 4. go back to your other window. Invoke reader mode using the button in the
> location bar.
> 
> Reader Mode dispatches an "AboutReader:Ready" notification via the observer
> service when it's loaded (here:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/
> AboutReader.jsm#772 ). Your code adds an observer for this notification, and
> the removeObserver call passes something other than the function that you
> added as the observer, and so the observer you've added is never removed. As
> a result, I believe it will keep being called for any other reader mode tabs
> being loaded.

I followed the steps you mentioned and couldn't reproduce the issue. I used console.log() to confirm it, and it was called only once.

> Whether that is related to the exception you're seeing I cannot say, but it
> should still be fixed, and it might have been related depending on the other
> code you had (which I can't see, of course...).

It should certainly be fixed, it was already on my list here to address.

Would something like this work?

Services.obs.addObserver( function observer(subject, topic) {
    if (topic === "AboutReader:Ready") {
        Services.obs.removeObserver(observer, topic);
    }
}, "AboutReader:Ready", false);

Comment 57

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

(In reply to :Gijs Kruitbosch from comment #55)
> Mike, is there a reason not to just change the existing tab/docshell to
> enter/leave reader mode ? Then you don't need to care about the internals of
> it so much ... or are we not allowed to navigate the docshell/tab that's in
> print preview mode or something?
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tab-
> content.js#260-266
> 
> responds to the user toggling reader mode, and will reuse the document
> that's currently loaded in the docshell/browser to enter reader mode, and do
> all the hard work for you...

The problem is that we want a print preview of the document in Reader Mode, and I'm afraid you cannot print preview your own docShell (we tried this earlier, and had to settle for opening Reader Mode in a background tab).

Comment 58

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

Hey - Mathius - you sent me some pastebins showing the console.dir of the documents during a race condition that you're sorting out. Can you show me the contents of document.body.innerHTML as well in both cases?

Comment 59

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

Sorry, Matheus, not Mathius.

Comment 60

[Matheus Longaray (:mlongaray)]

Attachment: Test Case

Mike Conley and I figured out why we were having an exception when using only ReaderMode primitives to come up with the solution. Mike thought it would be best if we just jam the JS object what we get from Reader Mode into the DOM of the simplified tab (while loading CSS resources from AboutReader). With this, we wouldn't be loading the AboutReader's UI and APIs.

We are facing an unexpected behaviour though. With two URLs used for testing (Wikipedia and Techcrunch), the solution seems to only works for the Wikipedia one. After adding a setTimeout() prior to parsing the document, the solutions works for both URLs. Attached is a .zip file containing snippet of code and results.

Comment 61

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

Hm. Interesting. The DOM is all there, but ReaderMode doesn't like it just yet. We need to find out why it's returning null, naturally.

Some suggestions:

1) Set this flag to 1: https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/toolkit/components/reader/ReaderMode.jsm#44 to enable more console logging - there might be useful error data in there
2) Check the Browser Console in the error case to see if ReaderMode is reporting any exceptions
3) Try to use the Browser Toolbox to step through the parseDocument ReaderMode method: 

https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

See https://developer.mozilla.org/en-US/docs/Tools/Debugger in particular.

Note that if you're running with e10s mode enabled, you'll need to use the Browser Content Toolbox to debug the script running in the content process: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Debugging_frame_scripts

4) If that doesn't work out, then use dumping to figure out where in parseDocument things are breaking down.

Comment 62

[Matheus Longaray (:mlongaray)]

Attachment: WIP8: Simplified tab using Reader Mode primitives directly

This patch addresses some last concerns Mike Conley had about using only Reader Mode primitives to come up with the solution.

Mike thought it would be best if we just jam the JS object that we get from Reader Mode into the DOM of the simplified tab (while loading CSS resources from AboutReader). With this, we wouldn't be loading the AboutReader's UI and APIs.

Since we decided to go in this direction, we realized that we don't even need to load the page from cache anymore, we just have to use the original browser's document as reference when using Reader Mode primitives. The original content is not affected when parsing it, and the result/feature is a lot more faster.

Comment 63

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

Comment on attachment 8758055 [details] [diff] [review]
WIP8: Simplified tab using Reader Mode primitives directly

Review of attachment 8758055 [details] [diff] [review]:
-----------------------------------------------------------------

We're definitely on the right track here, Matheus. I have some follow-up questions.

::: toolkit/components/printing/content/printUtils.js
@@ +517,5 @@
> +        // up a message to enter on print preview.
> +        simplifiedBrowser = this._listener.createSimplifiedBrowser();
> +
> +        let spMM = simplifiedBrowser.messageManager;
> +        spMM.addMessageListener("Printing:Preview:ReaderModeReady", function onReaderReady() {

Similar to my question during the last patch - why do we need to send this Printing:Preview:ReaderModeReady message to the parent? It's just sending another message down to enter print preview. Can we not just do that in browser-content.js and save the IPC traffic?

So perhaps instead of sending Printing:Preview:Enter after we've entered simplify mode, send Printing:Preview:EnterSimplified to the ppBrowser message manager, passing it the outerWindowID of the simplified browser, which will do the parsing and then enter print preview immediately.

@@ +577,5 @@
>        navToolbox.parentNode.insertBefore(printPreviewTB, navToolbox);
>        printPreviewTB.initialize(ppBrowser);
>  
> +      // Enable simplify page checkbox when the page is an article
> +      if (this._sourceBrowser.isArticle)

Now that we're not using AboutReader here directly, is isArticle still going to work? Or is it now up to us to message the parent if the document is simplify-able or not?

::: toolkit/content/browser-content.js
@@ +473,5 @@
> +    let articlePromise = ReaderMode.parseDocument(contentWindow.document).catch(Cu.reportError);
> +    articlePromise.then(function (article) {
> +      content.document.head.innerHTML = "";
> +
> +      //Set title of document

Nit - spaces after each // for each section

@@ +476,5 @@
> +
> +      //Set title of document
> +      content.document.title = article.title;
> +
> +      //Set base URI of document

Maybe add a comment here saying that the print preview code will read this value to populate the URL field in print settings so that it doesn't show "about:blank".

@@ +504,5 @@
> +
> +      //Create style element for header div and import aboutReaderControls.css
> +      let controlHeaderStyle = content.document.createElement("style");
> +      controlHeaderStyle.setAttribute("scoped", "");
> +      controlHeaderStyle.textContent = "@import url(\"chrome://global/skin/aboutReaderControls.css\");";

Out of curiosity, why is aboutReaderControls.css needed?

::: toolkit/themes/shared/aboutReaderContent.css
@@ +43,5 @@
>  #moz-reader-content.line-height9 {
>    line-height: 2.6em;
>  }
>  
> +@media print {

Still interested in knowing why we need this file included, and these changes.

Comment 64

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #63)
> Comment on attachment 8758055 [details] [diff] [review]
> WIP8: Simplified tab using Reader Mode primitives directly
> 
> Review of attachment 8758055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We're definitely on the right track here, Matheus. I have some follow-up
> questions.
> 
> ::: toolkit/components/printing/content/printUtils.js
> @@ +517,5 @@
> > +        // up a message to enter on print preview.
> > +        simplifiedBrowser = this._listener.createSimplifiedBrowser();
> > +
> > +        let spMM = simplifiedBrowser.messageManager;
> > +        spMM.addMessageListener("Printing:Preview:ReaderModeReady", function onReaderReady() {
> 
> Similar to my question during the last patch - why do we need to send this
> Printing:Preview:ReaderModeReady message to the parent? It's just sending
> another message down to enter print preview. Can we not just do that in
> browser-content.js and save the IPC traffic?
> 
> So perhaps instead of sending Printing:Preview:Enter after we've entered
> simplify mode, send Printing:Preview:EnterSimplified to the ppBrowser
> message manager, passing it the outerWindowID of the simplified browser,
> which will do the parsing and then enter print preview immediately.
> 

Regarding this, Mike and I had a conversation on IRC and decided to keep the original approach. We will document why we're doing this way.

> @@ +577,5 @@
> >        navToolbox.parentNode.insertBefore(printPreviewTB, navToolbox);
> >        printPreviewTB.initialize(ppBrowser);
> >  
> > +      // Enable simplify page checkbox when the page is an article
> > +      if (this._sourceBrowser.isArticle)
> 
> Now that we're not using AboutReader here directly, is isArticle still going
> to work? Or is it now up to us to message the parent if the document is
> simplify-able or not?
> 

Yes, it will work, and even for different cases like when opening print preview from a new tab (about:blank) or a page that's already in reader mode (about:reader?url=). We are leveraging a work that's already being done by reader mode in background for every tab (http://searchfox.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm#163). So I think we don't need to be worried about sending a message to the parent if the document is simplify-able or not. What do you think?

> ::: toolkit/content/browser-content.js
> @@ +473,5 @@
> > +    let articlePromise = ReaderMode.parseDocument(contentWindow.document).catch(Cu.reportError);
> > +    articlePromise.then(function (article) {
> > +      content.document.head.innerHTML = "";
> > +
> > +      //Set title of document
> 
> Nit - spaces after each // for each section
> 

Done.

> @@ +476,5 @@
> > +
> > +      //Set title of document
> > +      content.document.title = article.title;
> > +
> > +      //Set base URI of document
> 
> Maybe add a comment here saying that the print preview code will read this
> value to populate the URL field in print settings so that it doesn't show
> "about:blank".
> 

Done.

> @@ +504,5 @@
> > +
> > +      //Create style element for header div and import aboutReaderControls.css
> > +      let controlHeaderStyle = content.document.createElement("style");
> > +      controlHeaderStyle.setAttribute("scoped", "");
> > +      controlHeaderStyle.textContent = "@import url(\"chrome://global/skin/aboutReaderControls.css\");";
> 
> Out of curiosity, why is aboutReaderControls.css needed?
> 

The only reason why we added this one was because we need a specific CSS rule for the header element (title and author). You can see at http://searchfox.org/mozilla-central/source/toolkit/themes/shared/aboutReaderControls.css#55 that it defines cool properties for it, i.e., font size, margins, etc. We can add some of these specific rules in another file and don't import aboutReaderControls.css at all. We could embed these rules in aboutReaderContent.css per say, that deals with rules for the content/elements part. 

> ::: toolkit/themes/shared/aboutReaderContent.css
> @@ +43,5 @@
> >  #moz-reader-content.line-height9 {
> >    line-height: 2.6em;
> >  }
> >  
> > +@media print {
> 
> Still interested in knowing why we need this file included, and these
> changes.

Reader Mode has its own way when presenting the article, and the margins are really big. By default, the print output doesn't look good. So, the reason why we added this is because we need to define the right margin space for these elements when printing. 

Since this file deals with CSS rules for the content part itself, not body, it sounded reasonable to add @media print queries for these elements on this file in specific. We also added some rules in aboutReader.css that deals with body-related queries.

However, we could define every @media print rule that we want in aboutReader.css only. But, it sounds more cohesive to separate body-related and element-related rules (aboutReader.css vs aboutReaderContent.css).

What do you think?

Comment 65

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

(In reply to Matheus Longaray from comment #64)
> 
> Yes, it will work, and even for different cases like when opening print
> preview from a new tab (about:blank) or a page that's already in reader mode
> (about:reader?url=). We are leveraging a work that's already being done by
> reader mode in background for every tab
> (http://searchfox.org/mozilla-central/source/toolkit/components/reader/
> ReaderMode.jsm#163). So I think we don't need to be worried about sending a
> message to the parent if the document is simplify-able or not. What do you
> think?
> 

Okay, sounds good.


> The only reason why we added this one was because we need a specific CSS
> rule for the header element (title and author). You can see at
> http://searchfox.org/mozilla-central/source/toolkit/themes/shared/
> aboutReaderControls.css#55 that it defines cool properties for it, i.e.,
> font size, margins, etc. We can add some of these specific rules in another
> file and don't import aboutReaderControls.css at all. We could embed these
> rules in aboutReaderContent.css per say, that deals with rules for the
> content/elements part. 

Ah, I see. In that case, how about we add a new CSS file for just the rules that we need here for the header elements. We can store it in toolkit/components/ printing/content (an entry for the new file will be needed at toolkit/components/printing/jar.mn). Maybe call it simplifyMode.css or something. The header of that new file will need the MPL license block, and should probably include a comment defining what the styles are being used for.

> Reader Mode has its own way when presenting the article, and the margins are
> really big. By default, the print output doesn't look good. So, the reason
> why we added this is because we need to define the right margin space for
> these elements when printing. 
> 
> Since this file deals with CSS rules for the content part itself, not body,
> it sounded reasonable to add @media print queries for these elements on this
> file in specific. We also added some rules in aboutReader.css that deals
> with body-related queries.
> 
> However, we could define every @media print rule that we want in
> aboutReader.css only. But, it sounds more cohesive to separate body-related
> and element-related rules (aboutReader.css vs aboutReaderContent.css).
> 
> What do you think?

I think the changes to aboutReaderContent.css make sense - let's take them.

Comment 66

[Matheus Longaray (:mlongaray)]

Attachment: WIP9: Simplified tab using Reader Mode primitives directly

This patch provides the simplify feature using Reader Mode primitives directly while addressing Mike's concerns.

Comment 67

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

Comment on attachment 8759217 [details] [diff] [review]
WIP9: Simplified tab using Reader Mode primitives directly

Review of attachment 8759217 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. I think we can land this, assuming margaret is still okay with the style changes in aboutReader*.css

I think we should land this preffed off for now, and have some folk test it a bit to see if we can iron out any kinks before we try flipping it on by default.

Great job on this, Matheus! Long-haul work, but you got there!

No need to update your patch unless margaret thinks the CSS needs tweaks. If not, I can fix the last few bits and land it for you.

::: browser/app/profile/firefox.js
@@ +436,5 @@
>  
>  pref("browser.ctrlTab.previews", false);
>  
> +// Whether we should display simplify page checkbox on print preview UI
> +pref("print.use_simplify_page", true);

Let's keep this preffed off until we've got UX sign-off. We can land this patch, and they can test with it preffed on, and when we're all happy with it, we'll flip it on.

::: toolkit/components/printing/content/printUtils.js
@@ +580,5 @@
>        navToolbox.parentNode.insertBefore(printPreviewTB, navToolbox);
>        printPreviewTB.initialize(ppBrowser);
>  
> +      // Enable simplify page checkbox when the page is an article
> +      if (this._sourceBrowser.isArticle)

I guess this can go wrong if the _sourceBrowser is still in the midst of computing whether or not the content is an article, but that can probably be follow-up work.

::: toolkit/content/browser-content.js
@@ +582,5 @@
> +      readerContent.style.display = "block";
> +
> +      // Here we tell the parent that we have parsed the document successfully
> +      // using ReaderMode primitives and we are able to enter on preview mode.
> +      sendAsyncMessage('Printing:Preview:ReaderModeReady');

Double-quotes please.

Comment 68

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #67)
> Comment on attachment 8759217 [details] [diff] [review]
> WIP9: Simplified tab using Reader Mode primitives directly
> 
> Review of attachment 8759217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me. I think we can land this, assuming margaret is still
> okay with the style changes in aboutReader*.css
> 
> I think we should land this preffed off for now, and have some folk test it
> a bit to see if we can iron out any kinks before we try flipping it on by
> default.
> 
> Great job on this, Matheus! Long-haul work, but you got there!
> 
> No need to update your patch unless margaret thinks the CSS needs tweaks. If
> not, I can fix the last few bits and land it for you.
>

That's awesome! It feels great to get it done and to contribute to Mozilla.

Though I've been the one communicating through IRC and here, I'd like to mention Thauã Silveira ([e-mail]) who has also worked with me on this.

If you need me to update the patch, please let me know.

Looking forward to finally land this feature :)

> ::: browser/app/profile/firefox.js
> @@ +436,5 @@
> >  
> >  pref("browser.ctrlTab.previews", false);
> >  
> > +// Whether we should display simplify page checkbox on print preview UI
> > +pref("print.use_simplify_page", true);
> 
> Let's keep this preffed off until we've got UX sign-off. We can land this
> patch, and they can test with it preffed on, and when we're all happy with
> it, we'll flip it on.
> 

PS: I handed over a Mac build to Christopher Arnold with this feature on by default so he could pass it around to UX folks. Hoping everything goes well.

> ::: toolkit/components/printing/content/printUtils.js
> @@ +580,5 @@
> >        navToolbox.parentNode.insertBefore(printPreviewTB, navToolbox);
> >        printPreviewTB.initialize(ppBrowser);
> >  
> > +      // Enable simplify page checkbox when the page is an article
> > +      if (this._sourceBrowser.isArticle)
> 
> I guess this can go wrong if the _sourceBrowser is still in the midst of
> computing whether or not the content is an article, but that can probably be
> follow-up work.
> 
> ::: toolkit/content/browser-content.js
> @@ +582,5 @@
> > +      readerContent.style.display = "block";
> > +
> > +      // Here we tell the parent that we have parsed the document successfully
> > +      // using ReaderMode primitives and we are able to enter on preview mode.
> > +      sendAsyncMessage('Printing:Preview:ReaderModeReady');
> 
> Double-quotes please.

Comment 69

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

Comment on attachment 8759217 [details] [diff] [review]
WIP9: Simplified tab using Reader Mode primitives directly

Actually, given comment 41, I'm going to clear this off margaret's plate.

I'll flip the pref and change the quotes before landing.

Comment 70

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

Attachment: Patch for landing (r=mconley)

This adds a checkbox to the print preview toolbar for simplifying
a document before printing.

This feature is currently disabled by default, hidden behind the
print.use_simplify_page pref.

MozReview-Commit-ID: HOtkuEQnJFg

Comment 71

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

Comment on attachment 8759776 [details] [diff] [review]
Patch for landing (r=mconley)

I've applied the last piece of feedback regarding the double-quotes and pref value to mlongaray's patch. This is what's going to be checked in.

Comment 72

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

https://hg.mozilla.org/integration/fx-team/rev/dc5692c66a028e98980527c9a637932b992874b2
Bug 962433 - "Use Reader Mode for printing articles" [r=mconley]

Comment 73

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/dc5692c66a02
"Use Reader Mode for printing articles" [r=mconley]

Comment 74

[Carsten Book [:Tomcat]]

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

Comment 75

[Ryan VanderMeulen [:RyanVM]]

We should get QA to look at this as well before preffing it on by default.

Comment 76

[Sylvestre Ledru [:Sylvestre]]

Release Note Request (optional, but appreciated)
[Why is this notable]: New feature 
[Suggested wording]: Improved printing
[Links (documentation, blog post, etc)]: We cannot link to this page http://www.ghacks.net/2016/06/17/firefox-49-simpify-page-printing/ but this is a great content.

Comment 77

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes using "Improved printing by using the Reader Mode"

Comment 78

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

sylvestre:

Hey - I saw that this got added to the 49 Beta release notes, but this feature landed disabled by default. It's only enabled by default in 50.

Sorry, I didn't notice your relnote-firefox? in comment 46 until now - my bad for not alerting you sooner. :/

Is it possible to remove that entry from the release notes?

Comment 79

[Sylvestre Ledru [:Sylvestre]]

moved to 50, thanks!

Comment 80

[Saheda Reza [:Antora]]

I have reproduced this bug on firefox nightly according to(2014-01-21)

The Fixing bug is verified on Latest Developer Edition, Beta.
Build ID   :20160805004022
User Agent :Mozilla/5.0 (Windows NT 10.0; rv:50.0) Gecko/20100101 Firefox/50.0

Tested OS--Windows10 32bit

Comment 81

[Andrei Vaida [:avaida]]

[Test Plan]:
https://wiki.mozilla.org/QA/Simplify_Page

Comment 82

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Simplify page was turned off for 50. I've removed this from 50 release notes.

Comment 83

[Sylvestre Ledru [:Sylvestre]]

Mike, do you know when/if we are planning to ship it? Thanks

Comment 84

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #83)
> Mike, do you know when/if we are planning to ship it? Thanks

This feature is currently enabled up to early beta on Windows, and
enabled on Nightly-only for Linux. QA did a pass, and we found some bugs
that are preventing us from enabling the feature by default for Windows.
Those blocking bugs are:

1: Bug 1306294 - Restarting browser while Simplify Page mode on,
restores 2 new empty tabs

2: Bug 1306299 - First transition to Simplify page will display no images

Bug 1306299 was recently fixed on 53. Bug 1306294 was marked fixed, but
has caused a regression (bug 1323987). We're in the process of reviewing
a patch in bug 1323987 which should address the regression.

If these 2 land, I suspect with Product and QA's approval, we'll be able
to turn this feature on in 53 by default on Windows. For 52 and 51, I
believe this feature will be disabled on release. I suppose there is a
slight chance we could uplift the fixes to 52, but I guess we'll cross
that bridge when we get there.

So bug 1306299 is where most HP activity is currently occurring. I hope
that helps! Let me know if you have further questions.

Comment 85

[Liz Henry (:lizzard) (relman/hg->git project)]

Mike, Is this planned to be on by default in beta 53? If so, I can add a release note.

Comment 86

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

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #85)
> Mike, Is this planned to be on by default in beta 53? If so, I can add a
> release note.

Jury still out. If we want it in 53, we need to uplift bug 1323987, and I think we should only do that if we figure out what might be a regression caused by that patch happening in bug 1342849.

Comment 87

[Julien Cristau [:jcristau]]

Moved the relnote flag over to bug 1362526 since that's where this feature is being enabled.