Show current color in the Color Dialog

[Dumluregn]

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

This PR contains

• Unit tests
• Manual tests

What changes did you make?

I added a new function to colordialog plugin (not changing dialog definition as it is used in other plugins). Right now it sets selection color as selected and shows its #code in color dialog. It does not change highlight or select color on palette yet, but besides that it works as intended. The code needs a lot of refactoring too. I just want to know if my proposal logic is valid and I can continue or I should find another way.

Closes #2639.

[f1ames]

The code needs a lot of refactoring too. I just want to know if my proposal logic is valid and I can continue or I should find another way.

Remember that you can always create Draft PR for such cases.

---

Please see t/2639-2 branch (there is basically one commit - 055ea29). I prepared some quick PoC, you can see how you can reuse color detected on opening color dropdown without duplicating detection logic and how to pass it to color dialog via show event.

not changing dialog definition as it is used in other plugins

Did you encountered issues with other plugins, what approach was problematic?

[Dumluregn]

Remember that you can always create Draft PR for such cases.

I thought reviewers do not look at drafts, sorry, should've ask about it before.

Please see t/2639-2 branch (there is basically one commit - 055ea29). I prepared some quick PoC, you can see how you can reuse color detected on opening color dropdown without duplicating detection logic and how to pass it to color dialog via show event.

This is obviously a much better way to do this. Great to know we have such an awesome API for events - I didn't know I can edit event before passing it to another listener, lesson learned, thanks!

not changing dialog definition as it is used in other plugins

Did you encountered issues with other plugins, what approach was problematic?

I found out colordialog is used in several other plugins and wanted to avoid regression errors, but with event editing logic and line:

if ( evt.data.colordialog && evt.data.colordialog.selectedColor )

there should be no issues. I will use your PoC and come up with a better solution.

[Dumluregn]

I implemented solution according to suggestion and manual tests covering all changes.

[Dumluregn]

PR is ready for review.

[Comandeer]

I'm not sure if we should show default font color (not applied with color button) inside the dialog (colorbutton shows it as "automatic"). I think that we should show only the ones actually applied by the plugin. IMO showing default styles (not only colors, but also font families and sizes etc.) should be extracted to separate issue.

I'm also not sure if we should consider text in whole selection (however it seems to be implemented this way in colorbutton). Most of our plugins use info from the beginning of the selection only (see #745).

There are no unit tests for this changes. It should be easy to use bot.dialog API to simulate opening of the dialog and checking what color is selected.

I also don't understand why tests/plugins/colordialog/colordialog.js was renamed?

Sometimes focus is inconsistent with the value in the input:

[Dumluregn]

I'm not sure if we should show default font color (not applied with color button) inside the dialog (colorbutton shows it as "automatic"). I think that we should show only the ones actually applied by the plugin. IMO showing default styles (not only colors, but also font families and sizes etc.) should be extracted to separate issue.

Do you suggest focusing color in the top left corner and leaving selected color box empty if default one is chosen (as it is currently on master branch)?

I'm also not sure if we should consider text in whole selection (however it seems to be implemented this way in colorbutton). Most of our plugins use info from the beginning of the selection only (see #745).

I didn't change logic here, so if it should be changed I think we should create a separate task to avoid overlooking this later.

There are no unit tests for this changes. It should be easy to use bot.dialog API to simulate opening of the dialog and checking what color is selected.

You're right, I will add unit test here.

I also don't understand why tests/plugins/colordialog/colordialog.js was renamed?

I changed it because the name addinghash.js seemed more descriptive about what is tested here to me. If the tests for this issue are added to the same file the former name will be still proper and I will restore it.

Sometimes focus is inconsistent with the value in the input

It happens in case of choosing color from outside a palette, i.e. by typing it's hex code. We discussed this case with @f1ames and decided it's better to focus default palette color than not set focus and highlight at all.

[Comandeer]

Do you suggest focusing color in the top left corner and leaving selected color box empty if default one is chosen (as it is currently on master branch)?

Yes, that's exactly what I'm proposing.

[f1ames]

@Dumluregn please fix failing test (see CI):

[Comandeer]

Rebased onto latest major.

[Comandeer]

Scenario 1 in IE8:

Initial opening of the dialog places focus in incorrect place.

Scenario 2 in IE8:

Opening the dialog after following all steps shows incorrect selected color.

[Comandeer]

The mentioned issue is still present, this time if color is not set (e.g. in the second case in manual test).

Additionally please rebase the branch onto latest major.

[Dumluregn]

Ok, so there are two questions about colordialog look.

The first one is directly connected with this PR. What should be the colors of UI elements on dialog show if you selected text with inconsistent color, e.g.
<h1>Hello <span style="color:#3399cc">world</span>!</h1>?

1. Default dialog look, i.e. focus and highlight set on the top left color (black), no selected color, no color code. The problem here is that focus is on black and selected color is white (actually it isn't but it looks so) so it's a little inconsistent.
2. Hands-off: no focus, no highlight, no selected color, no color code. Bad thing is that focus stays in the background (i.e. in the editor), so the accessibility is impaired.
3. Default dialog as in 1. but with the word none (or sth similar) in code field. This would be more readable for user but the color input is quite narrow and in some languages the equivalent of none may turn out to be too wide to fit in.

Of course if you have different ideas please give them.

The second question is about colordialog layout and do we want to change it (in the other PR then).
I think right now the right column of dialog is a bit messy. The RGB code below the highlighted color looks like it belonged to the selected color. The highlight and selected color boxes should be the same size, and there should be a bit of space between. I'd propose something like this:

We could go even further and try to add some sample text (like lorem ipsum) in dialog that would change its color with either highlight or selected color, because the box is not always a good indicator of how the text will look.

WDYT?

[Dumluregn]

As a reference - for inconsistent text color gdocs is selecting top left color (i.e. red).

[engineering-this]

Default dialog look, i.e. focus and highlight set on the top left color (black), no selected color, no color code. The problem here is that focus is on black and selected color is white (actually it isn't but it looks so) so it's a little inconsistent.

I'm for this. To solve problem with selected color I'd consider just hiding it from dialog when no single color can be detected. Alternatively replace hash with No selected color, like in option 3., but with full sentence.

[Dumluregn]

Yeah, that git history was odd indeed. I fixed it, rebased onto the latest major and deleted the last commit 'fixing' the editor version number in the changelog. We should be ready to go now.

[Comandeer]

LGTM!