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] Change the cursor when we switch to FreeText mode (bug 1787297) #15386

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

calixteman
Copy link
Contributor

No description provided.

cursor: auto;
}

.annotationEditorLayer.freeTextEditing {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the Ink-case we're setting the cursor with an .annotationEditorLayer .inkEditor.editing rule, so do we perhaps want to use similar naming here as well (or are those cases not similar enough to warrant that)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The situation is slightly different from ink stuff.
For ink stuff, when we're editing, we always have a div (with a canvas inside) which covers all the page.
But I'll change that to have something more consistent.

}

.annotationEditorLayer.freeTextEditing {
cursor: var(--editorFreeText-editing-cursor), auto;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you perhaps want to use text as the fallback instead?

Comment on lines +26 to +27
--editorFreeText-editing-cursor: url(images/toolbarButton-editorFreeText.svg)
8 16;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, but this won't work in the components build unfortunately. I can't comment on the all the relevant lines of the diff, but what you want/need is (assuming again that "text" is a reasonable default value):

diff --git a/web/annotation_editor_layer_builder.css b/web/annotation_editor_lay
er_builder.css
index 5251eaeeb..fa8dc8e67 100644
--- a/web/annotation_editor_layer_builder.css
+++ b/web/annotation_editor_layer_builder.css
@@ -20,8 +20,11 @@
   --freetext-padding: 2px;
   /*#if COMPONENTS*/
   --editorInk-editing-cursor: pointer;
+  --editorFreeText-editing-cursor: text;
   /*#else*/
   --editorInk-editing-cursor: url(images/cursor-editorInk.svg) 0 16;
+  --editorFreeText-editing-cursor: url(images/toolbarButton-editorFreeText.svg)
+    8 16;
   /*#endif*/
 }

Comment on lines 107 to 111
if (mode === AnnotationEditorType.FREETEXT) {
this.div.classList.add("freeTextEditing");
} else {
this.div.classList.remove("freeTextEditing");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be shorter/simpler written as:

Suggested change
if (mode === AnnotationEditorType.FREETEXT) {
this.div.classList.add("freeTextEditing");
} else {
this.div.classList.remove("freeTextEditing");
}
this.div.classList.toggle("freeTextEditing", mode === AnnotationEditorType.FREETEXT);

@calixteman
Copy link
Contributor Author

/botio integrationtest

@pdfjsbot
Copy link

pdfjsbot commented Sep 2, 2022

From: Bot.io (Linux m4)


Received

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

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

@pdfjsbot
Copy link

pdfjsbot commented Sep 2, 2022

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

pdfjsbot commented Sep 2, 2022

From: Bot.io (Linux m4)


Success

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

Total script time: 5.00 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Sep 2, 2022

From: Bot.io (Windows)


Failed

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

Total script time: 7.42 mins

  • Integration Tests: FAILED

@calixteman
Copy link
Contributor Author

/botio-windows integrationtest

@pdfjsbot
Copy link

pdfjsbot commented Sep 2, 2022

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/002a43a7cda37cc/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 2, 2022

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/002a43a7cda37cc/output.txt

Total script time: 7.21 mins

  • Integration Tests: FAILED

@calixteman
Copy link
Contributor Author

I ran integration tests locally on windows and I can't reproduce this issue which very unlikely related to this patch.
So I merge it and I'll keep an eye open on those failures.

@calixteman calixteman merged commit c503006 into mozilla:master Sep 2, 2022
@calixteman calixteman deleted the text_cursor branch September 2, 2022 15:58
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