Simplified Format fails to load the article (PDF) in print preview
Categories
(Toolkit :: Reader Mode, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr78 | --- | unaffected |
firefox-esr91 | --- | verified |
firefox90 | --- | unaffected |
firefox91 | --- | wontfix |
firefox92 | --- | verified |
firefox93 | --- | verified |
People
(Reporter: csasca, Assigned: Gijs)
References
(Regression)
Details
(Keywords: regression)
Attachments
(2 files)
154.68 KB,
image/png
|
Details | |
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr91+
|
Details | Review |
Affected versions
- Firefox 91.0b3
- Firefox 92.0a1
Affected platforms
- macOS 10.14
- Windows 10 & 8.1
- Ubuntu 18.04
Steps to reproduce
- Launch Firefox
- Access a PDF sample which has simplified mode option
- Click on Print and select More Settings in Print preview
- Select Format from "Original" to "Simplified"
Expected result
- The page is converted in simplified mode
Actual result
- "Failed to load article from page" message is shown in Print Preview
Regression range
- Will see for a regression
Additional notes
- The issue can be seen in the attachment
Reporter | ||
Updated•4 years ago
|
Updated•4 years ago
|
Comment 1•4 years ago
|
||
I've managed to reproduce the issue on Windows 10x64, and then investigated for the change that made this behavior possible:
2021-07-20T09:08:08.651000: DEBUG : Found commit message:
Bug 1713628 - Treat notBefore in the future of signed XPI files as valid + tests r=keeler
This patch also includes unit tests for this + previous untested cases
(bug 1713628, bug 1267318 and bug 1548973).
The tool to generate the test cases (zip files) has also been updated
because it has been broken by changes from bug 1699294.
Differential Revision: https://phabricator.services.mozilla.com/D119802
Confirmed and reg window provided.
Updated•4 years ago
|
Comment 2•4 years ago
|
||
The reported regression range is incorrect.
The feature from the "Simplified" checkbox was recently introduced in bug 1666247. I downloaded the first build with that patch from https://archive.mozilla.org/pub/firefox/nightly/2021/07/2021-07-08-09-18-56-mozilla-central/ (referenced at https://hg.mozilla.org/mozilla-central/rev/8df8fb3d2e0e) and can still reproduce the issue.
The checkbox is shown when isReader
is true. The reader button does appear in the location bar of the PDF file when I press the mouse button anywhere in the document (merely focusing it?), but upon clicking on that button the following error appears:
TypeError: can't access property "catch", ReaderMode.parseDocument(...) is null AboutReaderChild.jsm:50:45
After refreshing the page, the reader view button doesn't appear (and neither does the Simplified checkbox).
Comment 3•4 years ago
|
||
I notice that every time I run mozregression GUI I have different results:
Regression 2: Bug 1695195 - Set SandboxReporter profiler thread name r=jld
Regression 3: Bug 1719727 - Change "Personalize" on New Tab to gear icon. r=prathiksha,fluent-reviewers,Gijs,thecount
Regression 4: Bug 1720595 - Don't install dump_syms during mach bootstrap. r=firefox-build-system-reviewers,nalexander
Bug 1719229 - broke the install by accident, but configure takes care of
it by default since bug 1716911, bug 1716912 or bug 1717585 depending on
the platform, so we can just stop installing it from mach bootstrap.
Regression 5: Bug 1720696: Only fetch mMsaa from non-null AccessibleWraps r=eeejay
Comment 4•4 years ago
|
||
The inconsistent regression output is surprising.
What do you use to decide whether the bug was triggered?
The ideally observed behavior is that a print preview is shown when Simplified mode is used.
In comment 2 I noted that the bug either happened (as reported), or the Simplified checkbox didn't appear (after refreshing the page, in relation with the Reader mode button in the location bar).
Comment 5•4 years ago
•
|
||
I used mozregresssion GUI to find tthe regression and I introduce the following data:
Last known good build: 14-07-2021 (build without Simplified mode)
First known bad build : 15-07-2021(build with Simplified mode)
I observed that your bug is related to this one but mozregression didn't get anything of it on my runs.
Thank you for your help, Rob Wu.
Updated•4 years ago
|
Updated•3 years ago
|
Comment 6•3 years ago
|
||
Gijs should this be reassigned to reader mode (see comment 2, there's a TypeError when toggling reader mode on that pdf)?
Assignee | ||
Comment 7•3 years ago
|
||
(In reply to Julien Cristau [:jcristau] from comment #6)
Gijs should this be reassigned to reader mode (see comment 2, there's a TypeError when toggling reader mode on that pdf)?
Reader mode doesn't work on PDFs and print preview shouldn't be offering it. I don't know why it does, and that's the first thing that should be worked out.
if (
!Readerable.shouldCheckUri(doc.documentURIObject) ||
!Readerable.shouldCheckUri(doc.baseURIObject, true)
) {
this.log("Reader mode disabled for URI");
return null;
}
is the implementation of the method call that leads to a type error. The call to _readerParse
is to an async function, whereas parseDocument
is not async, so the return value would always be a promise (it may be a rejected one, but not a non-promise). The only way that the type error happens is if the initial condition quoted above passes and parseDocument
thus directly returns null - I'm guessing that it gets the PDF.js resource
URI. Looks like that's because reader mode checks both the document and base URI, and isProbablyReaderable only checks the document URI.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 8•3 years ago
|
||
Comment 10•3 years ago
|
||
bugherder |
Assignee | ||
Comment 11•3 years ago
|
||
Comment on attachment 9236816 [details]
Bug 1720877 - ensure we do not offer reader mode for PDFs by checking base URI as well as document URI when checking for reader-ability, r?robwu
Beta/Release Uplift Approval Request
- User impact if declined: Broken print preview and/or reader mode behaviour
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: Yes
- If yes, steps to reproduce: See comment 0
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): 1-line JS change to add a condition that we check later anyway; checking it earlier means we don't end up with broken UI.
- String changes made/needed: Nope
Assignee | ||
Updated•3 years ago
|
Comment 12•3 years ago
|
||
Comment on attachment 9236816 [details]
Bug 1720877 - ensure we do not offer reader mode for PDFs by checking base URI as well as document URI when checking for reader-ability, r?robwu
Approved for 92.0b6.
Comment 13•3 years ago
|
||
bugherder uplift |
Updated•3 years ago
|
Reporter | ||
Comment 14•3 years ago
|
||
Verified that the simplified option is no longer available in pdfs. Tests were performed on Windows 10, macOS 10.15.7 and Ubuntu 20.
Comment 15•3 years ago
|
||
Comment on attachment 9236816 [details]
Bug 1720877 - ensure we do not offer reader mode for PDFs by checking base URI as well as document URI when checking for reader-ability, r?robwu
Approved for 91.1esr also.
Comment 16•3 years ago
|
||
bugherder uplift |
Reporter | ||
Comment 17•3 years ago
|
||
Verified the fix on 91.1esr as well. Tests were performed on Windows 7, macOS 10.15.7 and Ubuntu 20.
Description
•