Closed Bug 1317228 Opened 4 years ago Closed 4 years ago

[jsplugins][UI] Implement mouse control in presentation mode

Categories

(Firefox :: PDF Viewer, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: lchang, Assigned: rexboy)

References

Details

(Whiteboard: [pdfium-viewer-mvpscope])

Attachments

(1 file)

Let's make presentation mode support mouse control.
Summary: [jsplugins][UI] Implement mous control in presentation mode → [jsplugins][UI] Implement mouse control in presentation mode
There is a problem here. In PDF.js, we use left-mouse-click to jump to next page in presentation mode. However, it seems that we're unable to know whether the click should be handled by plugin (say, clicking on a link or an input field). I've tried the patch in bug 1273472, but it seems not to fulfill this case. Thus, there are there choices if we can't find a solution for this.

1. Disable in-page links, input feature and other mouse interactions in presentation mode.
2. Let "left-click-to-next-page" only affect when not clicking within the page, i.e. only affect when clicking in the black background area.
3. Remove "left-click-to-next-page" behavior entirely.
Hi Vance,

Since we can't find a proper solution, we probably need UX to decide what I mentioned in comment 1. Could you help for communicating? Thanks.
Flags: needinfo?(vchen)
Group: mozilla-employee-confidential
Hi Stephen, could you help to check and comment ?
There are two kinds of links here:
1) Internal link (which raises goToPage event when clicking)
2) External link (which raises navigate event when clicking)

1) is clickable in PDF.js and 2) is not clickable in PDF.js.
This is still easy to imitate in mortar.

There's a not really fine way to try to overcome the question mentioned in 2):
just go next page on every click and also send click down to plugin.
If the clicking was on a link, PDFium will send GoToPage notification
which brings us to the link specified page next.
The screen may flick once for the next page which shown first though.
(The "clickable/not clickable" I talked about means just presentation mode.)
Assignee: lchang → rexboy
Comment on attachment 8849514 [details]
Bug 1317228 - [Mortar][PDF] Implement mouse and keyboard control in presentation mode. , f=lchang

As a result this patch included a few other stuffs:
- Go Next/Prev page by arrow key in presentation mode.
- Go to specified page when clicking on an internal link.

The main reason for including the second feature is that I need to solve the problem of clicking an internal link under presentation mode. The solution I made can be concluded below.
When a user clicks on an internal link, 
(1) goToPage event is sent out *on mousedown*. Page jumping followed.
(2) Go next page should have been performed *on click*(which means after mouseup),
(3) however if we found page number changed between (1) and (2), that means a internal link is clicked. The action of (2) is cancelled then.

Evelyn&Luke would you review this patch?
Attachment #8849514 - Flags: feedback?(lchang)
by the way I didn't find a good way to prevent mouse selection under presentation mode; unless I add an mousemove listener before runtime hooks its listener, leave it there and block propagation depends on an if(fullscreen){...} statement. But that seems overkill and causes a constant resource waste so I decide not to touch it for now.
Comment on attachment 8849514 [details]
Bug 1317228 - [Mortar][PDF] Implement mouse and keyboard control in presentation mode. , f=lchang

https://reviewboard.mozilla.org/r/122312/#review124756

1) To prevent mouse selection under presentation mode, you can listen to `mousemove` event in the capturing phase as well.
2) I'm fine with handling key control in this bug, but it would be better to handle all keys (PgDn, PgUp, etc) instead of only arrow keys. Otherwise, I would suggest doing that in another bug.
3) The original PDF.js listens to `mousedown` for jumping to the next page. I don't think it matters if we change it to `click` (`mouseup`) but you probably can explain more about why doing that.

::: browser/extensions/mortar/host/pdf/chrome/js/presentation-controller.js:10
(Diff revision 1)
> +    this._viewportController = document.getElementById('viewportController');
> +    this._viewport = viewport;
> +
> +    // We need to catch mousedown earlier than runtime to detect if user clicked
> +    // on an internal link by watching changes of page number.
> +    this._viewportController.addEventListener('mousedown', this);

As our discussion, you can register it in the capturing phase on `window` object so that you don't need to do it in the constructor.

::: browser/extensions/mortar/host/pdf/chrome/js/presentation-controller.js:11
(Diff revision 1)
> +    this._viewport = viewport;
> +
> +    // We need to catch mousedown earlier than runtime to detect if user clicked
> +    // on an internal link by watching changes of page number.
> +    this._viewportController.addEventListener('mousedown', this);
> +    document.addEventListener('fullscreenchange', this);

I guess you forgot to remove this line.

::: browser/extensions/mortar/host/pdf/chrome/js/presentation-controller.js:22
(Diff revision 1)
> +  }
> +
> +  _onFullscreenChange(isFullscreen) {
> +    if (isFullscreen) {
> +      window.addEventListener('keydown', this);
> +      this._viewportController.addEventListener('click', this);

I would recommend to use `viewport.bindUIEvent` so this module doesn't need to know anything about `viewportController`.

::: browser/extensions/mortar/host/pdf/chrome/js/presentation-controller.js:45
(Diff revision 1)
> +    let MOUSE_DOM_DELTA_LINE_MODE = 1;
> +    let MOUSE_PIXELS_PER_LINE = 30;
> +    let MOUSE_LINES_PER_PAGE = 30;
> +
> +    // Converts delta to per-page units
> +    if (evt.deltaMode === MOUSE_DOM_DELTA_PIXEL_MODE) {

In Firefox, we can use the native constant `WheelEvent.DOM_DELTA_PIXEL` directly.

::: browser/extensions/mortar/host/pdf/chrome/js/presentation-controller.js:47
(Diff revision 1)
> +    let MOUSE_LINES_PER_PAGE = 30;
> +
> +    // Converts delta to per-page units
> +    if (evt.deltaMode === MOUSE_DOM_DELTA_PIXEL_MODE) {
> +      delta /= MOUSE_PIXELS_PER_LINE * MOUSE_LINES_PER_PAGE;
> +    } else if (evt.deltaMode === MOUSE_DOM_DELTA_LINE_MODE) {

Ditto. `WheelEvent.DOM_DELTA_LINE`.
Attachment #8849514 - Flags: feedback?(lchang)
Blocking propagation on capturing phase does prevent user from selecting texts, but it also rubs out the cursor (I guess that's because cursor is determined by PDFium and not it loses data it needed). and therefore the possibility of clicking on hyperlink.
So seems we need to choice one and sacrifice another.
Oh wait, this situation in comment 10 dose not appear everytime.
I maybe need to check the reason.
Comment on attachment 8849514 [details]
Bug 1317228 - [Mortar][PDF] Implement mouse and keyboard control in presentation mode. , f=lchang

Patch updated. Please take a look.

By the way I found the keyboard event has something to do with regular mode and maybe it's better to consider them independently, so let's leave them to another bug.
Attachment #8849514 - Flags: feedback?(lchang)
Comment on attachment 8849514 [details]
Bug 1317228 - [Mortar][PDF] Implement mouse and keyboard control in presentation mode. , f=lchang

https://reviewboard.mozilla.org/r/122312/#review124830

Looks good! Thanks.

::: browser/extensions/mortar/host/pdf/chrome/js/presentation-controller.js:5
(Diff revision 2)
> +'use strict';
> +
> +class PresentationController {
> +  constructor(viewport) {
> +    this._viewportController = document.getElementById('viewportController');

We don't need this anymore.
Attachment #8849514 - Flags: feedback?(lchang) → feedback+
Updated with the issue in comment 14. Carrying f+ from Luke.
Comment on attachment 8849514 [details]
Bug 1317228 - [Mortar][PDF] Implement mouse and keyboard control in presentation mode. , f=lchang

https://reviewboard.mozilla.org/r/122312/#review124906

Rubberstamp r+ since Luke has done the review. Thanks!
Attachment #8849514 - Flags: review?(ehung) → review+
Pushed by ehung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/305be9a6c255
[Mortar][PDF] Implement mouse and keyboard control in presentation mode. r=evelyn, f=lchang
https://hg.mozilla.org/mozilla-central/rev/305be9a6c255
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Clear NI as it's been fixed.
Flags: needinfo?(vchen)
You need to log in before you can comment on or make changes to this bug.