Other: Remove env.isEdge. Closes ckeditor/ckeditor5#6202.

[tomalec]

Remove some special cases for Edge, as since it's Chromium-based now, it behaves closer to others.

BREAKING CHANGE: env.isEdge is no longer available in utils API, see #6202.

Suggested merge commit message (convention)

Other: Remove env.isEdge. Closes ckeditor/ckeditor5#6202.

Remove some special cases for Edge, as since it's Chromium-based now, it behaves closer to others.

---

Additional information

I removed all occurrences of env.isEdge and tests for specific behavior in case of its presence.
Run npm run test for Firefox, Chrome. I didn't find any manual tests related to it, so I just check a few random ones.

I was not sure, whether I should touch

1. https://github.com/ckeditor/ckeditor5-engine/blob/master/src/view/domconverter.js#L172
   Was the initial, non-Edge/pre-Edge16 intent to use
   domElement.childNodes.forEach? (https://caniuse.com/#search=forEach). I leave it as it is, as it's still uniform cross-browser solution.

2. https://github.com/ckeditor/ckeditor5-engine/blob/master/tests/view/renderer.js#L2059
   I guess Chromium-based Edge is no longer an exception here, but I assume it may be still neded for Safari.

3. https://github.com/ckeditor/ckeditor5-image/blob/master/src/imageupload/utils.js#L96
   According to https://developer.mozilla.org/en-US/docs/Web/API/File#Browser_compatibility since (Chromium-based) Edge 79 constructor is supported. However, I'm not sure whether it is in the scope of this issue.

4. https://github.com/ckeditor/ckeditor5-link/blob/master/src/linkui.js#L188
   I haven't dive deaper, but assume it's needed for FF and not in scope of this issue.

5. https://github.com/ckeditor/ckeditor5-paste-from-office/blob/master/src/filters/space.js#L43 ...

6. https://github.com/ckeditor/ckeditor5-table/blob/master/tests/converters/upcasttable.js#L207 ...

7. https://github.com/ckeditor/ckeditor5-theme-lark/blob/master/theme/ckeditor5-ui/components/button/switchbutton.css#L93 ...

8. https://github.com/ckeditor/ckeditor5-ui/blob/master/src/inputtext/inputtextview.js#L119 ...

9. https://github.com/ckeditor/ckeditor5-ui/blob/master/tests/icon/iconview.js#L134
   Seems 5-9 could be simplified, but I assume that it is not in the scope of this issue.

---

And naturally we could consider updating https://github.com/ckeditor/ckeditor5/blob/master/docs/builds/guides/support/browser-compatibility.md#desktop-environment

Branches on other repose linked to this change:

• https://github.com/ckeditor/ckeditor5-image/compare/i/6202-remove-env.isEdge

• https://github.com/ckeditor/ckeditor5-engine/compare/i/6202-remove-env.isEdge

• https://github.com/ckeditor/ckeditor5-horizontal-line/compare/i/6202-remove-env.isEdge

• https://github.com/ckeditor/ckeditor5-media-embed/compare/i/6202-remove-env.isEdge

• https://github.com/ckeditor/ckeditor5-mention/compare/i/6202-remove-env.isEdge

• https://github.com/ckeditor/ckeditor5-page-break/compare/i/6202-remove-env.isEdge

• https://github.com/ckeditor/ckeditor5-paste-from-office/compare/i/6202-remove-env.isEdge

• https://github.com/ckeditor/ckeditor5-table/compare/i/6202-remove-env.isEdge

• https://github.com/ckeditor/ckeditor5-widget/compare/i/6202-remove-env.isEdge

• https://github.com/ckeditor/ckeditor5-link/compare/i/6202-remove-env.isEdge

• https://github.com/ckeditor/ckeditor5-theme-lark/compare/i/6202-remove-env.isEdge

• https://github.com/ckeditor/ckeditor5-ui/compare/i/6202-remove-env.isEdge

• https://github.com/ckeditor/ckeditor5/compare/i/6202-remove-env.isEdge

[coveralls]

Coverage remained the same at 100.0% when pulling 153f576 on i/6202-remove-env.isEdge into fb13d9d on master.

[Reinmar]

1. https://github.com/ckeditor/ckeditor5-engine/blob/master/src/view/domconverter.js#L172
   Was the initial, non-Edge/pre-Edge16 intent to use
   domElement.childNodes.forEach? (https://caniuse.com/#search=forEach). I leave it as it is, as it's still uniform cross-browser solution.

AFAIR, yes: https://github.com/ckeditor/ckeditor5-engine/blame/3d494a37bff839ef5c3f712343d5269ed20436d2/src/view/domconverter.js#L149.

We could revert that change as it may be confusing one day. OTOH, I don't remember if for-each is identical to Array.from in terms of whether it works on a copy of a live collection and whether childNodes is a live collection. It would not be funny if we'd revert this change and it turned out that at some point between switching to Array.from and now we started relying on the static nature of a for loop over Array.from :D 

2. https://github.com/ckeditor/ckeditor5-engine/blob/master/tests/view/renderer.js#L2059
I guess Chromium-based Edge is no longer an exception here, but I assume it may be still neded for Safari.

No idea. You'd need to change this line: ckeditor/ckeditor5-engine@045fdad#diff-81db83bc0746c7f974c179f77699a073R1285 (which accepts space or nbbs) and run the tests.

3. https://github.com/ckeditor/ckeditor5-image/blob/master/src/imageupload/utils.js#L96
According to https://developer.mozilla.org/en-US/docs/Web/API/File#Browser_compatibility since (Chromium-based) Edge 79 constructor is supported. However, I'm not sure whether it is in the scope of this issue.

By the comment, I think you can remove that try-catch now. I'd consider it in the scope of this ticket.

4. https://github.com/ckeditor/ckeditor5-link/blob/master/src/linkui.js#L188
I haven't dive deaper, but assume it's needed for FF and not in scope of this issue.

I'd assume the same. So he only thing to do here is removing the mention of Edge.

• https://github.com/ckeditor/ckeditor5-paste-from-office/blob/master/src/filters/space.js#L43 ...

That made me think that we may need to refresh all the assets (see https://github.com/ckeditor/ckeditor5-paste-from-office/tree/master/tests), but from what I can see we've got special assets only for Safari, not Edge. So it seems that Edge was getting the same HTML as Chrome and FF and thankfully we don't need to do anything here now.

As for the linked code, I'd leave the current implementation unless you want to analyze what could've been there if not old Edge :D

6. https://github.com/ckeditor/ckeditor5-table/blob/master/tests/converters/upcasttable.js#L207 ...

No idea. I'd just remove the comment.

7. https://github.com/ckeditor/ckeditor5-theme-lark/blob/master/theme/ckeditor5-ui/components/button/switchbutton.css#L93 ...

You could replace this var with the original calculation if it works in Edge. But we can live with that (although, I'd leave the comment as it explains the calculation). You can mention that it applied to old Edge.

8. https://github.com/ckeditor/ckeditor5-ui/blob/master/src/inputtext/inputtextview.js#L119 ...

I'd remove it as it is as it feels more natural to me than using the attribute heavily.

9. https://github.com/ckeditor/ckeditor5-ui/blob/master/tests/icon/iconview.js#L134
Seems 5-9 could be simplified, but I assume that it is not in the scope of this issue.

I'd clean up the tests now.

And naturally we could consider updating https://github.com/ckeditor/ckeditor5/blob/master/docs/builds/guides/support/browser-compatibility.md#desktop-environment

I can see that one of the linked issues is solved. Also, dev tools should not cause any issues anymore as they use the same engine that Chrome uses. I know that we were able to close most Edge related issues once the Blink-based version was resolved, so I think that we can move Edge to the "Full support" section now. You can make a branch in the main repo for that.

---

One more note – we use the i/<number> convention for branch names. We do not add additional textual description to the branch names. I'll cover that in the knowledge base. Open to discuss this, of course.

[Reinmar]

Could you test whether we can remove this: https://github.com/ckeditor/ckeditor5-paste-from-office/compare/i/6202-remove-env.isEdge#diff-cfd2fd58320fbf691cd712eff0c462c3R48? I'm not sure at what point did those tests cause issue on Edge. I guess the easiest way to check that will be by running theses tests on Edge.

[Reinmar]

I reviewed all changes and had just one comment there. All's good, in general.

I also replied to the linked code lines that were unclear.

[tomalec]

I'll try to apply all the changes you requested. Just few more clarifying questions:

8. ckeditor/ckeditor5-ui:src/inputtext/inputtextview.js@master#L119 ...

I'd remove it as it is as it feels more natural to me than using the attribute heavily.

Do you mean "I'd remove it, as it feels ..." or "I'd leave it as it is, as it feels ..."

---

Could you test whether we can remove this: ckeditor/ckeditor5-paste-from-office@i#diff-cfd2fd58320fbf691cd712eff0c462c3R48 (compare)? I'm not sure at what point did those tests cause issue on Edge. I guess the easiest way to check that will be by running theses tests on Edge.

I did check it, and something was failing, but will investigate deeper, maybe it's something that is a low hanging fruit now.

[tomalec]

Regarding #333 (comment)

One more note – we use the i/<number> convention for branch names. We do not add additional textual description to the branch names. I'll cover that in the knowledge base. Open to discuss this, of course.

Sure, I'll change it. I just got used to adding short descriptions, as my memory is too bad to keep track of issue numbers. When I work on a few branches in the same repo, I like to have something meaningful to distinguish one from another.

[tomalec]

1-4. done

5. 5. https://github.com/ckeditor/ckeditor5-paste-from-office/blob/master/src/filters/space.js#L43
      That made me think that we may need to refresh all the assets (see ckeditor/ckeditor5-paste-from-office:tests@master ), but from what I can see we've got special assets only for Safari, not Edge. So it seems that Edge was getting the same HTML as Chrome and FF and thankfully we don't need to do anything here now.

   As for the linked code, I'd leave the current implementation unless you want to analyze what could've been there if not old Edge :D

   I'd risk reverting to const innerTextLength = el.innerText.length || 0;, as in https://github.com/ckeditor/ckeditor5-paste-from-office/blame/eca95b1678174341c073ba2c9456cdf0038be6c1/src/filters/utils.js#L112
   As all tests pass in Chrome and Edgium, but maybe not in this PR, @Reinmar WDYT?

6-7. done

8. see the question above
9. done
10. Browser compatibility in the docs:

---

New branches:

• https://github.com/ckeditor/ckeditor5-link/compare/i/6202-remove-env.isEdge
• https://github.com/ckeditor/ckeditor5-theme-lark/compare/i/6202-remove-env.isEdge
• https://github.com/ckeditor/ckeditor5-ui/compare/i/6202-remove-env.isEdge
• https://github.com/ckeditor/ckeditor5/compare/i/6202-remove-env.isEdge

---

Do you want me to change the branch name, or just leave it, as it will be removed anyway after the merge?

[Reinmar]

• ckeditor/ckeditor5-engine@66b8a7f fails on FF according to the CI.

Do you mean "I'd remove it, as it feels ..." or "I'd leave it as it is, as it feels ..."

I'd leave it as it is. Sorry :)

[Reinmar]

The change in the engine breaks tests on FF so I'll change it to only cover the comment there.

[tomalec]

Do you mean "I'd remove it, as it feels ..." or "I'd leave it as it is, as it feels ..."

I'd leave it as it is. Sorry :)

Then 8. is also done, by doing nothing ;)

---

2. ckeditor/ckeditor5-engine@66b8a7f fails on FF according to the CI.
   The change in the engine breaks tests on FF so I'll change it to only cover the comment there.

   Maybe I'll change to comment to mention, that now the FF is the one-off?

[tomalec]

Maybe I'll change to comment to mention, that now the FF is the one-off?

Damn, you were 30sec faster than me. 🏁 🏎️

[Reinmar]

All done already :) In all repos other than this I merged this as an internal change because it does not need to be mentioned in their changelogs.

Let's now hope that CI will pass.