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

Remove unused is-hovered class #19390

Merged
merged 2 commits into from
Jan 3, 2020
Merged

Remove unused is-hovered class #19390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b878d7
Remove unused is-hovered class
ellatrix Jan 2, 2020
94cbec6
Revert 0d0c8e0d55849529227df3540ad35ff3edf5be5b
ellatrix Jan 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 1 addition & 68 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,59 +125,6 @@ function BlockListBlock( {

const breadcrumb = useRef();

// Keep track of touchstart to disable hover on iOS
const hadTouchStart = useRef( false );
const onTouchStart = () => {
hadTouchStart.current = true;
};
const onTouchStop = () => {
// Clear touchstart detection
// Browser will try to emulate mouse events also see https://www.html5rocks.com/en/mobile/touchandmouse/
hadTouchStart.current = false;
};

// Handling isHovered
const [ isBlockHovered, setBlockHoveredState ] = useState( false );

/**
* Sets the block state as unhovered if currently hovering. There are cases
* where mouseleave may occur but the block is not hovered (multi-select),
* so to avoid unnecesary renders, the state is only set if hovered.
*/
const hideHoverEffects = () => {
if ( isBlockHovered ) {
setBlockHoveredState( false );
}
};
/**
* A mouseover event handler to apply hover effect when a pointer device is
* placed within the bounds of the block. The mouseover event is preferred
* over mouseenter because it may be the case that a previous mouseenter
* event was blocked from being handled by a IgnoreNestedEvents component,
* therefore transitioning out of a nested block to the bounds of the block
* would otherwise not trigger a hover effect.
*
* @see https://developer.mozilla.org/en-US/docs/Web/Events/mouseenter
*/
const maybeHover = () => {
if (
isBlockHovered ||
isPartOfMultiSelection ||
isSelected ||
hadTouchStart.current
) {
return;
}
setBlockHoveredState( true );
};

// Set hover to false once we start typing or select the block.
useEffect( () => {
if ( isTypingWithinBlock || isSelected ) {
hideHoverEffects();
}
} );

// Handling the error state
const [ hasError, setErrorState ] = useState( false );
const onBlockError = () => setErrorState( true );
Expand Down Expand Up @@ -389,8 +336,6 @@ function BlockListBlock( {
if ( isSelected && ( buttons || which ) === 1 ) {
onSelectionStart( clientId );
}

hideHoverEffects();
};

const selectOnOpen = ( open ) => {
Expand All @@ -413,7 +358,6 @@ function BlockListBlock( {
);

// Rendering the output
const isHovered = isBlockHovered && ! isPartOfMultiSelection;
const blockType = getBlockType( name );
// translators: %s: Type of block (i.e. Text, Image etc)
const blockLabel = sprintf( __( 'Block: %s' ), blockType.title );
Expand All @@ -424,17 +368,12 @@ function BlockListBlock( {

// If the block is selected and we're typing the block should not appear.
// Empty paragraph blocks should always show up as unselected.
const showEmptyBlockSideInserter = ! isNavigationMode && ( isSelected || isHovered || isLast ) && isEmptyDefaultBlock && isValid;
const showEmptyBlockSideInserter = ! isNavigationMode && ( isSelected || isLast ) && isEmptyDefaultBlock && isValid;
const shouldAppearSelected =
! isFocusMode &&
! showEmptyBlockSideInserter &&
isSelected &&
! isTypingWithinBlock;
const shouldAppearHovered =
! isFocusMode &&
! hasFixedToolbar &&
isHovered &&
! isEmptyDefaultBlock;
const shouldShowBreadcrumb = isNavigationMode && isSelected;
const shouldShowContextualToolbar =
! isNavigationMode &&
Expand Down Expand Up @@ -467,7 +406,6 @@ function BlockListBlock( {
'is-selected': shouldAppearSelected && hasSelectedUI,
'is-navigate-mode': isNavigationMode,
'is-multi-selected': isMultiSelected,
'is-hovered': shouldAppearHovered,
'is-reusable': isReusableBlock( blockType ),
'is-dragging': isDragging,
'is-typing': isTypingWithinBlock,
Expand Down Expand Up @@ -534,14 +472,9 @@ function BlockListBlock( {
<IgnoreNestedEvents
id={ blockElementId }
ref={ wrapper }
onMouseOver={ maybeHover }
onMouseOverHandled={ hideHoverEffects }
Copy link
Member

Choose a reason for hiding this comment

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

I think this was the only usage of the "Handled" behavior for IgnoreNestedEvents, so it could be removed. Not sure that it will make a ton of difference, but at least simplifying a bit.

Effectively would amount to a revert of 0d0c8e0d55849529227df3540ad35ff3edf5be5b (from #5658)

onMouseLeave={ hideHoverEffects }
className={ wrapperClassName }
data-type={ name }
onTouchStart={ onTouchStart }
onFocus={ onFocus }
onClick={ onTouchStop }
onKeyDown={ onKeyDown }
tabIndex="0"
aria-label={ blockLabel }
Expand Down
27 changes: 8 additions & 19 deletions packages/block-editor/src/components/ignore-nested-events/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ export class IgnoreNestedEvents extends Component {
* @param {Event} event Event object.
*/
proxyEvent( event ) {
const isHandled = !! event.nativeEvent._blockHandled;
// Skip if already handled (i.e. assume nested block)
if ( event.nativeEvent._blockHandled ) {
return;
}

// Assign into the native event, since React will reuse their synthetic
// event objects and this property assignment could otherwise leak.
Expand All @@ -50,13 +53,7 @@ export class IgnoreNestedEvents extends Component {
event.nativeEvent._blockHandled = true;

// Invoke original prop handler
let propKey = this.eventMap[ event.type ];

// If already handled (i.e. assume nested block), only invoke a
// corresponding "Handled"-suffixed prop callback.
if ( isHandled ) {
propKey += 'Handled';
}
const propKey = this.eventMap[ event.type ];

if ( this.props[ propKey ] ) {
this.props[ propKey ]( event );
Expand All @@ -71,24 +68,16 @@ export class IgnoreNestedEvents extends Component {
...Object.keys( props ),
], ( result, key ) => {
// Try to match prop key as event handler
const match = key.match( /^on([A-Z][a-zA-Z]+?)(Handled)?$/ );
const match = key.match( /^on([A-Z][a-zA-Z]+)$/ );
if ( match ) {
const isHandledProp = !! match[ 2 ];
if ( isHandledProp ) {
// Avoid assigning through the invalid prop key. This
// assumes mutation of shallow clone by above spread.
delete props[ key ];
}

// Re-map the prop to the local proxy handler to check whether
// the event has already been handled.
const proxiedPropName = 'on' + match[ 1 ];
result[ proxiedPropName ] = this.proxyEvent;
result[ key ] = this.proxyEvent;

// Assign event -> propName into an instance variable, so as to
// avoid re-renders which could be incurred either by setState
// or in mapping values to a newly created function.
this.eventMap[ match[ 1 ].toLowerCase() ] = proxiedPropName;
this.eventMap[ match[ 1 ].toLowerCase() ] = key;
}

return result;
Expand Down
31 changes: 6 additions & 25 deletions packages/block-editor/src/components/ignore-nested-events/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { mount } from 'enzyme';
import { shallow } from 'enzyme';

/**
* Internal dependencies
Expand All @@ -10,24 +10,24 @@ import { IgnoreNestedEvents } from '../';

describe( 'IgnoreNestedEvents', () => {
it( 'passes props to its rendered div', () => {
const wrapper = mount(
const wrapper = shallow(
<IgnoreNestedEvents className="foo" />
);

expect( wrapper.find( 'div' ) ).toHaveLength( 1 );
expect( wrapper.type() ).toBe( 'div' );
expect( wrapper.prop( 'className' ) ).toBe( 'foo' );
} );

it( 'stops propagation of events to ancestor IgnoreNestedEvents', () => {
const spyOuter = jest.fn();
const spyInner = jest.fn();
const wrapper = mount(
const wrapper = shallow(
<IgnoreNestedEvents onClick={ spyOuter }>
<IgnoreNestedEvents onClick={ spyInner } />
</IgnoreNestedEvents>
);

wrapper.find( 'div' ).last().simulate( 'click' );
wrapper.childAt( 0 ).simulate( 'click' );

expect( spyInner ).toHaveBeenCalled();
expect( spyOuter ).not.toHaveBeenCalled();
Expand All @@ -36,7 +36,7 @@ describe( 'IgnoreNestedEvents', () => {
it( 'stops propagation of child handled events', () => {
const spyOuter = jest.fn();
const spyInner = jest.fn();
const wrapper = mount(
const wrapper = shallow(
<IgnoreNestedEvents onClick={ spyOuter }>
<IgnoreNestedEvents childHandledEvents={ [ 'onClick' ] }>
<div />
Expand All @@ -51,23 +51,4 @@ describe( 'IgnoreNestedEvents', () => {
expect( spyInner ).not.toHaveBeenCalled();
expect( spyOuter ).not.toHaveBeenCalled();
} );

it( 'invokes callback of Handled-suffixed prop if handled', () => {
const spyOuter = jest.fn();
const spyInner = jest.fn();
const wrapper = mount(
<IgnoreNestedEvents onClickHandled={ spyOuter }>
<IgnoreNestedEvents childHandledEvents={ [ 'onClick' ] }>
<div />
<IgnoreNestedEvents onClick={ spyInner } />
</IgnoreNestedEvents>
</IgnoreNestedEvents>
);

const div = wrapper.childAt( 0 ).childAt( 0 );
div.simulate( 'click' );

expect( spyInner ).not.toHaveBeenCalled();
expect( spyOuter ).toHaveBeenCalled();
} );
} );