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

Restore native backspace behaviour #11627

Merged
merged 5 commits into from
Nov 9, 2018
Merged

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Nov 8, 2018

Description

Restores a part of #11287. It should not restore any bugginess! :)

We need to make sure none of the issues that #10799 fixed are reintroduced.

Still needs e2e test and investigate failing e2e tests.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@ellatrix ellatrix added this to the 4.3 milestone Nov 8, 2018
@ellatrix
Copy link
Member Author

ellatrix commented Nov 8, 2018

There's one failing e2e test: if you select a whole paragraph, it seems TinyMCE is doing some weird stuff.

@@ -252,9 +252,11 @@ export default class TinyMCE extends Component {

onKeyDown( event ) {
const { keyCode } = event;
const { startContainer, startOffset, endContainer, endOffset } = getSelection().getRangeAt( 0 );
const isCollapsed = startContainer === endContainer && startOffset === endOffset;
Copy link
Member Author

Choose a reason for hiding this comment

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

getSelection().isCollapsed is not accurate, should prob be commented.

const start = getSelectionStart( value );
const end = getSelectionEnd( value );

// Always handle uncollapsed selections ourselves.
Copy link
Member Author

Choose a reason for hiding this comment

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

Could use more comment sorry.

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Nov 9, 2018
@ellatrix
Copy link
Member Author

ellatrix commented Nov 9, 2018

Thanks! Could you also add an e2e test for word wise deletion alt+backspace? Thanks so much!

@notnownikki
Copy link
Member

Sure :)

await page.keyboard.type( 'delta' );
const blockText = await page.evaluate( () => document.activeElement.textContent );
expect( blockText ).toBe( 'alpha beta gamma delta' );
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

I love these tests, thanks @notnownikki

@notnownikki
Copy link
Member

I'm going through the linked issues and adding e2e tests where I can. I want to get an e2e test for #6021 next. So far, all is looking good!

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@youknowriad
Copy link
Contributor

I'm going through the linked issues and adding e2e tests where I can. I want to get an e2e test for #6021 next. So far, all is looking good!

Nice, feel free to open follow-up PRs with more e2e test 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants