1306299 - First transition to Simplify page will display no images

Status: VERIFIED FIXED

(Toolkit :: Printing, defect, P3)

Description

[Bogdan Maris, Desktop QA]

Attachment: Screencast showing the issue

[Affected versions]:
- Firefox 50 beta 2
- latest Nightly 52.0a1

[Affected platforms]:
- Windows 7 64 bit
- Windows 10 64 bit
- Ubuntu 16.04 32 bit and 64 bit

[Unaffected platforms]:
- Mac OS X - since print preview is not available on this OS

[Steps to reproduce]:
1. Start Firefox
2. Visit a page with Reader View (ex: wikipedia.org)
3. Go to File -> Print Preview
4. Check "Simplify Page"
5. Uncheck "Simplify Page"
6. Check "simplify Page"

[Expected result]:
- Article is displayed as in Reader View.

[Actual result]:
- No images are displayed after first entering simplify page mode (Step 4). After step 6 images are shown again.

[Regression range]:
- This is not a recent regression, it reproduces on the old Nightly from 2016-06-06 where Simplify page was introduced. 

[Additional notes]:
- Screencast showing the issue attached.

Comment 1

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

Hey Bogdan, is this reproducible by using normal Reader Mode as well?

Comment 2

[Bogdan Maris, Desktop QA]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #1)
> Hey Bogdan, is this reproducible by using normal Reader Mode as well?

Nope, this is only repro with Simplify Page. Reader Mode loads images from the first use.

Comment 3

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

Great, thanks Bogdan.

Comment 4

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

Hi Bogdan, I was unable to play the attached screen cast in Beta50 or Aurora51. On DevEd, I get a message that the video file is corrupt.

I tried the STR for this page https://en.wikipedia.org/wiki/Ruth_Bader_Ginsburg and noticed that in reader mode, I don't see the picture of Ruth and when I do the print -> print preview -> safe mode, I don't see the images either and it looks just like the reader mode. I am using 50.0b5 and win10. So this may not be a 100% repro I think.

Comment 5

[Bogdan Maris, Desktop QA]

(In reply to Ritu Kothari (:ritu) from comment #4)
> Hi Bogdan, I was unable to play the attached screen cast in Beta50 or
> Aurora51. On DevEd, I get a message that the video file is corrupt.

I made a .gif with this, it shouldn't be a problem now to watch (the other video was made on Ubuntu so maybe it has something to do with it's format, don't know. Chrome played it well, Firefox did not).

Gif: https://dl.dropboxusercontent.com/u/109148197/Simplify%20Page%20teeest.gif

> I tried the STR for this page
> https://en.wikipedia.org/wiki/Ruth_Bader_Ginsburg and noticed that in reader
> mode, I don't see the picture of Ruth and when I do the print -> print
> preview -> safe mode, I don't see the images either and it looks just like
> the reader mode. I am using 50.0b5 and win10. So this may not be a 100%
> repro I think.

So I investigated this case and drawn some conclusions:
- I could reproduce this issue every time, at the beginning of each session or at each first use of Simplify on a specific article. Restarting Firefox and again using Simplify for the first time will show this bug, using it multiple times on the same article will not.
- Indeed the first image with Ruth does not appear (that would be a bug in Reader mode i guess), but if you scroll through the whole article it can be seen that not a single image is loaded. After using Simplify the second time in the session, it will show images on page 4, 5 and 6 (exactly as Reader View).

Comment 6

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

We've made a decision to turn off "Simplify Page" feature on Fx50. This is now a wontfix for that version.

Comment 7

[Matheus Longaray (:mlongaray)]

Hey Mike,

I've been investigating this and it seems to be a synchronicity issue.

Here's the code that tells parent that our reader mode content is ready for preview: http://searchfox.org/mozilla-central/source/toolkit/content/browser-content.js#588

So, in some pages, by the time we enter on preview, images are not loaded/rendered yet in our "simplified browser" and so preview does not contain any image. If we generate a second transition, let's say by changing layout on preview, the images would've already finished loading/rendering in background and so preview will contain images as expected. (I could confirm this by setting a time out prior to sending "Printing:Preview:ReaderModeReady")

Do you know if there's a way to attach an event listener so we only tell parent we're ready after we've received such event? If not, what do you suggest?

PS: I also noticed that in some pages, when using Reader Mode, an image can become visible right after the text was shown (loading/rendering time).

Comment 8

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

(In reply to Matheus Longaray from comment #7)
> 
> Do you know if there's a way to attach an event listener so we only tell
> parent we're ready after we've received such event? If not, what do you
> suggest?
> 

Hm, okay, interesting. Do you know if the readystatechange event fires after parseDocument is called and the DOM is injected? Or perhaps DOMContentLoaded?

If so, perhaps we could use one of those.

Comment 9

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - (high latency on reviews and needinfos) from comment #8)
> Hm, okay, interesting. Do you know if the readystatechange event fires after
> parseDocument is called and the DOM is injected? Or perhaps DOMContentLoaded?
> 
> If so, perhaps we could use one of those.

Hey Mike,

readystatechange and DOMContentLoaded events are only fired after we create our simplified browser: http://searchfox.org/mozilla-central/source/toolkit/components/printing/content/printUtils.js#519.

After parseDocument is called and the DOM is injected, I could not capture any event (at least for these two events). I think these events are only fired when we finish loading a tab, not when we modify the DOM of such tab.

Wondering how we could solve this one. Do you have any other suggestion? Thank you.

Comment 10

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

The other thing that comes to mind is that we could add an nsIWebProgressListener to the simplify browser docShell, and wait for it to change state to STATE_STOP, so that we know that all resources are done loading.

Comment 11

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

Here's an example of us doing something somewhat similar for Fennec in PDF generation:

http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/mobile/android/chrome/content/PrintHelper.js#45

Is that enough to get you moving here, mlongaray?

Comment 12

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

Argh, sorry, that PrintHelper.js thing doesn't do what I said. Sorry, looking for a better example now...

Comment 13

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

Okay, here's an example of where we attach an nsIWebProgressListener, and wait for the STATE_STOP before doing something else:

http://searchfox.org/mozilla-central/source/toolkit/components/gfx/content/gfxFrameScript.js

Does that look workable, mlongaray?

Comment 14

[Matheus Longaray (:mlongaray)]

Attachment: WIP1: Rely on MozAfterPaint event when simplifying

This patch relies on MozAfterPaint event before telling parent that our reader mode content is ready for preview.

Comment 15

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - (digging out of review / needinfo backlog) from comment #13)
> Okay, here's an example of where we attach an nsIWebProgressListener, and
> wait for the STATE_STOP before doing something else:
> 
> http://searchfox.org/mozilla-central/source/toolkit/components/gfx/content/
> gfxFrameScript.js
> 
> Does that look workable, mlongaray?

It does look workable, yes.

I think we don't even need to attach an nsIWebProgressListener to the simplify browser docShell. I noticed from the example you provided that only relying on MozAfterPaint event would be enough in our case. I attached a working patch so you can give a better look.

Thanks for you guidance.

Comment 16

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

Comment on attachment 8805264 [details] [diff] [review]
WIP1: Rely on MozAfterPaint event when simplifying

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

As discussed in IRC, we'll wait until STATE_STOP and then the next MozAfterPaint.

Comment 17

[Matheus Longaray (:mlongaray)]

Attachment: WIP2: Rely on STATE_STOP and the next MozAfterPaint event when simplifying

This patch relies on STATE_STOP and then the next MozAfterPaint event before telling parent that our reader mode content is ready for preview.

Comment 18

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

Comment on attachment 8805931 [details] [diff] [review]
WIP2: Rely on STATE_STOP and the next MozAfterPaint event when simplifying

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

Does this still work if there are no images to load off of the network?

Comment 19

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

Thanks, by the way!

Comment 20

[Matheus Longaray (:mlongaray)]

(In reply to Mike Conley (:mconley) - (digging out of review / needinfo backlog) from comment #18)
> Comment on attachment 8805931 [details] [diff] [review]
> WIP2: Rely on STATE_STOP and the next MozAfterPaint event when simplifying
> 
> Review of attachment 8805931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does this still work if there are no images to load off of the network?

Yes, it does still work. I tested using an article that did not contain images (I even removed every icon/image from the DOM just to make sure).

Comment 21

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

Comment on attachment 8805931 [details] [diff] [review]
WIP2: Rely on STATE_STOP and the next MozAfterPaint event when simplifying

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

Looks good to me. I'm surprised that we get a STATE_STOP / request when just injecting stuff into the DOM, but I trust you that it works. Just two nits below.

::: toolkit/content/browser-content.js
@@ +508,5 @@
> +      let webProgressListener = {
> +        onStateChange: function (webProgress, req, flags, status) {
> +          if (flags & Ci.nsIWebProgressListener.STATE_STOP) {
> +            webProgress.removeProgressListener(webProgressListener);
> +            let domUtils =  content.QueryInterface(Ci.nsIInterfaceRequestor)

Nit - extra space after =

@@ +532,5 @@
> +      };
> +
> +      // Here we QI the docShell into a nsIWebProgress passing our web progress listener in.
> +      let webProgress =  docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                         .getInterface(Ci.nsIWebProgress);

Let's line up the periods, like:

let webProgress =  docShell.QueryInterface(Ci.nsIInterfaceRequestor)
                           .getInterface(Ci.nsIWebProgress);

and do the same thing with the getInterface(Ci.nsIDOMWindowUtils) above.

Comment 22

[Matheus Longaray (:mlongaray)]

Attachment: WIP3: Fix nits

This patch addresses some of Mike's last concerns.

Comment 23

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

Comment on attachment 8806496 [details] [diff] [review]
WIP3: Fix nits

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

Looks okay to me. We should make sure QA tests this too. Thanks mlongaray!

Comment 24

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=538279b92217

Comment 25

[Matheus Longaray (:mlongaray)]

Are we good with those results?

Comment 26

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

I think so, yes. Thanks Matheus!

Author: Matheus Longaray
Bug number: 1306299
Commit message: "Bug 1306299 - Wait for network activity stop and paint in ReaderMode browser before showing the Simplified Print Preview mode. r=mconley"

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5598e1579a1f
Wait for network activity stop and paint in ReaderMode browser before showing the Simplified Print Preview mode. r=mconley

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5598e1579a1f

Comment 29

[Matheus Longaray (:mlongaray)]

Hey Bogdan, could you please test and verify this bug?

Comment 30

[Bogdan Maris, Desktop QA]

Managed to test this on Ubuntu 16.04 32bit, Windows 7 64bit and Windows 10 64bit, and I see that the images appear from the start. Tried this multiple times so we can call this verified fixed. I used latest Nightly 53.0a1 and latest Developer Edition 52.0a2 to test. 
Marking the status of the bug to verified fixed but still waiting for 51 to receive the fix.