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 dialog box in order to get alt-text data (bug 1844952) #16952

Merged
merged 1 commit into from
Sep 18, 2023

Conversation

calixteman
Copy link
Contributor

Implements the specifications provided by UX for light, dark and HCM modes.

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.

As far as I can tell there's nothing that actually displays this dialog, since there's no JS-code included in the patch, so how would you actually test this?

@calixteman
Copy link
Contributor Author

To just display it: in the console, document.getElementById("altTextDialog").showModal().
We want to have this patch (and another one with a button to trigger this dialog) asap in m-c in order to start the translation of the strings asap.

Copy link
Contributor

@flodolo flodolo left a comment

Choose a reason for hiding this comment

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

There still seems to be discussions over the copy in the doc? I see unresolved comments.

@moz-tools-bot
Copy link
Collaborator

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/2c5f5ce81102f13/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/2c5f5ce81102f13/output.txt

Total script time: 1.45 mins

Published

@Snuffleupagus
Copy link
Collaborator

Unfortunately I'm no longer able to access the Figma link in https://bugzilla.mozilla.org/show_bug.cgi?id=1844952#c12, since I'm now taken to a login page, so I can't really check that the implementation matches the specification.

@Snuffleupagus
Copy link
Collaborator

No longer being able to access the specification, as mentioned above, one question based on testing the preview:
Do we need to have this much total padding in the dialog, since here we get the padding of all dialog-elements from

padding: 15px;
plus the new padding added in and it's not entirely clear to me if it needs to be that much?

@calixteman
Copy link
Contributor Author

No longer being able to access the specification, as mentioned above, one question based on testing the preview: Do we need to have this much total padding in the dialog, since here we get the padding of all dialog-elements from

padding: 15px;

plus the new padding added in


and it's not entirely clear to me if it needs to be that much?

Good catch, thank you.

@Snuffleupagus
Copy link
Collaborator

Did the latest version of the patch accidentally revert the fix for #16952 (comment)?

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Sep 18, 2023

Also, testing the preview above it seems that the descriptionTextarea textArea has both a border and an outline when focused, and the specification seems to suggest that only the border color should change in that case?

@moz-tools-bot
Copy link
Collaborator

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/f04dc70956e505d/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

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

Total script time: 1.51 mins

Published

@Snuffleupagus
Copy link
Collaborator

When clicking on the radio-buttons (or the regular buttons) the focus-outline is displayed, do we want that or should it only apply when using the keyboard?

@calixteman
Copy link
Contributor Author

When clicking on the radio-buttons (or the regular buttons) the focus-outline is displayed, do we want that or should it only apply when using the keyboard?

I used focus-visible and I switched to focus when I played with some radios in the network settings in prefs: focus outline is shown when the radio are clicked, so I just did the same.

@Snuffleupagus
Copy link
Collaborator

I used focus-visible and I switched to focus when I played with some radios in the network settings in prefs: focus outline is shown when the radio are clicked, so I just did the same.

Sure, that seems fine regarding the radio-buttons. However, it also happens when clicking the Cancel/Save buttons in the dialog which isn't the case for buttons in about:preferences.

@Snuffleupagus
Copy link
Collaborator

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

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/05e905489b6bf67/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/05e905489b6bf67/output.txt

Total script time: 2.49 mins

Published

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.

r=me, since this now seems to agree with the specification; thank you!

web/annotation_editor_layer_builder.css Outdated Show resolved Hide resolved
web/annotation_editor_layer_builder.css Outdated Show resolved Hide resolved
web/plop.html Outdated Show resolved Hide resolved
Implement the specifications provided by UX for light, dark and HCM modes.
@calixteman calixteman merged commit 5ac40b7 into mozilla:master Sep 18, 2023
3 checks passed
@calixteman calixteman deleted the alt_text_window branch September 18, 2023 19:49
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.

5 participants