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

Manual link decorators for images will not crash decorators for linked text #8119

Merged
merged 8 commits into from
Sep 23, 2020
Merged

Manual link decorators for images will not crash decorators for linked text #8119

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
65f3519
Fixed the manual decorator converter for linked images. It should not…
pomek Sep 21, 2020
052032c
Added to all-features a case with decorators and images.
pomek Sep 21, 2020
3cbb748
Wording. [skip ci]
pomek Sep 22, 2020
a344f4e
Wording once again. [skip ci]
pomek Sep 22, 2020
dec11ed
Internal (link): Extracted common code into reused function.
mlewand Sep 23, 2020
b5a2d00
Refactored tests.
pomek Sep 23, 2020
4d734d4
Fixed indents.
pomek Sep 23, 2020
58fc76d
Tests (link): Added issue reference.
mlewand Sep 23, 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
16 changes: 13 additions & 3 deletions packages/ckeditor5-link/src/linkimageediting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,11 @@ function upcastLink() {
}

// A full definition of the image feature.
// figure > a > img: parent of the link element is an image element.
// figure > a > img: parent of the view link element is an image element (figure).
let modelElement = data.modelCursor.parent;

if ( !modelElement.is( 'element', 'image' ) ) {
// a > img: parent of the link is not the image element. We need to convert it manually.
// a > img: parent of the view link is not the image (figure) element. We need to convert it manually.
const conversionResult = conversionApi.convertItem( imageInLink, data.modelCursor );

// Set image range as conversion result.
Expand Down Expand Up @@ -229,6 +229,13 @@ function upcastImageLinkManualDecorator( manualDecorators, decorator ) {
return dispatcher => {
dispatcher.on( 'element:a', ( evt, data, conversionApi ) => {
const viewLink = data.viewItem;
const imageInLink = Array.from( viewLink.getChildren() ).find( child => child.name === 'img' );

// We need to check whether an image is inside a link because the converter handles
// only manual decorators for linked images. See #7975.
if ( !imageInLink ) {
return;
}

const consumableAttributes = {
attributes: manualDecorators.get( decorator.id ).attributes
Expand All @@ -248,7 +255,10 @@ function upcastImageLinkManualDecorator( manualDecorators, decorator ) {
}

// At this stage we can assume that we have the `<image>` element.
const modelElement = data.modelCursor.parent;
// `nodeBefore` comes after conversion: `<a><img></a>`.
// `parent` comes with full image definition: `<figure><a><img></a></figure>.
// See a body of the `upcastLink()` function.
const modelElement = data.modelCursor.nodeBefore || data.modelCursor.parent;

conversionApi.writer.setAttribute( decorator.id, true, modelElement );
}, { priority: 'high' } );
Expand Down
151 changes: 151 additions & 0 deletions packages/ckeditor5-link/tests/linkimageediting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,4 +711,155 @@ describe( 'LinkImageEditing', () => {
} );
} );
} );

describe( 'integration', () => {
let editor, model;

beforeEach( () => {
return VirtualTestEditor
.create( {
plugins: [ Paragraph, LinkImageEditing ],
link: {
decorators: {
isExternal: {
mode: 'manual',
label: 'Open in a new tab',
attributes: {
target: '_blank',
rel: 'noopener noreferrer'
}
},
isDownloadable: {
mode: 'manual',
label: 'Downloadable',
attributes: {
download: 'download'
}
},
isGallery: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that for better readability we can simplify it just to one or max two decorators to get the full coverage and keep the markup simpler.

mode: 'manual',
label: 'Gallery link',
attributes: {
class: 'gallery'
}
}
}
}
} )
.then( newEditor => {
editor = newEditor;
model = editor.model;
} );
} );

afterEach( () => {
return editor.destroy();
} );

// See: #7975.
describe( 'manual decorators: linked text and linked image', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're testing the upcast implementation here and there are already two tests for handling link decorators during the upcast.

Wouldn't it make sense to put your unit tests next to it?

I think a good idea would be to nest decorator-related suite in the upcast converter suite, there you'd have your new tests as well as the tests that are already there. Also old tests could reuse the editor that you're creating in beforeEach() rather than have a copy&paste editor initialization at the beginning.

That should result with nice and tight suite.

it( 'should upcast the decorators when linked image (figure > a > img)', () => {
editor.setData(
'<figure class="image">' +
'<a class="gallery" href="https://cksource.com" target="_blank" rel="noopener noreferrer" download="download">' +
'<img src="sample.jpg" alt="bar">' +
'</a>' +
'<figcaption>Caption</figcaption>' +
'</figure>' +
'<p>' +
'<a href="https://cksource.com" target="_blank" rel="noopener noreferrer" download="download">' +
'https://cksource.com' +
'</a>' +
'</p>'
);

expect( getModelData( model, { withoutSelection: true } ) ).to.equal(
'<image alt="bar" ' +
'linkHref="https://cksource.com" ' +
'linkIsDownloadable="true" ' +
'linkIsExternal="true" ' +
'linkIsGallery="true" ' +
'src="sample.jpg">' +
'</image>' +
'<paragraph>' +
'<$text linkHref="https://cksource.com" linkIsDownloadable="true" linkIsExternal="true">' +
'https://cksource.com' +
'</$text>' +
'</paragraph>'
);
} );

it( 'should upcast the decorators when linked image (a > img)', () => {
editor.setData(
'<a class="gallery" href="https://cksource.com" target="_blank" rel="noopener noreferrer" download="download">' +
'<img src="sample.jpg" alt="bar">' +
'</a>' +
'<p>' +
'<a href="https://cksource.com" target="_blank" rel="noopener noreferrer" download="download">' +
'https://cksource.com' +
'</a>' +
'</p>'
);

expect( getModelData( model, { withoutSelection: true } ) ).to.equal(
'<image alt="bar" ' +
'linkHref="https://cksource.com" ' +
'linkIsDownloadable="true" ' +
'linkIsExternal="true" ' +
'linkIsGallery="true" ' +
'src="sample.jpg">' +
'</image>' +
'<paragraph>' +
'<$text linkHref="https://cksource.com" linkIsDownloadable="true" linkIsExternal="true">' +
'https://cksource.com' +
'</$text>' +
'</paragraph>'
);
} );

it( 'should downcast the decorators after applying a change', () => {
setModelData( model,
'<image alt="bar" src="sample.jpg"></image>' +
'<paragraph>' +
'<$text>https://cksource.com</$text>' +
'</paragraph>'
);

model.change( writer => {
// <image>
writer.setSelection( model.document.getRoot().getChild( 0 ), 'on' );
Copy link
Contributor

Choose a reason for hiding this comment

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

A much simpler approach is to select image by enclosing it in brackets in line

ckeditor5/packages/ckeditor5-link/tests/linkimageediting.js

Line 822 in dec11ed

'<image alt="bar" src="sample.jpg"></image>' +

Like so '[<image alt="bar" src="sample.jpg"></image>]' +.

Copy link
Contributor

Choose a reason for hiding this comment

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

On a side note: I checked that the image does not necessarily need a focus, as the test passes if I remove this particular model.change() call (I assume that editor will simply put the focus to the first element by default).

} );

editor.execute( 'link', 'https://cksource.com', {
linkIsDownloadable: true,
linkIsExternal: true,
linkIsGallery: true
} );

model.change( writer => {
// <$text>
writer.setSelection( model.document.getRoot().getChild( 1 ).getChild( 0 ), 'on' );
} );

editor.execute( 'link', 'https://cksource.com', {
linkIsDownloadable: true,
linkIsExternal: true,
linkIsGallery: true
} );

expect( editor.getData() ).to.equal(
'<figure class="image">' +
'<a class="gallery" href="https://cksource.com" download="download" target="_blank" rel="noopener noreferrer">' +
'<img src="sample.jpg" alt="bar">' +
'</a>' +
'</figure>' +
'<p>' +
'<a class="gallery" href="https://cksource.com" download="download" target="_blank" rel="noopener noreferrer">' +
'https://cksource.com' +
'</a>' +
'</p>'
);
} );
} );
} );
} );
32 changes: 32 additions & 0 deletions tests/manual/all-features.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,38 @@ <h2>Code blocks in the table</h2>
</tbody>
</table>
</figure>

<hr>

<h2>Link images + Link decorators</h2>

<table>
<tr>
<td>Linked text</td>
<td>Linked image (<code>figure > a > img</code>)</td>
<td>Linked image (<code>a > img</code>)</td>
</tr>
<tr>
<td>
<p>
<a href="https://cksource.com" target="_blank" rel="noopener noreferrer" download="download">https://cksource.com</a>
</p>
</td>
<td>
<figure class="image">
<a class="gallery" href="https://cksource.com">
<img src="sample.jpg" alt="bar">
</a>
<figcaption>Caption</figcaption>
</figure>
</td>
<td>
<a class="gallery" href="https://cksource.com" target="_blank" rel="noopener noreferrer" download="download">
<img src="sample.jpg" alt="bar">
</a>
</td>
</tr>
</table>
</div>

<style>
Expand Down
26 changes: 26 additions & 0 deletions tests/manual/all-features.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,32 @@ ClassicEditor
minimumCharacters: 1
}
]
},
link: {
decorators: {
isExternal: {
mode: 'manual',
label: 'Open in a new tab',
attributes: {
target: '_blank',
rel: 'noopener noreferrer'
}
},
isDownloadable: {
mode: 'manual',
label: 'Downloadable',
attributes: {
download: 'download'
}
},
isGallery: {
mode: 'manual',
label: 'Gallery link',
attributes: {
class: 'gallery'
}
}
}
}
} )
.then( editor => {
Expand Down
16 changes: 16 additions & 0 deletions tests/manual/all-features.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

It should contain as many features as we developed. By resizing your viewport you can observe whether grouping toolbar items work.

---

## Editor

There should be "two pages" with two sections per each page. Pages should be separated by the page break feature.
Expand All @@ -20,16 +22,30 @@ There should be "two pages" with two sections per each page. Pages should be sep

**Code blocks in the table** - in the table (4x3), in the second and fourth columns should be visible code snippets.

**Horizontal line** - There is the `<hr>` in the source HTML. It should be displayed in the editor.

**Link images + Link decorators** - in the table (3x2), there be a link and two linked images that uses the manual decorators feature:

- Left column: a plain text with two decorators enabled: `Open in a new tab`, `Downloadable`
- Middle column: an image with the caption that is a `Gallery link`
- Right column: an image without the caption with enabled all decorators (listed above)
pomek marked this conversation as resolved.
Show resolved Hide resolved

---

## Action buttons

- Clear editor - calls `editor.setData( '' )`
- Open print preview - opens the print preview window
- Turn on/off read-only mode - toggle read-only mode

---

## Console

Wordcount plugin logs into the console number of characters and words in the editor's data.

---

## Additional

- Empty editor should display a placeholder: `'Type the content here!'`
Expand Down