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)
Tracking
()
VERIFIED
FIXED
mozilla61
People
(Reporter: asoncutean, Assigned: mlongaray)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
mconley
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
[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.
Comment 1•6 years ago
|
||
This seems like something we can recover from. mlongaray, do you have time to look at this?
Flags: needinfo?(mlongaray)
Assignee | ||
Comment 2•6 years ago
|
||
Yes, I'll get the cycles to look at it sometime this week. In!
Flags: needinfo?(mlongaray)
Assignee | ||
Comment 4•6 years ago
|
||
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)
Comment 5•6 years ago
|
||
(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)
Assignee | ||
Comment 6•6 years ago
|
||
(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)
Assignee | ||
Comment 7•6 years ago
|
||
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)
Comment 8•6 years ago
|
||
(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)
Assignee | ||
Comment 9•6 years ago
|
||
(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)
Comment 10•6 years ago
|
||
(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...
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•6 years ago
|
||
Mike, I've sent a patch for review. Please let me know if that's what was envisioned with comment #5 and comment #8.
Comment 13•6 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mconley)
Comment 15•6 years ago
|
||
(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.
Comment 16•6 years ago
|
||
Note that, case in point, the document referenced in comment #0 renders OK for me in reader mode on current nightly. :-)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mconley)
Assignee | ||
Comment 18•6 years ago
|
||
First plan sent for review. Test passed locally. Triggered mochitest-bc tests on try for double checking.
Comment 19•6 years ago
|
||
mozreview-review |
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+
Comment 20•6 years ago
|
||
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ea46c0d87382 Render error message for the Simplify Page document. r=mconley
Comment 21•6 years ago
|
||
ni?ing myself to potentially file a new bug to deal with --verify errors on the new test.
Flags: needinfo?(mconley)
Comment 22•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ea46c0d87382
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Flags: needinfo?(mconley)
Comment 23•6 years ago
|
||
AIUI bug 1444082 fixed the specific issue (comment 16), so we may not need this in 60.
Comment 24•6 years ago
|
||
(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.
Comment 25•6 years ago
|
||
(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)
Comment 26•6 years ago
|
||
(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 27•6 years ago
|
||
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?
Updated•6 years ago
|
Comment 28•6 years ago
|
||
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+
Comment 29•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/545ac09b18b1
Flags: in-testsuite+
Reporter | ||
Comment 30•6 years ago
|
||
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).
You need to log in
before you can comment on or make changes to this bug.
Description
•