Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Fix placeholder over media embed. #1749

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix placeholder over media embed. #1749

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6fb56bd
The placeholder will not be shown over widgets.
jodator Jun 25, 2019
89a5c88
Docs: Improved the placeholder-in-widget fix comment. [skip ci]
oleq Jun 25, 2019
b772835
Merge branch 'master' into t/ckeditor5/1684
jodator Aug 5, 2019
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
6 changes: 6 additions & 0 deletions src/view/placeholder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,12 @@ export function needsPlaceholder( element ) {
return false;
}

// If the element is a Widget, always consider it a non-empty and thus not needing a placeholder.
// https://github.com/ckeditor/ckeditor5/issues/1684
if ( element.getCustomProperty( 'widget' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

On the second thought, that would be the only place in the codebase other than isWidget() from ckeditor5-widget that calls getCustomProperty( 'widget' ) 😛

Maybe we could use ckeditor5-widget as an engine dependency after all. I'm not sure about consequences, though. cc @Reinmar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... it might be a cycling dependency then... or widget being an engine feature :P.

At the current state I rather have this check then importing isWidget() but let's see @Reinmar 2¢.

Copy link
Member

Choose a reason for hiding this comment

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

TBH I have nothing against the current state (element.getCustomProperty( 'widget' )) and some improvements in i26 as long as we don't change dependencies in i25.

return false;
}

// The element is empty only as long as it contains nothing but uiElements.
const isEmptyish = !Array.from( element.getChildren() )
.some( element => !element.is( 'uiElement' ) );
Expand Down
14 changes: 14 additions & 0 deletions tests/view/placeholder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,20 @@ describe( 'placeholder', () => {
expect( needsPlaceholder( element ) ).to.be.false;
} );

it( 'should return false if element is a widget', () => {
// Widget with ui-only elements inside (emptyish).
setData( view, '<div><ui:span></ui:span></div>' );
viewDocument.isFocused = false;

const element = viewRoot.getChild( 0 );

view.change( writer => {
writer.setCustomProperty( 'widget', true, element );
} );

expect( needsPlaceholder( element ) ).to.be.false;
} );

it( 'should return true if element is empty and document is blurred', () => {
setData( view, '<p></p>' );
viewDocument.isFocused = false;
Expand Down