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

Fix formatting keyboard shortcuts on non-text elements #13299

Merged
merged 4 commits into from May 16, 2023

Conversation

AnuragVasanwala
Copy link
Collaborator

@AnuragVasanwala AnuragVasanwala commented May 16, 2023

Context

The goal of the PR is to fix #13281.

Summary

Editor crashes when using a keyboard shortcut not applicable to a shape element. For example, enter cmd+b, cmd+i or cmd+u on a shape element.

This is happening because call to a callback function hasText has missing round brackets. hasText is never executed and following code-block never executes:

So, it do not return when text is not present.

Relevant Technical Choices

// Previously proposed solution when `hasText` is a useCallback function
- if (!hasText) {
+ if (!hasText()) {

// Newely proposed solution
- const hasText = useCallback(...
+ const hasText = selectedElements.find(({ type }) => type === 'text');

To-do

User-facing changes

Not applicable.

Testing Instructions

This PR can be tested by following these steps:

  1. Add shape element
  2. Click cmd+b
  3. RichText Component will not process non-text element key press and return.

Reviews

No.

Does this PR change what data or activity we track or use?

No.

Does this PR have a legal-related impact?

No.

Checklist

Not applicable.


Fixes #13281

@AnuragVasanwala AnuragVasanwala added Type: Bug Something isn't working P1 High priority, must do soon Group: Canvas Group: Keyboard Shortcuts Keyboard shortcuts + menu Package: Rich Text /packages/rich-text labels May 16, 2023
@AnuragVasanwala AnuragVasanwala added this to the 1.33.0 milestone May 16, 2023
@AnuragVasanwala AnuragVasanwala self-assigned this May 16, 2023
@googleforcreators-bot
Copy link
Collaborator

googleforcreators-bot commented May 16, 2023

Plugin builds for a2a96e8 are ready 🛎️!

@github-actions
Copy link
Contributor

github-actions bot commented May 16, 2023

Size Change: -15 B (0%)

Total Size: 2.71 MB

ℹ️ View Unchanged
Filename Size Change
assets/css/web-stories-block-rtl.css 4.6 kB 0 B
assets/css/web-stories-block.css 4.63 kB 0 B
assets/css/web-stories-carousel-rtl.css 702 B 0 B
assets/css/web-stories-carousel.css 701 B 0 B
assets/css/web-stories-dashboard-rtl.css 657 B 0 B
assets/css/web-stories-dashboard.css 659 B 0 B
assets/css/web-stories-editor-rtl.css 769 B 0 B
assets/css/web-stories-editor.css 771 B 0 B
assets/css/web-stories-embed-rtl.css 655 B 0 B
assets/css/web-stories-embed.css 656 B 0 B
assets/css/web-stories-list-styles-rtl.css 2.39 kB 0 B
assets/css/web-stories-list-styles.css 2.42 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 326 B 0 B
assets/css/web-stories-theme-style-twentytwentyone.css 326 B 0 B
assets/css/web-stories-widget-rtl.css 486 B 0 B
assets/css/web-stories-widget.css 486 B 0 B
assets/js/3248.js 38.3 kB 0 B
assets/js/3780.js 12.9 kB 0 B
assets/js/4422.js 49.3 kB 0 B
assets/js/4652.js 92.7 kB 0 B
assets/js/7357.js 92 B 0 B
assets/js/7418.js 7.97 kB 0 B
assets/js/83.js 4.62 kB 0 B
assets/js/9084.js 196 kB 0 B
assets/js/9750.js 12.8 kB 0 B
assets/js/chunk-colorthief.js 2.62 kB 0 B
assets/js/chunk-ffmpeg.js 5.98 kB 0 B
assets/js/chunk-html-to-image.js 4.51 kB 0 B
assets/js/chunk-media-gallery.js 6.18 kB 0 B
assets/js/chunk-mediainfo.js 96 B 0 B
assets/js/chunk-opentype.js 96 B 0 B
assets/js/chunk-react-calendar.js 11.6 kB 0 B
assets/js/chunk-react-color.js 26 kB 0 B
assets/js/chunk-selfie-segmentation.js 16.3 kB 0 B
assets/js/chunk-web-stories-template-0-metaData.js 546 B 0 B
assets/js/chunk-web-stories-template-0.js 11 kB 0 B
assets/js/chunk-web-stories-template-1-metaData.js 540 B 0 B
assets/js/chunk-web-stories-template-1.js 9.27 kB 0 B
assets/js/chunk-web-stories-template-10-metaData.js 532 B 0 B
assets/js/chunk-web-stories-template-10.js 7.23 kB 0 B
assets/js/chunk-web-stories-template-11-metaData.js 539 B 0 B
assets/js/chunk-web-stories-template-11.js 8.87 kB 0 B
assets/js/chunk-web-stories-template-12-metaData.js 496 B 0 B
assets/js/chunk-web-stories-template-12.js 8.67 kB 0 B
assets/js/chunk-web-stories-template-13-metaData.js 525 B 0 B
assets/js/chunk-web-stories-template-13.js 6.9 kB 0 B
assets/js/chunk-web-stories-template-14-metaData.js 582 B 0 B
assets/js/chunk-web-stories-template-14.js 7.26 kB 0 B
assets/js/chunk-web-stories-template-15-metaData.js 544 B 0 B
assets/js/chunk-web-stories-template-15.js 8.77 kB 0 B
assets/js/chunk-web-stories-template-16-metaData.js 588 B 0 B
assets/js/chunk-web-stories-template-16.js 10.5 kB 0 B
assets/js/chunk-web-stories-template-17-metaData.js 540 B 0 B
assets/js/chunk-web-stories-template-17.js 9.05 kB 0 B
assets/js/chunk-web-stories-template-18-metaData.js 587 B 0 B
assets/js/chunk-web-stories-template-18.js 9.31 kB 0 B
assets/js/chunk-web-stories-template-19-metaData.js 501 B 0 B
assets/js/chunk-web-stories-template-19.js 9.17 kB 0 B
assets/js/chunk-web-stories-template-2-metaData.js 586 B 0 B
assets/js/chunk-web-stories-template-2.js 9.02 kB 0 B
assets/js/chunk-web-stories-template-20-metaData.js 548 B 0 B
assets/js/chunk-web-stories-template-20.js 8.72 kB 0 B
assets/js/chunk-web-stories-template-21-metaData.js 536 B 0 B
assets/js/chunk-web-stories-template-21.js 9.48 kB 0 B
assets/js/chunk-web-stories-template-22-metaData.js 525 B 0 B
assets/js/chunk-web-stories-template-22.js 7.44 kB 0 B
assets/js/chunk-web-stories-template-23-metaData.js 604 B 0 B
assets/js/chunk-web-stories-template-23.js 6.95 kB 0 B
assets/js/chunk-web-stories-template-24-metaData.js 517 B 0 B
assets/js/chunk-web-stories-template-24.js 11.2 kB 0 B
assets/js/chunk-web-stories-template-25-metaData.js 543 B 0 B
assets/js/chunk-web-stories-template-25.js 6.79 kB 0 B
assets/js/chunk-web-stories-template-26-metaData.js 600 B 0 B
assets/js/chunk-web-stories-template-26.js 6.97 kB 0 B
assets/js/chunk-web-stories-template-27-metaData.js 542 B 0 B
assets/js/chunk-web-stories-template-27.js 7.66 kB 0 B
assets/js/chunk-web-stories-template-28-metaData.js 532 B 0 B
assets/js/chunk-web-stories-template-28.js 8.76 kB 0 B
assets/js/chunk-web-stories-template-29-metaData.js 561 B 0 B
assets/js/chunk-web-stories-template-29.js 8.94 kB 0 B
assets/js/chunk-web-stories-template-3-metaData.js 539 B 0 B
assets/js/chunk-web-stories-template-3.js 8.24 kB 0 B
assets/js/chunk-web-stories-template-30-metaData.js 576 B 0 B
assets/js/chunk-web-stories-template-30.js 7.43 kB 0 B
assets/js/chunk-web-stories-template-31-metaData.js 503 B 0 B
assets/js/chunk-web-stories-template-31.js 9.93 kB 0 B
assets/js/chunk-web-stories-template-32-metaData.js 552 B 0 B
assets/js/chunk-web-stories-template-32.js 12.3 kB 0 B
assets/js/chunk-web-stories-template-33-metaData.js 491 B 0 B
assets/js/chunk-web-stories-template-33.js 8.9 kB 0 B
assets/js/chunk-web-stories-template-34-metaData.js 570 B 0 B
assets/js/chunk-web-stories-template-34.js 7.43 kB 0 B
assets/js/chunk-web-stories-template-35-metaData.js 565 B 0 B
assets/js/chunk-web-stories-template-35.js 8.7 kB 0 B
assets/js/chunk-web-stories-template-36-metaData.js 575 B 0 B
assets/js/chunk-web-stories-template-36.js 12.1 kB 0 B
assets/js/chunk-web-stories-template-37-metaData.js 529 B 0 B
assets/js/chunk-web-stories-template-37.js 6.13 kB 0 B
assets/js/chunk-web-stories-template-38-metaData.js 572 B 0 B
assets/js/chunk-web-stories-template-38.js 7.63 kB 0 B
assets/js/chunk-web-stories-template-39-metaData.js 589 B 0 B
assets/js/chunk-web-stories-template-39.js 7.79 kB 0 B
assets/js/chunk-web-stories-template-4-metaData.js 564 B 0 B
assets/js/chunk-web-stories-template-4.js 11.8 kB 0 B
assets/js/chunk-web-stories-template-40-metaData.js 556 B 0 B
assets/js/chunk-web-stories-template-40.js 9.85 kB 0 B
assets/js/chunk-web-stories-template-41-metaData.js 573 B 0 B
assets/js/chunk-web-stories-template-41.js 7.45 kB 0 B
assets/js/chunk-web-stories-template-42-metaData.js 521 B 0 B
assets/js/chunk-web-stories-template-42.js 6.82 kB 0 B
assets/js/chunk-web-stories-template-43-metaData.js 557 B 0 B
assets/js/chunk-web-stories-template-43.js 8.49 kB 0 B
assets/js/chunk-web-stories-template-44-metaData.js 583 B 0 B
assets/js/chunk-web-stories-template-44.js 10.8 kB 0 B
assets/js/chunk-web-stories-template-45-metaData.js 565 B 0 B
assets/js/chunk-web-stories-template-45.js 7.15 kB 0 B
assets/js/chunk-web-stories-template-46-metaData.js 531 B 0 B
assets/js/chunk-web-stories-template-46.js 5.1 kB 0 B
assets/js/chunk-web-stories-template-47-metaData.js 591 B 0 B
assets/js/chunk-web-stories-template-47.js 8.97 kB 0 B
assets/js/chunk-web-stories-template-48-metaData.js 556 B 0 B
assets/js/chunk-web-stories-template-48.js 8.79 kB 0 B
assets/js/chunk-web-stories-template-49-metaData.js 518 B 0 B
assets/js/chunk-web-stories-template-49.js 8.58 kB 0 B
assets/js/chunk-web-stories-template-5-metaData.js 556 B 0 B
assets/js/chunk-web-stories-template-5.js 9.53 kB 0 B
assets/js/chunk-web-stories-template-50-metaData.js 503 B 0 B
assets/js/chunk-web-stories-template-50.js 8.93 kB 0 B
assets/js/chunk-web-stories-template-51-metaData.js 526 B 0 B
assets/js/chunk-web-stories-template-51.js 10.1 kB 0 B
assets/js/chunk-web-stories-template-52-metaData.js 601 B 0 B
assets/js/chunk-web-stories-template-52.js 9.97 kB 0 B
assets/js/chunk-web-stories-template-53-metaData.js 551 B 0 B
assets/js/chunk-web-stories-template-53.js 5.65 kB 0 B
assets/js/chunk-web-stories-template-54-metaData.js 547 B 0 B
assets/js/chunk-web-stories-template-54.js 7.46 kB 0 B
assets/js/chunk-web-stories-template-55-metaData.js 574 B 0 B
assets/js/chunk-web-stories-template-55.js 6.95 kB 0 B
assets/js/chunk-web-stories-template-56-metaData.js 542 B 0 B
assets/js/chunk-web-stories-template-56.js 9.54 kB 0 B
assets/js/chunk-web-stories-template-57-metaData.js 528 B 0 B
assets/js/chunk-web-stories-template-57.js 14.5 kB 0 B
assets/js/chunk-web-stories-template-58-metaData.js 554 B 0 B
assets/js/chunk-web-stories-template-58.js 5.43 kB 0 B
assets/js/chunk-web-stories-template-59-metaData.js 590 B 0 B
assets/js/chunk-web-stories-template-59.js 8.73 kB 0 B
assets/js/chunk-web-stories-template-6-metaData.js 568 B 0 B
assets/js/chunk-web-stories-template-6.js 6.93 kB 0 B
assets/js/chunk-web-stories-template-60-metaData.js 509 B 0 B
assets/js/chunk-web-stories-template-60.js 8.94 kB 0 B
assets/js/chunk-web-stories-template-7-metaData.js 569 B 0 B
assets/js/chunk-web-stories-template-7.js 7.15 kB 0 B
assets/js/chunk-web-stories-template-8-metaData.js 569 B 0 B
assets/js/chunk-web-stories-template-8.js 8.34 kB 0 B
assets/js/chunk-web-stories-template-9-metaData.js 580 B 0 B
assets/js/chunk-web-stories-template-9.js 8.25 kB 0 B
assets/js/chunk-web-stories-templates.js 1.16 kB 0 B
assets/js/chunk-web-stories-textset-0.js 4.6 kB 0 B
assets/js/chunk-web-stories-textset-1.js 5.61 kB 0 B
assets/js/chunk-web-stories-textset-2.js 6.83 kB 0 B
assets/js/chunk-web-stories-textset-3.js 12.8 kB 0 B
assets/js/chunk-web-stories-textset-4.js 3.92 kB 0 B
assets/js/chunk-web-stories-textset-5.js 5.27 kB 0 B
assets/js/chunk-web-stories-textset-6.js 4.98 kB 0 B
assets/js/chunk-web-stories-textset-7.js 8.89 kB 0 B
assets/js/generateBlurhash.worker.worker.js 1.1 kB 0 B
assets/js/imgareaselect.js 3.77 kB 0 B
assets/js/web-stories-activation-notice.js 26.9 kB 0 B
assets/js/web-stories-block.js 23.8 kB 0 B
assets/js/web-stories-carousel.js 3.5 kB 0 B
assets/js/web-stories-dashboard.js 61.8 kB 0 B
assets/js/web-stories-editor.js 1.46 MB -15 B (0%)
assets/js/web-stories-embed.js 20 B 0 B
assets/js/web-stories-lightbox.js 751 B 0 B
assets/js/web-stories-tinymce-button.js 2.85 kB 0 B
assets/js/web-stories-widget.js 587 B 0 B

compressed-size-action

@AnuragVasanwala AnuragVasanwala marked this pull request as draft May 16, 2023 08:48
@swissspidy
Copy link
Collaborator

Nice find! 😅

Looking at this hasText function now, I have to say it might just as well be a single const... No need for useCallback

const hasText = selectedElements.find(({ type }) => type === 'text');

What do you think?

@AnuragVasanwala
Copy link
Collaborator Author

Nice find! 😅

Looking at this hasText function now, I have to say it might just as well be a single const... No need for useCallback

const hasText = selectedElements.find(({ type }) => type === 'text');

What do you think?

Yes, that would be great! I am updating accordingly into the new commit.

But I'm facing an issue in unit-test after I have added brackets:

npm run test:js -- --runInBand --ci --cacheDirectory="$HOME/.jest-cache" --collectCoverage --shard=$SHARD

> pretest:js
> if [ -z $AMP_VALIDATOR_FILE ]; then curl --output-dir $TMPDIR -O -f https://cdn.ampproject.org/v0/validator_wasm.js; fi

  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 8290k  100 8290k    0     0  4584k      0  0:00:01  0:00:01 --:--:-- 4603k

> test:js
> jest --config=tests/js/jest.config.js --runInBand --ci --cacheDirectory=/Users/anurag/.jest-cache --collectCoverage --shard=

............
/Users/anurag/Local Sites/google-webstories/app/public/wp-content/plugins/web-stories/packages/story-editor/src/components/panels/design/textStyle/test/textStyle.js

  ● Panels/TextStyle › FontControls › should set the text bold when the key command is pressed

    TypeError: Cannot read properties of undefined (reading '0')

      310 |       });
      311 |
    > 312 |       const updatingFunction = pushUpdate.mock.calls[0][0];
          |                                                        ^
      313 |       const resultOfUpdating = updatingFunction({ content: 'Hello world' });
      314 |       expect(resultOfUpdating).toStrictEqual(
      315 |         {

      at Object.<anonymous> (packages/story-editor/src/components/panels/design/textStyle/test/textStyle.js:312:56)

  ● Panels/TextStyle › FontControls › should set the text underline when the key command is pressed

    TypeError: Cannot read properties of undefined (reading '0')

      329 |       });
      330 |
    > 331 |       const updatingFunction = pushUpdate.mock.calls[0][0];
          |                                                        ^
      332 |       const resultOfUpdating = updatingFunction({ content: 'Hello world' });
      333 |       expect(resultOfUpdating).toStrictEqual(
      334 |         {

      at Object.<anonymous> (packages/story-editor/src/components/panels/design/textStyle/test/textStyle.js:331:56)

  ● Panels/TextStyle › FontControls › should set the text italics when the key command is pressed

    TypeError: Cannot read properties of undefined (reading '0')

      349 |       });
      350 |
    > 351 |       const updatingFunction = pushUpdate.mock.calls[0][0];
          |                                                        ^
      352 |       const resultOfUpdating = updatingFunction({ content: 'Hello world' });
      353 |       expect(resultOfUpdating).toStrictEqual(
      354 |         {

      at Object.<anonymous> (packages/story-editor/src/components/panels/design/textStyle/test/textStyle.js:351:56)

I am looking into unit-test for the component. Any suggestion?

@swissspidy
Copy link
Collaborator

Looks like the dummy text element used in the tests is missing type: 'text',

const textElement = {
id: '1',
textAlign: 'normal',
fontSize: 30,
lineHeight: 1,
font: {
family: 'ABeeZee',
},
x: 0,
y: 0,
height: 100,
width: 120,
rotationAngle: 0,
padding: DEFAULT_PADDING,
backgroundTextMode: BACKGROUND_TEXT_MODE.NONE,
};

Fixing that makes the tests pass

AnuragVasanwala and others added 2 commits May 16, 2023 15:54
If attribute `type: 'text'` does not present it will `hasText()` will return `false` and following three dependent test will fails:
- should set the text bold when the key command is pressed
- should set the text underline when the key command is pressed
- should set the text italics when the key command is pressed

Co-Authored-By: Pascal Birchler <pascal.birchler@gmail.com>
Co-Authored-By: Pascal Birchler <pascal.birchler@gmail.com>
@AnuragVasanwala
Copy link
Collaborator Author

Looks like the dummy text element used in the tests is missing type: 'text',

const textElement = {
id: '1',
textAlign: 'normal',
fontSize: 30,
lineHeight: 1,
font: {
family: 'ABeeZee',
},
x: 0,
y: 0,
height: 100,
width: 120,
rotationAngle: 0,
padding: DEFAULT_PADDING,
backgroundTextMode: BACKGROUND_TEXT_MODE.NONE,
};

Fixing that makes the tests pass

Thank you so much 👍🏻
It fixed✅ unit-test issue.

@AnuragVasanwala AnuragVasanwala marked this pull request as ready for review May 16, 2023 10:26
@swissspidy swissspidy changed the title Fix: Callback to function hasText() Fix formatting keyboard shortcuts on non-text elements May 16, 2023
Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

LGTM!

@swissspidy swissspidy merged commit dcc900a into main May 16, 2023
45 of 69 checks passed
@swissspidy swissspidy deleted the fix/13281-richtext-hastext-check-issue branch May 16, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: Canvas Group: Keyboard Shortcuts Keyboard shortcuts + menu P1 High priority, must do soon Package: Rich Text /packages/rich-text Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read properties of undefined (reading 'replace')
3 participants