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

RichText: Fix browser formatting buttons #13833

Merged
merged 3 commits into from Feb 18, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Feb 12, 2019

Description

This PR fixes any formatting buttons that are provided by the browser, e.g. formatting buttons in Safari and iOS Safari.

bc744485ab70fcb0bd108a8f371ec8a2

screenshot 2019-02-12 at 13 35 45

What's the problem at the moment?

  • We don't control what the button is inserting. E.g. Safari's bold button inserts <b> tags.
  • Some buttons don't make any sense in RichText, such as the align and list buttons. They could be linked at a block level though. I've prevented them from doing anything, but ideally they should work at the block level (maybe a future PR). I've found no way to remove the buttons.
  • When using the buttons, the first letter didn't receive any formatting.
  • It also fixes the formatting added by Chrome when pressing ctrl+u on a Mac, a shortcut that wasn't overridden by us.

The most reliable solution I've found is checking the input event for inputType, and then resetting the DOM. After this a format in the format library may use RichTextInputEvent to do something based on that information.

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.
  • I've included developer documentation if appropriate.

@ellatrix ellatrix added [Type] Bug An existing feature does not function as intended [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable Browser Issues Issues or PRs that are related to browser specific problems [Package] Format library /packages/format-library [Package] Rich text /packages/rich-text and removed [Package] Rich text /packages/rich-text labels Feb 12, 2019
@ellatrix ellatrix added this to the 5.1 (Gutenberg) milestone Feb 12, 2019
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

I tested this in Safari 12 and confirmed that all of the Touch Bar formatting buttons either work or don't break anything.

Agree that it would be nice to get the Insert List buttons working in a follow-up PR.

The approach makes sense to me, but take my ✅ with a grain of salt as I'm not a RichText expert! 🙂

*/
import { Component } from '@wordpress/element';

export class RichTextInputEvent extends Component {
Copy link
Member

Choose a reason for hiding this comment

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

A unit test for this component could be good.

Copy link
Member Author

Choose a reason for hiding this comment

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

How would it work?

Copy link
Member

Choose a reason for hiding this comment

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

Something like this pseudocode:

const onInput = jest.fn();

mount( <RichTextInputEvent inputType="ella-is-cool" onInput={ onInput } /> );

const event = new Event( 'input' );
event.inputType = 'ella-is-cool';
document.dispatchEvent( event );

expect( onInput ).toHaveBeenCalled();

I find these sorts of tests don't hurt and sometimes catch silly regressions e.g. typos.

packages/editor/src/components/rich-text/input-event.js Outdated Show resolved Hide resolved

onInput( event ) {
if ( event.inputType === this.props.inputType ) {
this.props.onInput();
Copy link
Member

Choose a reason for hiding this comment

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

Is there a need to preventDefault as well, to ensure it's fully controlled?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's not really anything to prevent. The input event happens after the fact.

Copy link
Member

Choose a reason for hiding this comment

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

There's not really anything to prevent. The input event happens after the fact.

And to that point, an input event is in-fact not cancelable.

https://w3c.github.io/uievents/#event-type-input

@noisysocks noisysocks merged commit 3db4ff5 into master Feb 18, 2019
@noisysocks noisysocks deleted the fix/browser-formatting-buttons branch February 18, 2019 00:50
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* RichText: Fix browser formatting buttons

* Simplify

* componentDidMount instead of componentWillMount
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* RichText: Fix browser formatting buttons

* Simplify

* componentDidMount instead of componentWillMount
This was referenced Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Browser Issues Issues or PRs that are related to browser specific problems [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Format library /packages/format-library [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants