Removed .grab-to-pan-grab:active to resolve toolbars going behind canvas issue (issue 5452)

[Abishek-Jayan]

Removed .grab-to-pan-grab:active to resolve toolbars going behind canvas issue (issue 5452).

While my fix was targeted toward the secondaryToolbar, I noticed that the issue regarding the toolbars going behind the canvas on using the hand tool persisted for the findToolbar as well. This was because the viewerContainer div had alot of unnecessary styling applied to it via .grab-to-pan-grab:active. On applying those rules to .grab-to-pan-grabbing alone the issue was resolved.
I tested the fix extensively in Chrome, Firefox and Brave with no issues seen.

This is a fix for #5452

[Snuffleupagus]

Why not just decrease the value for the HandTool-classes a little bit instead, e.g. to 29000, since for one thing the following comment is now wrong?

pdf.js/web/viewer.css

Line 1376 in 92f7653

z-index: 50000; /* should be higher than anything else in PDF.js! */

That comment probably ought to be updated to something like the following now.

/* Should be higher than anything else in the `#viewerContainer`. */

This is shown to resolve the issue in both Chrome and Brave. Tested Firefox just in case and no issues there either.

Please keep in mind that Firefox is the primary development target, hence that's the browser that you should do most testing in.

[Abishek-Jayan]

@Snuffleupagus as per ur suggestion Ive amended my commit to reduce the hand z-index value to 29000 and updated the comment next to it accordingly. Ive tested it again on Firefox, Chrome and Brave and the new fix holds up. Let me know if I have missed anything.

[Snuffleupagus]

Please re-format the commit message to something like the following (since the first line should be a summary of the changes):

Decrease z-index for .grab-to-pan to resolve toolbars going behind canvas issue (issue 5452)

While my fix was targeted toward the secondaryToolbar, I noticed that the issue regarding
the toolbars going behind the canvas on using the hand tool persisted for the findToolbar
as well. So to fix I just reduced the z-index of the .grab-to-pan-grabbing class to 29000
and updated its comment accordingly.
I tested the fix extensively in Chrome, Firefox and Brave and it seems to be fixed.

[Abishek-Jayan]

@Snuffleupagus reformatted the commit message to your given specifications.

[Snuffleupagus]

Some quick testing shows that this patch leads to another kind of graphical glitch, and while that one may be less annoying than the current behaviour it's still not perfect; so there's apparently a reason for the existing (high) z-index and its comment.
Hence it's clear that this issue is less straightforward than it seems at first, and given that it could be seen as a browser bug (not affecting Firefox) I'm starting to wonder if we should just WONTFIX the referenced issue.

Steps to reproduce:

1. Enable the HandTool.
2. Open the SecondaryToolbar.
3. Click and hold the left mouse-button somewhere over the viewer to start panning.
4. Move the mouse over the (already open) SecondaryToolbar.

Expected behaviour:
The mouse cursor shows the grabbing-cursor until the mouse-button is released.

Actual behaviour:
The mouse cursor is "reset" immediately when the mouse enters the SecondaryToolbar.

[Abishek-Jayan]

I dont think marking this as a WONTFIX for a harmless glitch like that is a good idea. Ill try to find a fix for it.

[Abishek-Jayan]

@Snuffleupagus I found that the issue was happening cuz both the viewerContainer and the .grab-to-pan-grabbing div were being moved to z-index 50k. When I isolated the z-index move to just .grab-to-pan-grabbing it seemed to fix the issue. I tested it on Chrome, Brave and Firefox and it seems to be resolved now.

[Snuffleupagus]

When I isolated the z-index move to just .grab-to-pan-grabbing it seemed to fix the issue.

The "seemed to fix"-part makes me worried about what might break with these changes, since you've not really explained why the .grab-to-pan-grab:active, rule doesn't need a z-index set.

[Abishek-Jayan]

When I isolated the z-index move to just .grab-to-pan-grabbing it seemed to fix the issue.

The "seemed to fix"-part makes me worried about what might break with these changes, since you've not really explained why the .grab-to-pan-grab:active, rule doesn't need a z-index set.

@Snuffleupagus Apologies for that, so from what Ive understood the .grab-to-pan-grab:active rule is supposed to activate when we click and drag on the pdf content with the hand tool. However, when we click and drag, the .grab-to-pan-grabbing rule becomes the div that is overlaying the entire document. The code for the actual movement is done by manipulating the vertical and horizontal scroll bars. In fact, on testing further I havent seen the css rules in the .grab-to-pan-grab:active rule be applied at all when clicking and dragging with the hand tool.

So based on that, I havent detected any need for the grab-to-pan-grab:active rule at all. In fact, when i removed it and just kept all that css in grab-to-pan-grabbing alone, the hand movement still worked fine. I tested it just now in Chrome, Brave and Firefox and have not detected any issues at all. (In the image below i removed the .grab-to-pan-grab:active rule.)

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/c53a7062ed6eef6/output.txt

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.241.84.105:8877/c53a7062ed6eef6/output.txt

Total script time: 1.52 mins

Published

• Viewer: http://54.241.84.105:8877/c53a7062ed6eef6/web/viewer.html
• Viewer (legacy): http://54.241.84.105:8877/c53a7062ed6eef6/legacy/web/viewer.html

[Snuffleupagus]

@Rob--W The line that's being removed here was added by you in PR #4209. I realize that it's almost ten years ago, but would you be able to comment on whether this change is "safe" to make?

[Rob--W]

I'll take a closer look within 2 weeks.

[Rob--W]

@Rob--W The line that's being removed here was added by you in PR #4209. I realize that it's almost ten years ago, but would you be able to comment on whether this change is "safe" to make?

The purpose of the .grab-to-pan-grab:active is that the cursor's affordance indicates that the hand tool has grabbed something, which signals that moving the cursor around can activate the panning effect. This snippet is also seen in the reference implementation of grab-to-pan.css, with the minimal CSS at https://github.com/Rob--W/grab-to-pan.js/blob/49c89e3bf87657132ff35e3bdd8f2c4ef47b01b5/grab-to-pan.css.

Once the mouse has been moved, the browser will compute the appearance based on the overlay that is now under the cursor (.grab-to-pan-grabbing).

PDF.js's viewer.css has more CSS properties beyond the cursor: it has additional styles (position:fixed + inset:0) to ensure that the other HTML elements in the viewer are rendered behind the viewer. This is mainly of importance to the overlay (.grab-to-grab-grabbing), and to a lesser extent .grab-to-grab-grab:active (aka the #viewerContainer element). Without these, the cursor would not have the desired appearance. The other CSS properties are not needed, I have created #16896 for that. The result is equivalent to this PR, except with cursor:grabbing kept.

Earlier versions of this PR tried to tweak the z-index of the overlay: do not do that! Doing so can result in elements being rendered on top of the overlay, which can result in unwanted interaction with that element (e.g. triggering mousemove events) while the cursor is already moving around as part of the Hand Tool's panning operation.