Closed Bug 1006714 Opened 8 years ago Closed 8 years ago

'Find' fails to search whole PDF with pdf.js when opened in a new tab

Categories

(Firefox :: PDF Viewer, defect)

29 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox31 + verified
firefox32 --- verified

People

(Reporter: m8r-x3rq311, Assigned: unusualtears)

References

Details

(Keywords: regression, Whiteboard: [pdfjs-f-fixed-upstream] https://github.com/mozilla/pdf.js/pull/4753)

Attachments

(2 files)

Re-report of bug 941177 with more accurate bug behavior description.

Steps to reproduce:
1. Open a PDF in a new tab (example PDF links below)
2. Search for a term near the bottom of the PDF

Actual results:
For Link 1 where pdf.js is client side, term is not found
For Link 2 where pdf.js is server side, term _is_ found _only_ when using pdf.js's built-in search (magnifying glass icon in toolbar at top of page), however, using Firefox's own find bar (e.g via find-as-you-type) does _not_ find the term.

Expected results:
Term should be found regardless of how far down the docuement it is.

Link 1: http://www.hupfeld-software.de/files/Dynasys-Handbuch.pdf
Link 2: http://mozilla.github.io/pdf.js/web/viewer.html
Version: unspecified → 29 Branch
note: that this is broken only for "open in new tab" only; it works fine for "open in new window", regular hyperlink click opening or typing address in the address bar.
None of the events are called when PDF viewer is opened in new tab.

http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#718

Looks like it's a regression:

Last good nightly: 2013-07-07
First bad nightly: 2013-07-08

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f8a08d0a1b2a&tochange=17a47dcef75d

Suspect is
f29cb83f66bd	Adam Dane [:hobophobe] — Bug 537013 - Make the find bar exist on a per-tab basis. r=dao

Are we getting wrong gFindBar at http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#919 ?
Flags: needinfo?(unusualtears)
Attached patch Naive patchSplinter Review
Yes, it will need to use |tabbrowser.getFindBar(someTab)| to get the correct findbar: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#158

gFindBar only points to the active tab's findbar now.

Same looks to be true for lines 322, 323, and 440 of PdfStreamConverter.jsm as well.

I've attached a simple/untested patch. Basically adds a function |getFindBar| to lookup the tabbrowser, tab, and finally use that to get the findbar. I'm not very familiar with the PDF.jsm code, so it may be better to e.g., cache the findbar if it will be gotten frequently, etc.
Flags: needinfo?(unusualtears)
Unfortunately the patch will not work as is, since it prevents the PDF Viewer from loading with the following error in the console: "browser.getTabBrowser is not a function (PdfStreamConverter.jsm:66)".
Comment on attachment 8418444 [details] [diff] [review]
Naive patch

>+  var browser = getChromeWindow(domWindow);

getChromeWindow returns the chrome window, not a browser.
Thanks for the feedback!

As PDF.js is a github project I submitted a pull request there: https://github.com/mozilla/pdf.js/pull/4753
We might want to try to uplift this patch (without the test though) to FF31.
Assignee: nobody → unusualtears
Whiteboard: [pdfjs-f-fixed-upstream] https://github.com/mozilla/pdf.js/pull/4753
Target Milestone: Firefox 31 → ---
Depends on: 1007627
Set to New as per above comments.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Adam, could you fill the uplift request? Thanks
Flags: needinfo?(unusualtears)
Attached patch Patch for upliftSplinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): per-tab findbar, bug 537013
User impact if declined: Finding in PDFs displayed via PDF.js will not always work.
Testing completed (on m-c, etc.): Two weeks on m-c with one minor issue (bug 1008450, intermittent test failure caused in combination with findbar code that's new to m-c, but that's been fixed on m-c and wouldn't affect Aurora).
Risk to taking this patch (and alternatives if risky): Low risk, as this only changes how the extension locates the findbar (score-or-so of lines changed in one file that is only used by the extension).
String or IDL/UUID changes made by this patch: None
Attachment #8427259 - Flags: approval-mozilla-aurora?
Flags: needinfo?(unusualtears)
Attachment #8427259 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/46f6b49cc290

Fixed on trunk by bug 1007627.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
bug 1008450 might need to uplifted as well
Keywords: verifyme
Bug 1008450 should not need uplift because it was caused by a combination of this bug fix and another code change (bug 257061) that's new to mozilla-central/not on Aurora.
Reproduced in nightly 2014-05-06.
Verified fixe 32.0a1 (2014-05-25), Win 7 x64
Status: RESOLVED → VERIFIED
Depends on: 1016308
No longer depends on: 1016308
Reproduced in Nightly (2014-05-06), verified as fixed on Windows 7 64bit, Mac OS X 10.9.4 and Ubuntu 13.04 64bit using Firefox 31 beta 6.
You need to log in before you can comment on or make changes to this bug.