The default bug view has changed. See this FAQ.

First transition to Simplify page will display no images

VERIFIED FIXED in Firefox 52

Status

()

Toolkit
Printing
P3
normal
VERIFIED FIXED
6 months ago
2 months ago

People

(Reporter: bogdan_maris, Assigned: mlongaray)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox49 unaffected, firefox50 wontfix, firefox51 affected, firefox52 verified, firefox53 verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 months ago
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.
Hey Bogdan, is this reproducible by using normal Reader Mode as well?
Flags: needinfo?(bogdan.maris)
(Reporter)

Comment 2

6 months ago
(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.
Flags: needinfo?(bogdan.maris)
Great, thanks Bogdan.
Priority: -- → P3
(Reporter)

Updated

6 months ago
Summary: First transition to Simplify page will display no images no images → First transition to Simplify page will display no images

Comment 4

6 months ago
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.
Flags: needinfo?(bogdan.maris)
(Reporter)

Comment 5

6 months ago
(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).
Flags: needinfo?(bogdan.maris)

Comment 6

6 months ago
We've made a decision to turn off "Simplify Page" feature on Fx50. This is now a wontfix for that version.
status-firefox50: affected → wontfix
(Assignee)

Comment 7

5 months ago
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).
Flags: needinfo?(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.
Flags: needinfo?(mconley)
(Assignee)

Comment 9

5 months ago
(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.
Flags: needinfo?(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.
Flags: needinfo?(mconley)
Comment hidden (obsolete)
Argh, sorry, that PrintHelper.js thing doesn't do what I said. Sorry, looking for a better example now...
Flags: needinfo?(mlongaray)
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?
Flags: needinfo?(mlongaray)
(Assignee)

Comment 14

5 months ago
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.
Flags: needinfo?(mlongaray)
Attachment #8805264 - Flags: feedback?(mconley)
(Assignee)

Comment 15

5 months ago
(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 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.
Attachment #8805264 - Flags: feedback?(mconley)
(Assignee)

Comment 17

5 months ago
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.
Attachment #8805264 - Attachment is obsolete: true
Attachment #8805931 - Flags: feedback?(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?
Attachment #8805931 - Flags: feedback?(mconley) → feedback+
Thanks, by the way!
(Assignee)

Comment 20

5 months ago
(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).
(Assignee)

Updated

5 months ago
Attachment #8805931 - Flags: review?(mconley)
Assignee: nobody → mlongaray
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.
Attachment #8805931 - Flags: review?(mconley) → review+
(Assignee)

Comment 22

5 months ago
Created attachment 8806496 [details] [diff] [review]
WIP3: Fix nits

This patch addresses some of Mike's last concerns.
Attachment #8806496 - Flags: review?(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!
Attachment #8806496 - Flags: review?(mconley) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=538279b92217
(Assignee)

Comment 25

5 months ago
Are we good with those results?
Flags: needinfo?(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"
Flags: needinfo?(mconley)
Keywords: checkin-needed
Attachment #8805931 - Attachment is obsolete: true

Comment 27

5 months ago
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
Keywords: checkin-needed

Comment 28

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5598e1579a1f
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 29

3 months ago
Hey Bogdan, could you please test and verify this bug?
Flags: needinfo?(bogdan.maris)
(Reporter)

Comment 30

3 months ago
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.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
status-firefox53: --- → verified
Flags: needinfo?(bogdan.maris)
(Assignee)

Updated

2 months ago
Blocks: 962433
You need to log in before you can comment on or make changes to this bug.