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.

Sign up for GitHub

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

Jump to bottom

Fix shifted Moveable in RTL mode with scrollbar #7968

Closed
wants to merge 2 commits into from
Closed

Fix shifted Moveable in RTL mode with scrollbar #7968

wants to merge 2 commits into from

Conversation

miina
Copy link
Contributor

@miina miina commented Jun 17, 2021 •
edited

Summary

Considers the scrollbar when in RTL mode and the user has zoomed in enough for the vertical scrollbar to be visible.

Relevant Technical Choices

To-do

User-facing changes

Before After
Screenshot 2021-06-17 at 12 56 20 Screenshot 2021-06-17 at 12 55 03

Testing Instructions

  1. Switch to RTL mode
  2. Add any element
  3. Zoom to 200% (or enough for the vertical scrollbar to appear)
  4. Select the element
  5. Verify that the selection box matches the element borders and is not out of sync with it.

QA

See above.

Checklist

  • This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This PR contains automated tests (unit, integration, and/or e2e) to verify the code works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #7638

@miina miina added Type: Bug Something isn't working Accessibility Changes that impact accessibility and need corresponding review (e.g. markup changes). Group: Selection Pod: Prometheus labels Jun 17, 2021
@miina miina requested a review from merapi June 17, 2021 10:00
@miina miina self-assigned this Jun 17, 2021
@google-cla google-cla bot added the cla: yes label Jun 17, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jun 17, 2021 •
edited

Size Change: +105 B (0%)

Total Size: 2.13 MB

ℹ️ View Unchanged
Filename Size Change
assets/css/carousel-view-rtl.css 701 B 0 B
assets/css/carousel-view.css 701 B 0 B
assets/css/edit-story-rtl.css 277 B 0 B
assets/css/edit-story.css 277 B 0 B
assets/css/stories-dashboard-rtl.css 276 B 0 B
assets/css/stories-dashboard.css 276 B 0 B
assets/css/vendors-edit-story-rtl.css 706 B 0 B
assets/css/vendors-edit-story.css 706 B 0 B
assets/css/web-stories-block-rtl.css 3.21 kB 0 B
assets/css/web-stories-block.css 3.24 kB 0 B
assets/css/web-stories-embed-rtl.css 288 B 0 B
assets/css/web-stories-embed.css 288 B 0 B
assets/css/web-stories-list-styles-rtl.css 2.24 kB 0 B
assets/css/web-stories-list-styles.css 2.25 kB 0 B
assets/css/web-stories-theme-style-twentyeleven-rtl.css 102 B 0 B
assets/css/web-stories-theme-style-twentyeleven.css 102 B 0 B
assets/css/web-stories-theme-style-twentyfifteen-rtl.css 251 B 0 B
assets/css/web-stories-theme-style-twentyfifteen.css 251 B 0 B
assets/css/web-stories-theme-style-twentyfourteen-rtl.css 287 B 0 B
assets/css/web-stories-theme-style-twentyfourteen.css 287 B 0 B
assets/css/web-stories-theme-style-twentyseventeen-rtl.css 288 B 0 B
assets/css/web-stories-theme-style-twentyseventeen.css 288 B 0 B
assets/css/web-stories-theme-style-twentysixteen-rtl.css 224 B 0 B
assets/css/web-stories-theme-style-twentysixteen.css 224 B 0 B
assets/css/web-stories-theme-style-twentyten-rtl.css 143 B 0 B
assets/css/web-stories-theme-style-twentyten.css 143 B 0 B
assets/css/web-stories-theme-style-twentytwelve-rtl.css 256 B 0 B
assets/css/web-stories-theme-style-twentytwelve.css 256 B 0 B
assets/css/web-stories-theme-style-twentytwenty-rtl.css 86 B 0 B
assets/css/web-stories-theme-style-twentytwenty.css 86 B 0 B
assets/css/web-stories-theme-style-twentytwentyone-rtl.css 263 B 0 B
assets/css/web-stories-theme-style-twentytwentyone.css 264 B 0 B
assets/css/web-stories-widget-rtl.css 484 B 0 B
assets/css/web-stories-widget.css 484 B 0 B
assets/js/carousel-view.js 3.72 kB 0 B
assets/js/chunk-fonts-********************.js 45.4 kB 0 B
assets/js/chunk-web-stories-template-0-********************.js 465 B 0 B
assets/js/chunk-web-stories-template-10-********************.js 8.11 kB 0 B
assets/js/chunk-web-stories-template-12-********************.js 423 B 0 B
assets/js/chunk-web-stories-template-16-********************.js 9.73 kB 0 B
assets/js/chunk-web-stories-template-18-********************.js 459 B 0 B
assets/js/chunk-web-stories-template-22-********************.js 8.54 kB 0 B
assets/js/chunk-web-stories-template-24-********************.js 450 B 0 B
assets/js/chunk-web-stories-template-28-********************.js 6.99 kB 0 B
assets/js/chunk-web-stories-template-30-********************.js 461 B 0 B
assets/js/chunk-web-stories-template-34-********************.js 6.06 kB 0 B
assets/js/chunk-web-stories-template-36-********************.js 488 B 0 B
assets/js/chunk-web-stories-template-4-********************.js 7.86 kB 0 B
assets/js/chunk-web-stories-template-40-********************.js 7.84 kB 0 B
assets/js/chunk-web-stories-template-42-********************.js 445 B 0 B
assets/js/chunk-web-stories-template-46-********************.js 8.65 kB 0 B
assets/js/chunk-web-stories-template-6-********************.js 478 B 0 B
assets/js/chunk-web-stories-textset-0-********************.js 5.29 kB 0 B
assets/js/chunk-web-stories-textset-1-********************.js 6.81 kB 0 B
assets/js/chunk-web-stories-textset-2-********************.js 7.92 kB 0 B
assets/js/chunk-web-stories-textset-3-********************.js 15.4 kB 0 B
assets/js/chunk-web-stories-textset-4-********************.js 4.43 kB 0 B
assets/js/chunk-web-stories-textset-5-********************.js 5.71 kB 0 B
assets/js/chunk-web-stories-textset-6-********************.js 5.5 kB 0 B
assets/js/chunk-web-stories-textset-7-********************.js 10.4 kB 0 B
assets/js/edit-story.js 524 kB +65 B (0%)
assets/js/lightbox.js 986 B 0 B
assets/js/stories-dashboard.js 436 kB -24 B (0%)
assets/js/tinymce-button.js 3.48 kB 0 B
assets/js/vendors-chunk-ffmpeg-********************.js 5.65 kB 0 B
assets/js/vendors-edit-story-********************.js 61.5 kB 0 B
assets/js/vendors-edit-story-stories-dashboard-********************.js 245 kB 0 B
assets/js/vendors-web-animations-js-********************.js 14.6 kB 0 B
assets/js/web-stories-activation-notice.js 65.1 kB 0 B
assets/js/web-stories-block.js 571 kB +64 B (0%)
assets/js/web-stories-embed.js 493 B 0 B
assets/js/web-stories-widget.js 984 B 0 B

compressed-size-action

merapi
merapi reviewed Jun 17, 2021
View reviewed changes
Copy link
Contributor

@merapi merapi left a comment

Choose a reason for hiding this comment

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

Chrome looks good, but can you check Safari? Looks like this for me (is ok on main):
image

@merapi
Copy link
Contributor

merapi commented Jun 17, 2021

I've noticed some RTL+zoom oddities, can you also see all of them?

Screen.Recording.2021-06-17.at.23.26.17.mov

@miina
Copy link
Contributor Author

miina commented Jun 18, 2021

I've noticed some RTL+zoom oddities, can you also see all of them?

Hm, didn't see it before but yes, now I can see that the horizontal scrolling doesn't seem to be working.

And yes, looks like it was working as expected before in Safari and now is not. It's also not working as expected in Firefox anymore so looks like this fix should be for Chrome only.

I'll move this back to the backlog and expand the scope to generally fixing the issues with Zoom.

@swissspidy swissspidy added the Status: Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jul 13, 2021
@swissspidy
Copy link
Collaborator

@miina is this one still relevant?

@miina
Copy link
Contributor Author

miina commented Sep 9, 2021

I think we can close, thanks for checking.

@miina miina closed this Sep 9, 2021
@swissspidy swissspidy deleted the fix/7638-layers-sync branch September 9, 2021 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merapi merapi merapi left review comments

Assignees

@miina miina

Labels
Accessibility Changes that impact accessibility and need corresponding review (e.g. markup changes). Group: Selection Status: Stale Gives the original author opportunity to update before closing. Can be reopened as needed. Type: Bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RTL: Frame and display layer not in sync when zoomed
3 participants
@miina @merapi @swissspidy