Last Comment Bug 1306299 - First transition to Simplify page will display no images
: First transition to Simplify page will display no images
Status: VERIFIED FIXED
:
Product: Toolkit
Classification: Components
Component: Printing (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla52
Assigned To: Matheus Longaray (:mlongaray)
: Bogdan Maris, QA [:bogdan_maris]
: Mike Conley (:mconley)
Mentors:
Depends on:
Blocks: 962433
  Show dependency treegraph
 
Reported: 2016-09-29 07:11 PDT by Bogdan Maris, QA [:bogdan_maris]
Modified: 2017-01-19 10:32 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
wontfix
affected
verified
verified


Attachments
Screencast showing the issue (6.97 MB, video/mp4)
2016-09-29 07:11 PDT, Bogdan Maris, QA [:bogdan_maris]
no flags Details
WIP1: Rely on MozAfterPaint event when simplifying (1.29 KB, patch)
2016-10-27 14:08 PDT, Matheus Longaray (:mlongaray)
no flags Details | Diff | Splinter Review
WIP2: Rely on STATE_STOP and the next MozAfterPaint event when simplifying (3.42 KB, patch)
2016-10-31 06:27 PDT, Matheus Longaray (:mlongaray)
mconley: review+
mconley: feedback+
Details | Diff | Splinter Review
WIP3: Fix nits (3.44 KB, patch)
2016-11-01 14:00 PDT, Matheus Longaray (:mlongaray)
mconley: review+
Details | Diff | Splinter Review

Description User image Bogdan Maris, QA [:bogdan_maris] 2016-09-29 07:11:48 PDT
Created attachment 8796173 [details]
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 User image Mike Conley (:mconley) 2016-09-29 07:22:41 PDT
Hey Bogdan, is this reproducible by using normal Reader Mode as well?
Comment 2 User image Bogdan Maris, QA [:bogdan_maris] 2016-09-29 23:47:31 PDT
(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 User image Mike Conley (:mconley) 2016-10-03 07:35:35 PDT
Great, thanks Bogdan.
Comment 4 User image Ritu Kothari (:ritu) 2016-10-11 15:55:24 PDT
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 User image Bogdan Maris, QA [:bogdan_maris] 2016-10-12 04:49:24 PDT
(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 User image Ritu Kothari (:ritu) 2016-10-13 12:51:35 PDT
We've made a decision to turn off "Simplify Page" feature on Fx50. This is now a wontfix for that version.
Comment 7 User image Matheus Longaray (:mlongaray) 2016-10-17 10:48:38 PDT
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 User image Mike Conley (:mconley) 2016-10-20 13:17:50 PDT
(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 User image Matheus Longaray (:mlongaray) 2016-10-21 09:31:02 PDT
(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 User image Mike Conley (:mconley) 2016-10-27 08:22:45 PDT
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 User image Mike Conley (:mconley) 2016-10-27 08:24:28 PDT Comment hidden (obsolete)
Comment 12 User image Mike Conley (:mconley) 2016-10-27 08:25:33 PDT
Argh, sorry, that PrintHelper.js thing doesn't do what I said. Sorry, looking for a better example now...
Comment 13 User image Mike Conley (:mconley) 2016-10-27 08:28:21 PDT
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 User image Matheus Longaray (:mlongaray) 2016-10-27 14:08:09 PDT
Created attachment 8805264 [details] [diff] [review]
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 User image Matheus Longaray (:mlongaray) 2016-10-27 14:15:12 PDT
(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 User image Mike Conley (:mconley) 2016-10-28 11:35:09 PDT
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 User image Matheus Longaray (:mlongaray) 2016-10-31 06:27:53 PDT
Created attachment 8805931 [details] [diff] [review]
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 User image Mike Conley (:mconley) 2016-10-31 13:37:39 PDT
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 User image Mike Conley (:mconley) 2016-10-31 13:37:51 PDT
Thanks, by the way!
Comment 20 User image Matheus Longaray (:mlongaray) 2016-11-01 05:08:04 PDT
(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 User image Mike Conley (:mconley) 2016-11-01 12:33:34 PDT
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 User image Matheus Longaray (:mlongaray) 2016-11-01 14:00:11 PDT
Created attachment 8806496 [details] [diff] [review]
WIP3: Fix nits

This patch addresses some of Mike's last concerns.
Comment 23 User image Mike Conley (:mconley) 2016-11-04 12:35:32 PDT
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 25 User image Matheus Longaray (:mlongaray) 2016-11-08 04:46:13 PST
Are we good with those results?
Comment 26 User image Mike Conley (:mconley) 2016-11-09 07:34:35 PST
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 User image Pulsebot 2016-11-09 10:13:25 PST
Pushed by ryanvm@gmail.com:
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 User image Carsten Book [:Tomcat] 2016-11-10 08:08:35 PST
https://hg.mozilla.org/mozilla-central/rev/5598e1579a1f
Comment 29 User image Matheus Longaray (:mlongaray) 2016-12-16 10:53:12 PST
Hey Bogdan, could you please test and verify this bug?
Comment 30 User image Bogdan Maris, QA [:bogdan_maris] 2016-12-19 07:12:56 PST
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.

Note You need to log in before you can comment on or make changes to this bug.