Fix: Editor Placeholder plugin throws an error during editor initialization when used with bbcode and fullpage

[sculpt0r]

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches that 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

• Unit tests
• Manual tests

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

• PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4253](https://github.com/ckeditor/ckeditor4/issues/4253): Prevent index-reading from undefined variable.

What changes did you make?

Check RegExp match results before trying access via index.

Which issues does your PR resolve?

Closes #4253

[f1ames]

Looks good, some polishing in tests needed 👍

[f1ames]

Looks good, some polishing of unit tests needed 👍

---

There is also one issue we have stumbled upon earlier (see #4251 (review)):

Also for some reason for empty editor, getData() returns one empty space (so length 1 instead of 0 which doesn't help here). And yes, it is an empty space, so there is now way to distinguish if editor is empty or someone just typed space because charCode is the same.

So we can only fix error being thrown for bbcode + fullpage but editor placeholder will not be displayed anyway (that's how it works now, which is good) due to single space character always being returned. But as @sculpt0r mentioned:

It appears bbcode plugin filter-out all non-body content.

This is very rare edge case so we can accept that.

[f1ames]

Rebased onto latest master.

[f1ames]

One test fails in IE9 (probably IE10 too but haven't checked):

The issue here is that, without dev console open, older IEs don't have console object available... So probably stubbing console.log for older IEs should be skipped. I think it's not a problem here since the error the test is checking for is runtime error so it still will be caught.

[sculpt0r]

One test fails in IE9 (probably IE10 too but haven't checked):

First, I introduce pre-check with if (window.console) before stubbing console in the test. With the issue fix, all tests passed. Tested on IE (11, 10, 9), Chrome, FF, Safari(mobile), Chrome (android).

IE 8 is ignored for all editorplaceholder plugin.

Then, run a single runtime checking error test, but with old-broken logic in the plugin.
IE (11, 10), Chrome, FF, Safari(mobile), Chrome (android) - test failed, but with proper error catch. So it's ok.
IE 9 - failed with theoretically proper error catch, but still with console errors

Even with console open, I've got

So I'm moving to skip console stubbing with pre-check and also CKEDITOR.env.ie && CKEDITOR.env.version < 10

All tests made with BrowserStack.

IE9 console error appears due to console.error() invoked as we expect runtime error in old-broken plugin logic. So I decide to ignore runtime check error test for IE below version 10.

[f1ames]

LGTM 👍 🎉

Agree that skipping runtime error checks for IE9 as mentioned in #4370 (comment) makes sense 👍

---

Unit tests

• Chrome:heavy_check_mark:
• Firefox:heavy_check_mark:
• Safari:heavy_check_mark:
• Safari@iOS:heavy_check_mark:
• Chrome@Android:heavy_check_mark:
• Edge:heavy_check_mark:
• IE11:heavy_check_mark:
• IE8:heavy_check_mark:
• IE9:heavy_check_mark:
• IE10:heavy_check_mark:

[f1ames]

Rebased onto latest master.