Closed Bug 1440638 Opened 6 years ago Closed 6 years ago

Notify the Simplify Page user if any error occurs inside the Reader View mode

Categories

(Toolkit :: Printing, defect)

60 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox60 --- verified
firefox61 --- verified

People

(Reporter: asoncutean, Assigned: mlongaray)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

[Affected versions]: 
- 60.0a1 (2018-02-22)

[Affected platforms]:
- Ubuntu 16.04 (x86 and x64)
- Ubuntu 14.04 x64
- Windows 10 x64

[Steps to reproduce]:
1. Launch Firefox
2. Go to http://www.spiegel.de/fotostrecke/gertrude-pressburger-ich-bin-zweimal-geboren-worden-fotostrecke-157700.html
3. Enter Reader view mode and observe the message 
4. Exit the Reader View mode
5. Print Preview the page and click on the Simplify Page button
6. Observe the Print preview notification

[Expected result]:
- The user should be notify if any error occurred inside the Reader View page.

[Actual result]:
Step [3]: “Failed to load article from page”  error message is displayed.
Step [6]: The user is stuck with a  progress bar notification ( https://goo.gl/KfT8iF ).

[Regression range]:
No regression because this is an enhancement.
This seems like something we can recover from. mlongaray, do you have time to look at this?
Flags: needinfo?(mlongaray)
Yes, I'll get the cycles to look at it sometime this week. In!
Flags: needinfo?(mlongaray)
Great, thanks!
Assignee: nobody → mlongaray
Mike, I've taken a stab at this, see what I found below.

Calling ReaderMode.parseDocument() at https://searchfox.org/mozilla-central/source/toolkit/content/browser-content.js#623 does not throw any errors, but returns a null result. Then, we access properties for what we expect to be the right object and that causes a type error at https://searchfox.org/mozilla-central/source/toolkit/content/browser-content.js#662 breaking javascript execution. Thus, the web progress listener instantiated at https://searchfox.org/mozilla-central/source/toolkit/content/browser-content.js#628 does not get called, and the user gets stuck with the print progress notification bar.

So, I think, ideally we'd need to handle a null object and tell parent reader mode could not parse the article via https://searchfox.org/mozilla-central/source/toolkit/content/browser-content.js#639. Parent would then fallback to the default printout (no simplify), inform the user somehow, and disable simplify checkbox for that preview session.

I see a way to fallback to default preview output and disable simplify, but I'm not aware of how we could display a notification to the user.

What do you think? Sounds reasonable? How could we notify the user on preview mode?
Flags: needinfo?(mconley)
(In reply to Matheus Longaray (:mlongaray) from comment #4)
> What do you think? Sounds reasonable? How could we notify the user on
> preview mode?

When ReaderMode fails to parse the document, it shows a message:

"Failed to load article from page".

Could we render something similar for the Simplify Page document?
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #5)
> (In reply to Matheus Longaray (:mlongaray) from comment #4)
> > What do you think? Sounds reasonable? How could we notify the user on
> > preview mode?
> 
> When ReaderMode fails to parse the document, it shows a message:
> 
> "Failed to load article from page".
> 
> Could we render something similar for the Simplify Page document?

Yes, we could render that, we'd only need to jam it into the DOM of the simplified tab (same way we do when Reader Mode returns the parsed article object)
Flags: needinfo?(mconley)
Note 1: parsing issue is now fixed after PR https://github.com/mozilla/readability/pull/423 was merged in Readability.js (as informed before by Gijs at https://public.etherpad-mozilla.org/p/Simplify_Page_PreliminaryStatus).

Note 2: comment #5 would definitely be easier to implement (and I agree), although I'm a bit concerned regarding UX when compared to comment #4. What we could do is evaluate if this bug is corner case, and decide what's really worth implementing, effort-wise (more complexity for comment #4). Gijs, do you see this kind of issue very often*? Where reader mode tells it can read/parse the web page but it fails when it tries to? Thoughts?

Mike, what do you recommend?

(*): found reader mode parsing result data at https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2018-03-01&keys=__none__!__none__!__none__&max_channel_version=beta%252F59&measure=READER_MODE_PARSE_RESULT&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2018-01-22&table=0&trim=1&use_submission_date=0 - error rate is around 8% for beta 59.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Matheus Longaray (:mlongaray) from comment #7)

I think, ideally, we should try to reduce the probability of evaluating a document as readable and then failing to parse it to zero.

Until then, I suspect a UX similar to what we use for ReaderMode is probably sufficient. Certainly it is better than a popup modal dialog (which is what Printing tends to fall back to).
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #8)
> (In reply to Matheus Longaray (:mlongaray) from comment #7)
> 
> I think, ideally, we should try to reduce the probability of evaluating a
> document as readable and then failing to parse it to zero.
> 
> Until then, I suspect a UX similar to what we use for ReaderMode is probably
> sufficient. Certainly it is better than a popup modal dialog (which is what
> Printing tends to fall back to).

Sounds good! Let's ship it then. I'll try to send a patch for review in the next few days. Thanks, Mike!
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #8)
> (In reply to Matheus Longaray (:mlongaray) from comment #7)
> 
> I think, ideally, we should try to reduce the probability of evaluating a
> document as readable and then failing to parse it to zero.

FWIW, my understanding is that updating reader mode in mozilla-central to the version that includes PR 423 from readability would get this to happen, at least on desktop. I don't think (off-hand, I haven't double-checked and I'm still kinda not 100% healthy so take with appropriate amounts of salt) that there are other potential causes for this on desktop (on mobile, there's a limit to the size of document we can handle because mobile - but that shouldn't affect us here). I think that would be straightforward to do, and probably the best way of resolving this - the "it broke" message is super terrible.

Of course, belt and suspenders is a thing, at least in programming, so having 'something' for simplify page printing may still  be wise...
Mike, I've sent a patch for review. Please let me know if that's what was envisioned with comment #5 and comment #8.
See Also: → 1444082
Comment on attachment 8956853 [details]
Bug 1440638 - Render error message for the Simplify Page document.

https://reviewboard.mozilla.org/r/225804/#review232848

This looks great, mlongaray! Would it be possible to write a test for this? Do we know of a reliably way to produce a document that'll appear to be readable, but will produce a parse error when run through the readability library?

::: toolkit/content/browser-content.js:656
(Diff revision 1)
>          ]),
>        };
>  
>        // Here we QI the docShell into a nsIWebProgress passing our web progress listener in.
> -      let webProgress =  docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +      let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> -                                 .getInterface(Ci.nsIWebProgress);
> +        .getInterface(Ci.nsIWebProgress);

Nit: I actually find the previous formatting more readable, myself.
Attachment #8956853 - Flags: review?(mconley)
Comment on attachment 8956853 [details]
Bug 1440638 - Render error message for the Simplify Page document.

https://reviewboard.mozilla.org/r/225804/#review232848

Yes, let's write a new test!

I don't know fully how the Reader Mode's parsing logic actually works, but we could use a non-article document (e.g., https://searchfox.org/mozilla-central/source/toolkit/components/reader/test/readerModeNonArticle.html) and trick the browser to think the document is actually parseable by setting isArticle flag (https://searchfox.org/mozilla-central/source/toolkit/components/printing/content/printUtils.js#570). Then, Reader Mode would return null when simplify page is clicked. Or maybe, we could stub ReaderMode.parseDocument method to return null for a parseable document (although I'm not sure how to accomplish this within our mochitests).

Would that fulfill what you first thought of? We could also ask Gijs if there's some special article document he can think of that we could use here to induce the parsing error.
Flags: needinfo?(mconley)
(In reply to Matheus Longaray (:mlongaray) from comment #14)
> Comment on attachment 8956853 [details]
> Bug 1440638 - Render error message for the Simplify Page document.
> 
> https://reviewboard.mozilla.org/r/225804/#review232848
> 
> Yes, let's write a new test!
> 
> I don't know fully how the Reader Mode's parsing logic actually works, but
> we could use a non-article document (e.g.,
> https://searchfox.org/mozilla-central/source/toolkit/components/reader/test/
> readerModeNonArticle.html) and trick the browser to think the document is
> actually parseable by setting isArticle flag
> (https://searchfox.org/mozilla-central/source/toolkit/components/printing/
> content/printUtils.js#570). Then, Reader Mode would return null when
> simplify page is clicked. Or maybe, we could stub ReaderMode.parseDocument
> method to return null for a parseable document (although I'm not sure how to
> accomplish this within our mochitests).
> 
> Would that fulfill what you first thought of? We could also ask Gijs if
> there's some special article document he can think of that we could use here
> to induce the parsing error.

I think the first of your plans sounds sensible. There used to be more documents that triggered parsing errors.  bug 1444082 reduced that number dramatically, and at this point I'm not aware of any documents that would both (a) pass `isProbablyReaderable` (ie cause it to return `true`) and (b) fail to actually render in reader mode.
Note that, case in point, the document referenced in comment #0 renders OK for me in reader mode on current nightly. :-)
Flags: needinfo?(mconley)
First plan sent for review. Test passed locally. Triggered mochitest-bc tests on try for double checking.
Comment on attachment 8956853 [details]
Bug 1440638 - Render error message for the Simplify Page document.

https://reviewboard.mozilla.org/r/225804/#review233596

This is a lovely patch, thank you so much! Also, thank you for the excellent test! Very well written and easy to follow.
Attachment #8956853 - Flags: review?(mconley) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea46c0d87382
Render error message for the Simplify Page document. r=mconley
ni?ing myself to potentially file a new bug to deal with --verify errors on the new test.
Flags: needinfo?(mconley)
https://hg.mozilla.org/mozilla-central/rev/ea46c0d87382
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: needinfo?(mconley)
AIUI bug 1444082 fixed the specific issue (comment 16), so we may not need this in 60.
(In reply to Julien Cristau [:jcristau] from comment #23)
> AIUI bug 1444082 fixed the specific issue (comment 16), so we may not need
> this in 60.

That's what I thought, but it seems some of it still happens - e.g. the testcase in bug 1394941 still reproduces for me. It's on my list to investigate, but nobody (including me) is really actively working on reader mode given the priority list across the org, so I haven't been able to make time to do this yet.
(In reply to :Gijs from comment #24)
> (In reply to Julien Cristau [:jcristau] from comment #23)
> > AIUI bug 1444082 fixed the specific issue (comment 16), so we may not need
> > this in 60.
> 
> That's what I thought, but it seems some of it still happens - e.g. the
> testcase in bug 1394941 still reproduces for me. It's on my list to
> investigate, but nobody (including me) is really actively working on reader
> mode given the priority list across the org, so I haven't been able to make
> time to do this yet.

Mike, do you think we should just ask to uplift this to 60? At first I thought this patch would have l10n impact, but on closer inspection it doesn't look like it...
Flags: needinfo?(mconley)
(In reply to :Gijs from comment #25)
> Mike, do you think we should just ask to uplift this to 60? At first I
> thought this patch would have l10n impact, but on closer inspection it
> doesn't look like it...

My only reservation with that is the accompanying test still fails the TV test (bug 1447152), which is a Tier 2 problem, but means I suppose that the test is a little flake-y still.

It's certainly low-risk though. I'll request uplift and see what happens.
Flags: needinfo?(mconley)
Comment on attachment 8956853 [details]
Bug 1440638 - Render error message for the Simplify Page document.

Approval Request Comment
[Feature/Bug causing the regression]:

Simplify Print / Readability

[User impact if declined]:

Users can visit some pages that present as viewable in Reader Mode, and can be Simplified in Print Preview, but upon attempting to Simplify the page, an error dialog comes up and a progress window doesn't close automatically.

This error is recoverable by the user by dismissing the error dialog, and closing the progress dialog manually, but it's still not an amazing experience.

[Is this code covered by automated tests?]:

Yes.

[Has the fix been verified in Nightly?]:

Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 

No, I don't think so.

[List of other uplifts needed for the feature/fix]:

Bug 1445837 (test only), which patched the test to fix an intermittent issue.

[Is the change risky?]:

No, it's not. The only thing I'll point out is bug 1447152, which suggests that this test still has intermittent failure problems (but rarer ones).

[Why is the change risky/not risky?]:

See above.

[String changes made/needed]:

None - this shares / reuses a string from Reader Mode.
Attachment #8956853 - Flags: approval-mozilla-beta?
Comment on attachment 8956853 [details]
Bug 1440638 - Render error message for the Simplify Page document.

"simplify page" error handling, beta60+

Note that this should be uplifted together with bug 1445837.
Attachment #8956853 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I can confirm this fix on Firefox 60.0b10 (20180404171943) and Firefox 61.0a1 (2018-04-04) under Windows 10 x64 and Ubuntu 16.04 x 32, by using the link from bug 1394941 (since the one mentioned in comment 0 is not showing the error anymore).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: