Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Editor] Add a z-index in order to draw them in the right order #15196

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

calixteman
Copy link
Contributor

The elements in the annotationEditor layer are rearranged to make them
more accessible, but we must draw them in the order they have been created,
hence this patch adds a z-index to the editors.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will clash with existing tab-indexes used elsewhere in the viewer, note the toolbars.
Hence it seems to me that we need to do something similar as in the annotationLayer and xfaLayer:

@calixteman
Copy link
Contributor Author

This will clash with existing tab-indexes used elsewhere in the viewer, note the toolbars. Hence it seems to me that we need to do something similar as in the annotationLayer and xfaLayer:

* https://github.com/mozilla/pdf.js/blob/408c10b5bbd90381d152d0399588ad2fc62379d0/src/display/annotation_layer.js#L40

* https://github.com/mozilla/pdf.js/blob/de7d1d2167ad55bc215c98e43d4e0dec25db9b60/src/core/xfa/template.js#L125

How are tab-index and z-index related ?

@Snuffleupagus
Copy link
Collaborator

Sorry, trying to do two things at once and ended up completely misreading things!

@@ -221,6 +225,7 @@ class AnnotationEditor {
this.div.className = this.name;
this.div.setAttribute("id", this.id);
this.div.tabIndex = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is where my previous comment applies.

@@ -221,6 +225,7 @@ class AnnotationEditor {
this.div.className = this.name;
this.div.setAttribute("id", this.id);
this.div.tabIndex = 0;
this.div.style.zIndex = this.#zIndex;
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need both the static and the private member?
Could we not simply do the following instead:

Suggested change
this.div.style.zIndex = this.#zIndex;
this.div.style.zIndex = AnnotationEditor._zIndex++;

The elements in the annotationEditor layer are rearranged to make them
more accessible, but we must draw them in the order they have been created,
hence this patch adds a z-index to the editors.
@calixteman
Copy link
Contributor Author

There was a bad interaction between this new z-index and the setIn-Foreground/Background stuff. So I reimplemented the two setters using the new z-index.

@Snuffleupagus
Copy link
Collaborator

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/c53a7a08a6826f8/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

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

Total script time: 4.62 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/c53a7a08a6826f8/output.txt

Total script time: 10.72 mins

  • Integration Tests: FAILED

@Snuffleupagus Snuffleupagus merged commit 5e7eab4 into mozilla:master Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants