1299405 - [jsplugins][UI] Implement presentation mode

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect)

Description

[Luke Chang [:lchang] (inactive)]

Let's implement presentation mode (a.k.a fullscreen mode).

Comment 1

[Luke Chang [:lchang] (inactive)]

Attachment: Pull Request #84

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.

Comment 2

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1299405 - [jsplugins][UI] Implement presentation mode.

Review commit: https://reviewboard.mozilla.org/r/103228/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103228/

Comment 3

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8810303 [details] [review]
Pull Request #84

Changed to MozReview.

Comment 4

[Evelyn Hung]

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.

Comment 5

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8824896 [details]
Bug 1299405 - [jsplugins][UI] Implement presentation mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103228/diff/1-2/

Comment 6

[Luke Chang [:lchang] (inactive)]

Hi Evelyn,

I've added the comments. Thanks for your review.

Comment 7

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8824896 [details]
Bug 1299405 - [jsplugins][UI] Implement presentation mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103228/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae0f6512ab35
[jsplugins][UI] Implement presentation mode. r=evelyn

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ae0f6512ab35