Use Reader Mode for printing articles

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: Unfocused, Assigned: mconley)

Tracking

(Depends on 1 bug, {feature})

unspecified
Firefox 49
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, relnote-firefox -, firefox50 fixed)

Details

Attachments

(13 attachments, 10 obsolete attachments)

151.39 KB, image/png
Details
52.62 KB, image/png
Details
889.93 KB, image/png
Details
572.42 KB, image/png
Details
442.62 KB, image/png
Details
432.67 KB, image/png
Details
441.72 KB, image/png
Details
154.64 KB, image/png
Details
152.79 KB, image/png
Details
63.69 KB, image/png
Details
236.48 KB, image/png
Details
49.75 KB, application/zip
Details
26.08 KB, patch
mconley
: review+
Details | Diff | Splinter Review
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

6 years ago
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)
Assignee: nobody → mlongaray
[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.]
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.
Attachment #8725502 - Flags: feedback?(mconley)
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.
Attachment #8725502 - Attachment is obsolete: true
Attachment #8725502 - Flags: feedback?(mconley)
Attachment #8725760 - Flags: feedback?(mconley)
Thanks for this Matheus. Can you attach screenshots of the output you achieve with this patch?
I sure can, Christopher. I'll grab those for you and also attach a few more examples to provide broader understanding.
See tooltip of simplify page checkbox.
See tooltip of simplify page checkbox
See tooltip of simplify page checkbox
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.
Attachment #8725760 - Flags: feedback?(mconley) → feedback+
(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 :)
(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!
(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!
(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?
Flags: needinfo?(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 :)
Flags: needinfo?(margaret.leibovic)
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).
Posted patch WIP3: Partial Refactor (obsolete) — Splinter Review
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).
Attachment #8734478 - Flags: feedback?(mconley)
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
MozReview-Commit-ID: F0BztDWMJUn
Assignee

Updated

3 years ago
Assignee: mlongaray → mconley
Status: NEW → ASSIGNED
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.
Attachment #8734478 - Flags: feedback?(mconley)
(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 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)?
Assignee

Updated

3 years ago
Flags: needinfo?(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?
Assignee: mconley → mlongaray
Flags: needinfo?(mconley) → needinfo?(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.
Flags: needinfo?(mconley)
Flags: needinfo?(mlongaray)
(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.
Flags: needinfo?(mconley)
Posted patch WIP4: Lazy approach (obsolete) — Splinter Review
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.
Attachment #8725760 - Attachment is obsolete: true
Attachment #8734478 - Attachment is obsolete: true
Attachment #8743505 - Flags: feedback?(mconley)
(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 on attachment 8743505 [details] [diff] [review]
WIP4: Lazy approach

Set flag "feedback?" to "review?". Added Mike Conley and Margaret Leibovic as reviewers.
Attachment #8743505 - Flags: review?(mconley)
Attachment #8743505 - Flags: review?(margaret.leibovic)
Attachment #8743505 - Flags: feedback?(mconley)
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.
Attachment #8725894 - Flags: ui-review?(shorlander)
Attachment #8725898 - Flags: ui-review?(shorlander)
Attachment #8725901 - Flags: ui-review?(shorlander)
Small refactor for better and concise messaging.
Attachment #8743505 - Attachment is obsolete: true
Attachment #8743505 - Flags: review?(mconley)
Attachment #8743505 - Flags: review?(margaret.leibovic)
Attachment #8745538 - Flags: review?(mconley)
Attachment #8745538 - Flags: review?(margaret.leibovic)
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.
Attachment #8745538 - Flags: review?(mconley) → review-
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.
Attachment #8745538 - Flags: review?(margaret.leibovic)
(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.
Flags: needinfo?(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
Flags: needinfo?(margaret.leibovic)
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.
Attachment #8745538 - Attachment is obsolete: true
Attachment #8750492 - Flags: review?(mconley)
Sharing a model to better understand all IPC messaging, between parent and child process, related to print preview and reader mode.
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.
(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 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"
Attachment #8750492 - Flags: review?(mconley) → feedback+
(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.
This patch applies a small refactor given Mike Conley's review as well as addresses nits and unnecessary code.
Attachment #8750492 - Attachment is obsolete: true
Attachment #8754032 - Flags: review?(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.
Flags: needinfo?(mlongaray)
Here's a screenshot for an exception caused by calling About Reader.
(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).
Flags: needinfo?(mlongaray)

Comment 52

3 years ago
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.
(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?
Flags: needinfo?(mconley)

Comment 54

3 years ago
(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

3 years ago
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...
(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);
(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).
Flags: needinfo?(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?
Flags: needinfo?(mlongaray)
Sorry, Matheus, not Mathius.
Assignee

Updated

3 years ago
Depends on: 1275570
Posted file 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.
Flags: needinfo?(mlongaray)
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.
Flags: needinfo?(mlongaray)
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.
Attachment #8754032 - Attachment is obsolete: true
Attachment #8754032 - Flags: review?(mconley)
Flags: needinfo?(mlongaray)
Attachment #8758055 - Flags: review?(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.
Attachment #8758055 - Flags: review?(mconley) → review-
(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?
Assignee

Updated

3 years ago
Flags: needinfo?(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.
Flags: needinfo?(mconley)
This patch provides the simplify feature using Reader Mode primitives directly while addressing Mike's concerns.
Attachment #8758055 - Attachment is obsolete: true
Attachment #8759217 - Flags: review?(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.
Attachment #8759217 - Flags: review?(mconley)
Attachment #8759217 - Flags: review+
Attachment #8759217 - Flags: feedback?(margaret.leibovic)
(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 (thaua@hp.com) 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 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.
Attachment #8759217 - Flags: feedback?(margaret.leibovic)
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
Assignee

Updated

3 years ago
Attachment #8737422 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Assignee: mlongaray → 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.
Attachment #8759776 - Attachment description: "Use Reader Mode for printing articles" [ → Patch for landing (r=mconley)
Attachment #8759776 - Flags: review+
Assignee

Updated

3 years ago
Attachment #8759217 - Attachment is obsolete: true

Comment 73

3 years ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/dc5692c66a02
"Use Reader Mode for printing articles" [r=mconley]

Comment 74

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dc5692c66a02
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
We should get QA to look at this as well before preffing it on by default.
Flags: needinfo?(rares.bologa)

Updated

3 years ago
Flags: needinfo?(rares.bologa) → needinfo?(ciprian.muresan)
Assignee

Updated

3 years ago
Depends on: 1285544
Assignee

Updated

3 years ago
Depends on: 1285607
Assignee

Updated

3 years ago
Depends on: 1287587
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.
relnote-firefox: --- → ?
Added to the release notes using "Improved printing by using the Reader Mode"
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?
Flags: needinfo?(sledru)
moved to 50, thanks!
Flags: needinfo?(sledru)
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
QA Whiteboard: [bugday-20160803]
Flags: needinfo?(saheda.antora)
Flags: needinfo?(saheda.antora)

Comment 81

3 years ago
testplan
[Test Plan]:
https://wiki.mozilla.org/QA/Simplify_Page
Flags: needinfo?(ciprian.muresan)
Keywords: feature
QA Contact: bogdan.maris

Updated

3 years ago
Depends on: 1308461
Simplify page was turned off for 50. I've removed this from 50 release notes.

Updated

3 years ago
Depends on: 1319067
Depends on: 1329220
Mike, do you know when/if we are planning to ship it? Thanks
Flags: needinfo?(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.
Flags: needinfo?(mconley)
Depends on: 1306294
Depends on: 1306298
Depends on: 1306299
Depends on: 1323987
Depends on: 1336554
Mike, Is this planned to be on by default in beta 53? If so, I can add a release note.
Flags: needinfo?(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.
Flags: needinfo?(mconley)
Depends on: 1343056
Depends on: 1332386
Assignee

Updated

2 years ago
Depends on: 1362526
Moved the relnote flag over to bug 1362526 since that's where this feature is being enabled.
You need to log in before you can comment on or make changes to this bug.