Closed
Bug 1299405
Opened 5 years ago
Closed 4 years ago
[jsplugins][UI] Implement presentation mode
Categories
(Firefox :: PDF Viewer, defect)
Firefox
PDF Viewer
Tracking
()
RESOLVED
FIXED
Firefox 53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: lchang, Assigned: lchang)
References
Details
(Whiteboard: [pdfium-viewer-mvpscope])
Attachments
(1 file, 1 obsolete file)
Let's implement presentation mode (a.k.a fullscreen mode).
Whiteboard: [pdfium-viewer-mvpscope]
Assignee | ||
Updated•4 years ago
|
Summary: [Mortar/PDF] Implement presentation mode → [jsplugins][UI] Implement presentation mode
Assignee | ||
Updated•4 years ago
|
Assignee: nobody → lchang
Assignee | ||
Comment 1•4 years ago
|
||
Hi Evelyn, This patch depends on the workaround of "requestFullscreen" in bug 1311256. However, two patches don't conflict, so I think it's ok to review each other independently. Note that I only implemented the basic fullscreen style in this patch. Keyboard and mouse control will be addressed in the follow-up bugs. Thanks.
Attachment #8810303 -
Flags: review?(ehung)
Assignee | ||
Updated•4 years ago
|
Group: mozilla-employee-confidential
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•4 years ago
|
||
Comment on attachment 8810303 [details] [review] Pull Request #84 Changed to MozReview.
Attachment #8810303 -
Attachment is obsolete: true
Attachment #8810303 -
Flags: review?(ehung)
Comment 4•4 years ago
|
||
mozreview-review |
Comment on attachment 8824896 [details] Bug 1299405 - [jsplugins][UI] Implement presentation mode. https://reviewboard.mozilla.org/r/103228/#review105900 ::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:108 (Diff revision 1) > + set fullscreen(enable) { > + if (this._fullscreenStatus == 'changing' || > + this._fullscreenStatus == (enable ? 'fullscreen' : 'none')) { > + return; > + } > + this._fullscreenStatus = 'changing'; We rely on platform gives us `fullscreenchange` event so we could unlock the changing status here. Which means if, for some reason, we don't get the event, the whole UI will not respond to any view change. I can't thing of a better solution here, but we could at least leave a XXX comment to mention this known issue. ::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:399 (Diff revision 1) > + if (fullscreen) { > + this._previousZoom = this._zoom; > + this._previousFitting = this._fitting; > + this._fitting = 'page-fit'; > + // No need to call "_setZoom" here since "_setPage" is in charge of this > + // in fullscreen mode. nit: No need to call "_setZoom" here because we will deal with zooming case in the "_setPage" below. ::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:411 (Diff revision 1) > + this._fullscreenStatus = fullscreen ? 'fullscreen' : 'none'; > + > + // Reset position to the beginning of the current page. > + this._setPage(currentPage); > + this._refresh(); > + }, 100); nit: better to leave a comment here for this magic number.
Attachment #8824896 -
Flags: review?(ehung) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•4 years ago
|
||
Hi Evelyn, I've added the comments. Thanks for your review.
Assignee | ||
Updated•4 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ae0f6512ab35 [jsplugins][UI] Implement presentation mode. r=evelyn
Keywords: checkin-needed
Comment 9•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae0f6512ab35
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in
before you can comment on or make changes to this bug.
Description
•