Closed Bug 1299405 Opened 3 years ago Closed 3 years ago

[jsplugins][UI] Implement presentation mode

Categories

(Firefox :: PDF Viewer, defect)

defect
Not set

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).
Blocks: 1299406
No longer blocks: 1299406
Whiteboard: [pdfium-viewer-mvpscope]
Summary: [Mortar/PDF] Implement presentation mode → [jsplugins][UI] Implement presentation mode
Assignee: nobody → lchang
Depends on: 1311256
Attached file Pull Request #84 (obsolete) —
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)
Blocks: 1317228
Group: mozilla-employee-confidential
Status: NEW → ASSIGNED
Comment on attachment 8810303 [details] [review]
Pull Request #84

Changed to MozReview.
Attachment #8810303 - Attachment is obsolete: true
Attachment #8810303 - Flags: review?(ehung)
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+
Hi Evelyn,

I've added the comments. Thanks for your review.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ae0f6512ab35
[jsplugins][UI] Implement presentation mode. r=evelyn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ae0f6512ab35
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.