Fix for incorrect hover styles for buttons on mobile devices

[Comandeer]

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?

* [#4611](https://github.com/ckeditor/ckeditor4/issues/4611): Fixed: incorrect hover styles for buttons in the toolbar on mobile devices.

What changes did you make?

I've used hover media query to revert styles on :hover to the original ones. I've also restored :active styles after that.

I've changed styles for both buttons and rich combos.

Which issues does your PR resolve?

Closes #4611.

[f1ames]

TBH I can't recall many PRs not involving any JS code changes, this one looks nice in this regard 👍 😄

On iOS when you long tap buttons it focuses editable area if editor is not focused yet. And when it's already focused it does nothing (from what I see it's just the way it works there). But for regular tap it's visible there is a transition between hover and active styles. I wonder if we shouldn't adjust the scenario slightly to be not confuisng 🤔

On Android device I have tested with (Android 6.0.1, Chrome 89.0.4389.105) it still works the same as described in #4611:

• Short tap activates the button (with active styles which is fine), but another tab deactivates it with hover styles still visible (one have to tap outside of the editor to make hover styles disappear).
• Long tap activates hover styles but they do not disappear after releasing the finger.

---

I would be for splitting manual tests to desktop/mobile separately. On smaller screens desktop scenario is hardly visible which may be misleading:

WDYT?

The 2nd thing - I see styles were also updated for richcombo. Shouldn't we have that covered in a test scenario too?

[Comandeer]

On Android device I have tested with (Android 6.0.1, Chrome 89.0.4389.105) it still works the same as described in #4611:

Did you test in incognito mode? 🤔 It works correctly on my phone with Android, so maybe you hit some issues with cache.

As for iOS, I didn't have access to any device and I haven't tested it. I'll need to revisit it later.

[f1ames]

On Android device I have tested with (Android 6.0.1, Chrome 89.0.4389.105) it still works the same as described in #4611:

Did you test in incognito mode? It works correctly on my phone with Android, so maybe you hit some issues with cache.

Rechecked and I have the same issue there... 🤔

As for iOS, I didn't have access to any device and I haven't tested it. I'll need to revisit it later.

I mean it seems to be working fine but slightly different than on Android device (long tap does different thing) so maybe rewording test steps would be enough 🤔 Because for the initial scenario described in #4611 the behavior is now correct.

[Comandeer]

I've updated manual tests to separate mobile and desktop ones. I've also slightly adjusted mobile ones description to include possible iOS behaviour (yet I don't know how accurate the description is as I don't have access to any iOS device).

All tests are now moved to /tests/plugins/toolbar/manual/hover as there are 12 files in total 🙈

[f1ames]

Whooops... rebased this PR onto latest major instead of #4461 🤦 🙈 Reverting in progress...

[f1ames]

Thanks @Comandeer 👍

[f1ames]

Rebased onto latest master.

[f1ames]

Seems like Kama skin does not provide active styles for richcombo:

While for moono / moono lisa it looks like:

And also Kama does not provide hover styles for active rich combos... This makes this test scenario hard to describe and follow 🤔 I updated the test scenario but I'm not really happy with how complex it got... @Comandeer maybe you would have a better idea here?

I think it would be better like:

1. Check if buttons have hover styles.
2. Check if clicking on buttons activate their active state.
3. Check if active buttons have hover styles.
4. Repeat the procedure for rich combos.
5. Repeat the procedure for the second editor.

Then it will be easier to describe expected/unexpected for each step and notes could be placed in better context. WDYT?

---

As for iOS, long tap focuses the editor (if it's not focused yet) or does nothing if editor is already focused (during the tap, after releasing the button is active - so it works the sam way as regular tap). Hover styles could be seen after releasing the tap, before active styles are added.

So this scenario is kind of confusing / not true... I'm not sure... do you think we should have separate tests for Android and iOS here? Putting everything in a single tests scenario makes it quite complex 🤔

@Comandeer could you try to polish it a bit more?

[Comandeer]

Ok, so I've completely redesigned tests. There are now divided by:

• the environment (desktop, Android, iOS),
• the skin (Moono-Lisa, Moono, Kama),
• the type of UI element (button, rich combo).

So now we have 18 tests in total 🙈 But they should be much more readable and understandable.

I've also added stricter checking if the test should be run in the current env. It differentiates iOS and Android now.

[f1ames]

So now we have 18 tests in total But they should be much more readable and understandable.

I've also added stricter checking if the test should be run in the current env. It differentiates iOS and Android now.

A lot of tests and a bit of duplication but indeed seems like the most reasonable approach here 👍 I only fixed spacing in tests (since tabs doesn't work well with nested "Expected" blocks...).

LGTM 👍 🎉