Fix for: Image in table cell has an empty URL field when edited from context menu opened by right-click

[Dumluregn]

What is the purpose of this pull request?

Bug

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

As this was a specific issue and required opening context menu by right-click (keyboard shortcuts were working fine before fix) there is just a short 15 seconds manual test included.

• Unit tests
• Manual tests

What changes did you make?

I made Table Selection treat images from Image plugin the same way it operates on widgets, etc from Enhanced Image plugin (i.e. an early return in function checking if the table is selected).

Closes #2235.

[f1ames]

Rebased on the latest master.

[f1ames]

This one is a little tricky. The part detecting if image is selected is fine however I'm wondering on the placement of this code.

First of all, the isTableSelection() function is further used in selection.isInTable() method (apart from few other places in selection.js). This means that with this change, we change the selection logic - if image inside table is selected, selection.isInTable() returns false indicating that selection is not inside the table. I'm not sure if this is the right assumption 🤔
This affects all places and plugins using this method (not that much apart from tableselection and tabletools plugin).

I have also found one case where fake selection is not correctly reseted:

It happens after pressing Delete/Backspace over tableselection. Then the image cannot be selected and cursor is different (because the fake selection is still active). Only after reselecting some cells and then clicking somewhere in the table fake selection is properly reseted. This worked fine before this change.

My suggestion is to try to find the cause of the above issue and move proposed code from the selection.js to tableselection/plugin.js - fakeSelectionChangeHandler() function seems to be a good place to check (it uses selection.isInTable() method). I briefly checked and it worked (however, the issue mentioned above was still visible so it might require some additional changes).

[f1ames]

Have you checked if unit tests could be created to test this fix? I see there are some tests in tableselection plugin simulating right click (https://github.com/ckeditor/ckeditor-dev/blob/major/tests/plugins/tableselection/rightclick.js). Maybe this fix can be tested in a similar fashion?

[Dumluregn]

About unit test - it was impossible to exactly mimic the scenario (using context menu or dialog didn't work), so instead test is checking if after selecting image the cell becomes fake-selected or not. That was the reason of incorrect behaviour before, so as long as the test passes this patch should work fine.

[f1ames]

Rebased on the latest major.

[f1ames]

Looks good 👍

However, it doesn't work on Edge and IE11:

TBH, it looks like a different issue, not related to this one. Can you confirm if it happens also without tableselection plugin? If so, could you extract it to a separate ticket and then we can merge this one. However, if it happens only with tableselection it needs to be covered in this PR as a part of this bugfix.

[Dumluregn]

@f1ames - thanks for bringing my attention to IE11+ and Edge. The issue was occuring because those browsers has been marking selection as fake even when selection didn't exceed the original cell. I added an additional selection reset and now everything should work fine.

[f1ames]

@Dumluregn please take a look at CI, there is quite significant number of unit tests failing for editor build version (both Chrome and Firefox).

On the dev version only few tests are run with tableselection plugin (ones where it is explicitly added). On the build one, where full-all build is used, all default plugins (which means tableselection too) are present in most of the tests. And it seems that after recent changes, tableselection doesn't play well with the rest of the editor 😅

[Dumluregn]

Yeah, it seems I limited fake selection a little bit too much. Now CI is green again.

[f1ames]

Rebased on the latest major.

[f1ames]

LGTM, good job 👍

I found 2 not related issues in IE11 which I will extract to a separate tickets.

[f1ames]

Extracted two issues #3166 and #3167.