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

Blocks: Reselect the current block if we click again on it #556

Merged
merged 3 commits into from
May 2, 2017

Conversation

youknowriad
Copy link
Contributor

closes #498

This PR allows reselecting a block (showing the controls) when clicking on the block again.

Testing instructions

  • Select a block
  • Type some text on it
  • Click on the block again
  • The controls should show up

@youknowriad youknowriad added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Bug An existing feature does not function as intended labels Apr 28, 2017
@youknowriad youknowriad self-assigned this Apr 28, 2017
@youknowriad youknowriad requested review from mtias and ellatrix April 28, 2017 15:35
@ellatrix
Copy link
Member

Works really well. I'm experiencing a strange bug where, if I click on a block to show the controls, and then click on another, the caret appears on the start instead of the letter clicked.

@mtias
Copy link
Member

mtias commented Apr 28, 2017

I was seeing a similar thing but triggered by clicking on one of the formatting buttons repeatedly (three times on italics without pressing any other key).

@youknowriad
Copy link
Contributor Author

@mtias @iseulde These issues should be fixed by 473563a

The idea is to avoid unnecessary updateContent calls.

@@ -137,15 +137,13 @@ class VisualEditorBlock extends wp.element.Component {
wrapperProps = settings.getEditWrapperProps( block.attributes );
}

// Disable reason: Each block can receive focus but must be able to contain
// block children. Tab keyboard navigation enabled by tabIndex assignment.
// Disable reason: Each block can be selected by clicking on it
Copy link
Member

Choose a reason for hiding this comment

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

The changes here break tab navigation:

  1. Focus text block
  2. Press tab

Expected: Image block is selected
Actual: Third block (next text block) is selected

The disable reason doesn't seem acceptable for accessibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 11d81e4

Copy link
Member

Choose a reason for hiding this comment

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

Should the comment be reverted back to its previous text? Or at least updated to reflect that it accepts focus? Seems like suggesting "clicking on it" as reason for disabling an accessibility rule is destined to... provoke response 😄

@@ -266,10 +268,13 @@ export default class Editable extends wp.element.Component {
this.focus();
}

// The savedContent var allows us to avoid updating the content right after an onChange call
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow this. Why does the value of this.props.value differ from prevProps.value after the onChange? And if it does, why do we not want this to be reflected in the editable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you make a change on the editable. Say the old value is a and the new one is ab, we don't want to trigger the updateContent even if the old value prop and the new one are different. That's what I'm trying to achieve here by caching the savedContent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unclear how to enable controls after typing
4 participants