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] Avoid to have a selected stamp annotation on top of the secondary toolbar (bug 1911980) #18793

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

calixteman
Copy link
Contributor

And I changed the permissions on the images to remove the x flag which is useless.

@calixteman
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/38fac76a2e26c94/output.txt

@moz-tools-bot
Copy link
Collaborator

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/7bf1323c56bb6bc/output.txt

Total script time: 9.44 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/38fac76a2e26c94/output.txt

Total script time: 19.35 mins

  • Integration Tests: FAILED

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.

Pretty please test these sort of changes affecting the viewer more thoroughly during development, to save my time have to re-review stuff.

web/viewer.css Show resolved Hide resolved
@calixteman
Copy link
Contributor Author

I'm using the dark mode and this kind of difference is almost invisible for me.

That said, it's unrelated to this patch but I think the sidebar should live either under the toolbar or just after it, because if the user clicks or on the toggle sidebar button and then hit then the focus is moving on the toolbar, then the pages, then the firefox ui and finally the sidebar.
In Edge, the focus moves on the sidebar when in Chrome it moves on the toolbar and then on the sidebar.
I'll file a bug and see what the a11y team thinks about that.

@calixteman calixteman force-pushed the bug1911980 branch 2 times, most recently from da063cc to aab7f77 Compare September 26, 2024 06:01
@calixteman
Copy link
Contributor Author

I think it should safe: finally the patch just changes the current z-index values we've but in keeping the same order and just add a new one for the viewer container to make sure it's behind the sidebar and the toolbar.

@Snuffleupagus
Copy link
Collaborator

Should we perhaps also implement the suggestion from #18793 (comment), although not technically needed here, since having a hard-coded colour seems generally wrong?

@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: 1

Live output at: http://54.241.84.105:8877/0445fda180a9bdc/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/0445fda180a9bdc/output.txt

Total script time: 1.09 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, hopefully we've found all regressions now; thank you.

@calixteman calixteman merged commit 642b9a5 into mozilla:master Sep 26, 2024
6 checks passed
@calixteman calixteman deleted the bug1911980 branch September 26, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants