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.

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

Prevent invocation on not existing editable [FF] #4800

Merged
merged 5 commits into from
Jul 30, 2021
Merged

Prevent invocation on not existing editable [FF] #4800

merged 5 commits into from
Jul 30, 2021

Conversation

sculpt0r
Copy link
Contributor

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?

* [#4782](https://github.com/ckeditor/ckeditor4/issues/4782): Fixed: TypeError in autocomplete for FF.

What changes did you make?

I just added some prevention in one if statement. So in case the editable is null nothing happened. The source of the issue is described here. Also this event is fired again anyway after the editable is fully initialized. So ignoring it one time (actually only in FF) doesn't break the feature.

I wasn't able to get this error when I switched modes with automatic tests. So I decided to use the manual one.

Which issues does your PR resolve?

Closes #4782 .

@f1ames f1ames self-assigned this Jul 26, 2021
@f1ames f1ames self-requested a review July 26, 2021 13:02
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

The fix itself looks reasonable and works fine 👍 I have only some doubts regarding manual test (see review comment).

I wasn't able to get this error when I switched modes with automatic tests. So I decided to use the manual one.

👍

@sculpt0r sculpt0r self-assigned this Jul 27, 2021
@sculpt0r
Copy link
Contributor Author

Hi, @f1ames - PR is ready for another review.

I've removed the gecko restriction from manual test. The mobile restriction is still there due to the dev console usage...

@sculpt0r sculpt0r requested a review from f1ames July 27, 2021 05:57
@sculpt0r sculpt0r linked an issue Jul 27, 2021 that may be closed by this pull request
@f1ames f1ames self-assigned this Jul 28, 2021
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

Almost there 😉 👍

@sculpt0r sculpt0r self-assigned this Jul 29, 2021
@sculpt0r
Copy link
Contributor Author

Hi @f1ames,
according to our F2F conversation - I'm re-requesting you. I correct proposals to the changelog.

@sculpt0r sculpt0r requested a review from f1ames July 29, 2021 11:35
@f1ames f1ames self-assigned this Jul 29, 2021
@f1ames
Copy link
Contributor

f1ames commented Jul 30, 2021

Rebased onto latest master.

Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

source tab view out gives "Uncaught TypeError: b is null in Firefox"
2 participants