Problem/Motivation

Standard behavior for text-receiving inputs is a green text ring on focus. While focused, these inputs should still respond to hover in the same manner: a darker and slightly larger border.
The current WYSIWYG field inputs behavior is to change the border color to blue on focus, and no visual change is provided for hover when this blue border is present. This should be addressed as it provides both consistency and better accessibility.

Current focus effect
WYSIWYG current

Proposed resolution

Remove the current blue border change on focus.
Add the green focus ring and do so in a way that preserves hover behavior even while focused in the style guide.

Mockups / Specifications

Example

WYSIWYG figma solution

Note that red border on error should remain visible even if the focus ring is present, the focus ring should surround this.

Screenshots

After
Pictured: error@focus, focus, hover, default
After patch applied screenshot

Remaining tasks

User interface changes

CKEditor field inputs will have the same focus and hover styling that other text inputs have.

CommentFileSizeAuthor
#66 Screen Recording 2020-04-14 at 13.36.30.gif948.82 KBlauriii
#64 interdiff_62-64.txt1.62 KBkostyashupenko
#64 3078524-64.patch9.09 KBkostyashupenko
#62 interdiff_61-62.txt3.44 KBkostyashupenko
#62 3078524-62.patch9.21 KBkostyashupenko
#61 interdiff_57-61.txt959 bytesneelam_wadhwani
#61 3078524-61.patch8.16 KBneelam_wadhwani
#58 selected_8471.png126.33 KBKondratievaS
#57 interdiff_54-57.txt2.19 KBprabha1997
#57 3078524-57.patch7.89 KBprabha1997
#54 interdiff_50-54.txt2.74 KBkostyashupenko
#54 3078524-54.patch7.7 KBkostyashupenko
#51 interdiff-43-50.txt833 bytesthalles
#50 interdiff-43-50.patch833 bytesthalles
#50 claro_wysiwyg_body_field_focus_update_3078524-50.patch7.37 KBthalles
#49 Screenshot from 2020-03-26 12-49-18.png18.4 KBthalles
#49 Screenshot from 2020-03-26 12-49-53.png17.06 KBthalles
#44 selected_7836.png136.3 KBKondratievaS
#43 After Modify patch#39.png181.21 KBpranav45
#43 After applying patch#39.png173.92 KBpranav45
#43 claro_wysiwyg_body_field_focus_update_3078524-43.patch7.98 KBpranav45
#39 claro-wysiwyg_focus_update-3078524-39--do-no-test.patch7.37 KBMeenakshiG
#36 interdiff-3078524-28-36.txt1.66 KBhuzooka
#36 claro-wysiwyg_focus_update-3078524-36--do-no-test.patch8.28 KBhuzooka
#29 screenshot for when_focus_textarea.png86.14 KBVinodhini.E
#29 Screenshot for when type text.png88.05 KBVinodhini.E
#28 textareaScreenshots--windows-ubuntu.zip3.97 MBhuzooka
#28 textareaScreenshots--macos-and-mobile.zip6.62 MBhuzooka
#28 interdiff-3078524-27-28.txt864 byteshuzooka
#28 claro-wysiwyg_focus_update-3078524-28.patch7.51 KBhuzooka
#27 claro-wysiwyg_focus_update-3078524-27.patch7.2 KBhuzooka
#23 Screenshot at 2019-11-15 12-29-09.png91.49 KBshimpy
#23 Screenshot at 2019-11-15 12-29-39.png91.87 KBshimpy
#23 Screenshot at 2019-11-15 12-33-37.png67.88 KBshimpy
#23 Screenshot at 2019-11-15 12-35-08.png87.42 KBshimpy
#22 3078524-22.patch3.72 KBMaithri Shetty
#21 Claro-focused-hovered-wysiwyg-with-error.png81.4 KBhuzooka
#21 Claro-focused-hovered-textarea-with-error.png47.24 KBhuzooka
#20 Capture d'écran 2019-10-31 16.19.25.png399.07 KBcedric_a
#19 review-3078524-patch-15-FF.JPG70.88 KBpminf
#19 review-3078524-patch-15-Chrome.JPG60.09 KBpminf
#19 review-3078524-patch-15-IE11.JPG65.81 KBpminf
#19 review-3078524-patch-15-Edge.JPG67.47 KBpminf
#15 claro-green_focus_textarea-3078524-15.patch3.59 KBemma.maria
#10 claro-green_focus_textarea-3078524-10.patch1.8 KBckrina
#10 textarea.png157.48 KBckrina
#4 claro-green-focus-ring-wysiwyg-field-3078524-4.patch2.5 KBkatrienc
#3 claro-green-focus-ring-wysiwyg-field-screenshot-3078524-3.png13.24 KBkatrienc
#3 claro-green-focus-ring-wysiwyg-field-3078524-3.patch846 byteskatrienc
#2 Screenshot 2019-08-31 21.52.32.png89.73 KBfhaeberle
Screenshot 2019-08-31 21.42.25.png116.98 KBfhaeberle
Screenshot 2019-08-31 21.41.09.png101.66 KBfhaeberle
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fhaeberle created an issue. See original summary.

fhaeberle’s picture

Issue summary: View changes
FileSize
89.73 KB
katrienc’s picture

Patch that implements the ring on the WYSIWYG field.

claro-green-focus-ring-wysiwyg-field-screenshot

katrienc’s picture

fhaeberle’s picture

Status: Needs review » Needs work

I'm wondering here because the Figma WYSIWYG elements don't have a border in focus state except of the top border.

@lot007 Your screenshot/patch doesn't reflect that and lefts the borders as is and adds the green focus ring. I don't know if we should keep this behavior or change according to the design.

Anyway, we should be consistent and one part has to update. Lets get design review/approval for this.

huzooka’s picture

Marginal note: Editor hover on IE and Edge won't work as expected.

katrienc’s picture

Assigned: Unassigned » katrienc
katannshaw’s picture

@lot007: I've tested your #4 patch on my Mac with Chrome, Firefox, and Safari and can verify that it solved the issue. I can't test in IE and IE Edge so if someone can test in those and verify that it works we should be good-to-go.

@huzooka: I reviewed the code regarding the ckeditor iframe because I've seen strange behavior with it before and this patch only removes Claro's handling of :focus (blue) from ckeditor and regular form fields, letting the default admin :focus behavior (green) work. So it *shouldn't* be an issue.

-Kat

fhaeberle’s picture

I would really set this to needs review but I'm concerned that #5 still needs clarification if we color the actual border green or if we apply the green focus outline on focus.

ckrina’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
157.48 KB
1.8 KB

I can confirm the correct design is with the green focus, I just updated Figma and uploaded the summary issue with it.

The patch needed a reroll, so here it is.

It still needs IE testing or work as per comment #6.

fhaeberle’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev
Component: Needs design » Code
Assigned: katrienc » Unassigned
Status: Needs review » Reviewed & tested by the community

As the described concerns in #6 only targeting the hover effect (could be a follow-up) and after testing I'm setting this to RTBC.

katrienc’s picture

I think the hover effect in IE (and Edge) is only possible with adding some javascript.

The editor has 3 'parts':
- top (with the make bold, make italic, ... buttons)
- content: that's where you put your content and is an iframe
- bottom: show explanation about the tag (like body > p)

when you go above the top or bottom field then IE (and Edge) will like it. But not over the content part because it is an iframe.

The javascript could look like the following (but I'm not really a javascript specialist so there is maybe a better solution to do this)

/**
 * @file
 * Claro's polyfill enhancements for CKEditor.
 */

(($, Drupal) => {
    /**
     * Workaround for Internet Explorer.
     *
     * Internet Explorer and Edge doesn't react on hover events in the IFrame.
     *
     * @type {Drupal~behavior}
     */
    Drupal.behaviors.claroCKEditor = {
      attach(context) {
        $(context)
        .once('claroCKEditor')
        .on('mouseover', (event) => {
            if (event.target.nodeName === 'IFRAME' && $(event.target).hasClass('cke_wysiwyg_frame')) {
                $(event.target).parent().parent().addClass('cke_hover');
            }
          })
          .on('mouseout', (event) => {
            if (event.target.nodeName === 'IFRAME' && $(event.target).hasClass('cke_wysiwyg_frame')) {
                $(event.target).parent().parent().removeClass('cke_hover');
            }
          });
          
      },
    };
})(jQuery, Drupal);

It simply sets an extra class on the parents parent 'cke_hover'
In the css we then need to add the style for the cke_hover class (but only for IE and Edge).

Maybe there's a javascript specialst that has a better solution?

huzooka’s picture

Project: Claro » Drupal core
Version: 8.x-2.x-dev » 8.9.x-dev
Component: Code » Claro theme
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
emma.maria’s picture

I'm rerolling this patch now at the DrupalCon sprints.

emma.maria’s picture

I've manually rerolled the patch as the file locations have since moved.

I also added an extra change as I noticed that we can further condense down the selectors here:

+++ b/css/src/theme/ckeditor-editor.css
@@ -75,14 +75,13 @@
 .cke.cke_chrome.cke_focus,
-.cke.cke_focus .cke_contents,
-.cke.cke_focus .cke_top,
-.cke.cke_focus .cke_bottom,
+.cke.cke_focus,
 .error + .cke.cke_chrome.cke_focus,
-.error + .cke.cke_focus .cke_contents,
-.error + .cke.cke_focus .cke_top,
-.error + .cke.cke_focus .cke_bottom {

The patch in #11 did not remove .cke.cke_chrome.cke_focus, when we are adding .cke.cke_focus, to the selector list, so I removed that line too.

This is my first patch in years so I hopefully did it right 🤞

emma.maria’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
emma.maria’s picture

Issue tags: +Amsterdam2019, +Novice
pminf’s picture

I will test this on Windows with latest FF, Chrome, Edge and IE11.

pminf’s picture

I'm happy to confirm that the green focus ring is visible in FF, Chrome, Edge and even IE11! So there are no JS tweeks needed (as assumed in #12). Changes from #15 look fine to me.

With a final test on Safari we can mark this RTBC.

cedric_a’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
399.07 KB

I have tested the patch on #15 on Safari (Version 13.0.3) and confirm that it is fixed as shown in the screenshot.

huzooka’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
47.24 KB
81.4 KB

The error state styles are not consistent with the existing input error styles:

The expected behavior – textarea (or textfield) with error, :focus:hover (the border is still red):

WYSIWYG editor with error, :focus and :hover with patch #15 (the border color becomes black):

Maithri Shetty’s picture

Status: Needs work » Needs review
FileSize
3.72 KB

WYSIWYG editor with error, :focus and :hover changed color to red

shimpy’s picture

I have tested patch #22.
Textarea and Textfield border is showing green color as per styles. :hover is showing red color border.

While entering the text in the textarea and also keeping the :hover it is showing both green and red color border.

:hover textarea

Entering text in text area

Entering text in textfield

:hover and entering text in textarea

shimpy’s picture

Status: Needs work » Reviewed & tested by the community
huzooka’s picture

Status: Reviewed & tested by the community » Needs work

After applying #22, editors without any errors get red border, exactly like on the first screenshot in #23.

huzooka’s picture

Assigned: Unassigned » huzooka
huzooka’s picture

Assigned: huzooka » Unassigned
Status: Needs work » Needs review
FileSize
7.2 KB

Screenshots will be attached later.

huzooka’s picture

Screenshots (and a new patch with a minor, partially related fix) attached.

Vinodhini.E’s picture

I have tested patch #28.
While entering the text in the textarea :hover it is showing black color.
when focusing the textarea :focus it is showing both green and black color.

DyanneNova’s picture

Status: Needs review » Reviewed & tested by the community

I've tested patch #28 and can confirm that it fixes the issue with lack of focus. Now the WYSIWYG field has a green focus, with a red border inside for an error, and a black border inside when being hovered.

The text field should be updated to also show the red error border when focused but not hovered, but that's out of scope for this issue.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

As far as I know, the expected behavior is that the border remains red when a field in an error state is being hovered. Text fields already work this way too, which to my knowledge is correct. This has been described in #21.

lauriii’s picture

huzooka’s picture

@lauriii, did you test my patch in #28?

lauriii’s picture

@huzooka I think so. Tested #28 again and got the same result.

huzooka’s picture

Assigned: Unassigned » huzooka
huzooka’s picture

Assigned: huzooka » Unassigned
Status: Needs work » Needs review
FileSize
8.28 KB
1.66 KB
DyanneNova’s picture

Status: Needs review » Reviewed & tested by the community

@lauriii Text fields don't currently work that way in my testing so far (they show black when hovered, and no border other than the focus color when focused) but I agree that keeping the red border would be preferred, so this looks good to go. I can open a follow up issue to change text fields to match if that's the preferred pattern.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll, +Needs screenshots

It seems like the patch needs to be rerolled. Would be also good to add screenshots to the issue summary to make sure everyone is on the same page how this works.

MeenakshiG’s picture

Status: Needs work » Needs review
FileSize
7.37 KB
mradcliffe’s picture

Issue summary: View changes
Issue tags: -Needs reroll

Thank you for the re-rolled patch, @Meenakshi.g!

I removed the Needs re-roll tag after I confirmed that this patch still applies on the latest 8.9.x as of today. I think this is still a Novice issue. It looks like the last thing is to manually test the latest patch and update the before and after screenshots in the issue summary.

I expanded on the remaining tasks to give this a bit more detail.

fhaeberle’s picture

Status: Needs review » Reviewed & tested by the community

I tested the patch provided in #39 and it looks pretty good. I did manual browser testing in all major browsers and therefore RTBC.

fhaeberle’s picture

pranav45’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.98 KB
173.92 KB
181.21 KB

Have tested patch from #39 on drupal 8.9.x-dev version and it seems not working. While entering the text in the body field :hover it is showing black color.when focusing the body :focus it's not showing green color. So here is Updated patch and attachement before and after applying my patch

KondratievaS’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
136.3 KB

Tested in Chrome, FF, IE11, IE Edge, Safari and result is OK

OK

Status: Reviewed & tested by the community » Needs work
mradcliffe’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs screenshots

I added a retest of the patch in #43 last night because it seemed like a random failure. Sure enough, the tests pass. I'm resetting the status back to RTBC for now. I removed the Needs screenshots tag.

I added @KondratievaS screenshot to the issue summary.

The issue summary could still use an updated Before screenshot.

mradcliffe’s picture

Issue tags: +midcamp2020

Added an event tag.

KondratievaS’s picture

Status: Needs review » Reviewed & tested by the community
thalles’s picture

thalles’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.37 KB
833 bytes

I add opacity to cross browser to #43

thalles’s picture

FileSize
833 bytes
bnjmnm’s picture

Status: Needs review » Needs work

Just to be redundant I tested this on all OSX browsers plus IE11 on Virtualbox and it worked as hoped, just like the earlier comments.

  1. +++ b/core/themes/claro/css/components/form--text.pcss.css
    @@ -115,7 +115,9 @@ _:-ms-fullscreen,
     .form-element[disabled] {
       -webkit-opacity: 1;
    +  opacity: 1;
       color: var(--input--disabled-fg-color);
    +  border-width: var(--input-border-size);
       border-color: var(--input--disabled-border-color);
    

    I don't think either of the added styles are needed. .form-element already has border: var(--input-border-size) and I don't see any styles making the opacity of .form-element or [disabled] anything other than the default value of 1.

  2. +++ b/core/themes/claro/css/theme/ckeditor-editor.pcss.css
    @@ -5,12 +5,20 @@
    +:root {
    +  --chrome-border-size: var(--input-border-size); /* 1px */
    +  --chrome-border-radius: 2px;
    +  --inner-border-size: calc(var(--input--error-border-size) - var(--chrome-border-size)); /* 1px */
    +  --inner-border-radius: calc(var(--chrome-border-radius) - var(--chrome-border-size));
    +}
    

    --chrome-border-radius can be the value of --base-border-radius. These would also benefit from a comments explaining what is getting calculated here.

  3. +++ b/core/themes/claro/css/theme/ckeditor-editor.css
    @@ -56,10 +57,12 @@
     .cke.cke_chrome {
       border-radius: 0.125em; /* (2 / 16) */
    +  border-width: 1px;
    +  border-radius: 2px;
     }
    

    There shouldn't be two border-radius rules in a compiled file. I ran yarn build:css and this file and elements.css both were changed. Perhaps somwhere in the chain manual changes were being made to the .css files instead of the .pcss.css files. The next patch should have the recompiled files and another round of manual testing will be needed.

kostyashupenko’s picture

Assigned: Unassigned » kostyashupenko
kostyashupenko’s picture

Assigned: kostyashupenko » Unassigned
Status: Needs work » Needs review
FileSize
7.7 KB
2.74 KB
kostyashupenko’s picture

I also think it's better to rename these variables:
(from /css/theme/ckeditor-editor.pcss.css)

:root {
  --chrome-border-size: ...
  --chrome-border-radius: ...
  --inner-border-size: ...
  --inner-border-radius: ...
}

to

:root {
  --ckeditor-chrome-border-size: ...
  --ckeditor-chrome-border-radius: ...
  --ckeditor-inner-border-size: ...
  --ckeditor-inner-border-radius: ...
}

Since it is a part of only that component, so better stick these variables to its component.

prabha1997’s picture

Assigned: Unassigned » prabha1997
prabha1997’s picture

Assigned: prabha1997 » Unassigned
FileSize
7.89 KB
2.19 KB

Kindly review patch

KondratievaS’s picture

FileSize
126.33 KB

Tested patch from #57 and result is OK

OK

Still need dev to check latest changes

bnjmnm’s picture

Status: Needs review » Needs work

Nice fast responses on this, thanks!

One tiny thing and this may be ready:

+++ b/core/themes/claro/css/theme/ckeditor-editor.pcss.css
@@ -5,12 +5,20 @@
+  --ckeditor-inner-border-size: calc(var(--input--error-border-size) - var(--ckeditor-chrome-border-size)); /* 1px */
+  --ckeditor-inner-border-radius: calc(var(--ckeditor-chrome-border-radius) - var(--ckeditor-chrome-border-size));

This could still use a comment explaining what is being accomplished by calc() as opposed to using a fixed value.

neelam_wadhwani’s picture

Assigned: Unassigned » neelam_wadhwani
neelam_wadhwani’s picture

Assigned: neelam_wadhwani » Unassigned
Status: Needs work » Needs review
FileSize
8.16 KB
959 bytes

Kindly review patch.

kostyashupenko’s picture

Fixed comments and lint.
@neelam_wadhwani please run yarn build:css && yarn lint:css before patch next time ;)

bnjmnm’s picture

Title: Implement green focus ring on WYSIWYG field » Focus styling of WYSIWYG field inputs should be consistient with other intpus
Issue summary: View changes
Status: Needs review » Needs work

One tiny thing related to the most recent change!

+++ b/core/themes/claro/css/theme/ckeditor-editor.pcss.css
@@ -2,15 +2,24 @@
+  /* Calculation of inner border size is dynamic depending upon input and chrome border size. */
+  --ckeditor-inner-border-size: calc(var(--input--error-border-size) - var(--ckeditor-chrome-border-size)); /* 1px */
+  /* Calculation of inner border radius is dynamic depending upon chrome border radius and border size. */

Apparently this doesn't get caught by lint, but this comment exceeds 80 characters so it cant use the single line comment style. Reducing the comment length will probably be easiest - something like /* Calculation of inner border size must be based on chrome border size. */

kostyashupenko’s picture

Status: Needs work » Needs review
FileSize
9.09 KB
1.62 KB
bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

Feedback is all addressed, manual testing has been backed up by screenshots from contributors, and this patch applies to 9.1 as well.

lauriii’s picture

On IE 11 and Edge the CKEditor isn't hovered when the pointer is on the textarea:

Is this something we believe could be fixed?

bnjmnm’s picture

Re #66 I should have explicitly mentioned that in my testing. I also ran into the IE 11 hover styling not working over the textarea, but confirmed it's pre-existing behavior.

lauriii’s picture

Issue tags: +Needs followup

Let's open follow-up for #66.

bnjmnm’s picture

lauriii’s picture

Title: Focus styling of WYSIWYG field inputs should be consistient with other intpus » Focus styling of WYSIWYG field inputs should be consistent with other inputs
lauriii’s picture

+++ b/core/themes/claro/css/base/elements.css
@@ -310,6 +310,18 @@ img {
 .page-wrapper *:focus,
 .ui-dialog *:focus {
-  outline: 2px dotted transparent;
-  box-shadow: 0 0 0 2px #fff, 0 0 0 5px #26a769;
+  outline: 2px
+dotted
+transparent;
+  box-shadow: 0
+0
+0
+2px
+#fff
+,
+0
+0
+0
+5px
+#26a769;
 }

This is a bit concerning. We had a similar issue with the font short hand when were generating initial patch for Claro. I can't recall if there were any actual regressions that splitting the declaration value into multiple lines would cause. Regardless of that, we managed to solve it by not using the shorthand. I'm wondering if there would be a way to keep both, these values in a variable and generate this correctly.

  • lauriii committed 7e04d24 on 9.1.x
    Issue #3078524 by huzooka, kostyashupenko, thalles, katrienc, pranav45,...

  • lauriii committed d1dbb94 on 9.0.x
    Issue #3078524 by huzooka, kostyashupenko, thalles, katrienc, pranav45,...

  • lauriii committed 9910e3e on 8.9.x
    Issue #3078524 by huzooka, kostyashupenko, thalles, katrienc, pranav45,...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs followup

While the generated code in #71 isn't pretty, it doesn't seem to cause issues on how the CSS is read by browsers. This patch has been tested pretty extensively with various browsers (see #28 for example). I also validated this with the W3C validator without any problems.

I was able to figure out what the problem was earlier and it's not related. The earlier problem actually had to do with some CSS variables not being replaced correctly in the generated code.

Committed 7e04d24 and pushed to 9.1.x, 9.0.x and 8.9.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.