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
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
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
Remaining tasks
User interface changes
CKEditor field inputs will have the same focus and hover styling that other text inputs have.
Comment | File | Size | Author |
---|---|---|---|
#66 | Screen Recording 2020-04-14 at 13.36.30.gif | 948.82 KB | lauriii |
#64 | interdiff_62-64.txt | 1.62 KB | kostyashupenko |
#64 | 3078524-64.patch | 9.09 KB | kostyashupenko |
#62 | interdiff_61-62.txt | 3.44 KB | kostyashupenko |
#62 | 3078524-62.patch | 9.21 KB | kostyashupenko |
Comments
Comment #2
fhaeberleComment #3
katriencPatch that implements the ring on the WYSIWYG field.
Comment #4
katriencRemove the double empty line
Comment #5
fhaeberleI'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.
Comment #6
huzookaMarginal note: Editor hover on IE and Edge won't work as expected.
Comment #7
katriencComment #8
katannshaw CreditAttribution: katannshaw at Lullabot commented@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
Comment #9
fhaeberleI 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.
Comment #10
ckrinaI 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.
Comment #11
fhaeberleAs the described concerns in #6 only targeting the hover effect (could be a follow-up) and after testing I'm setting this to RTBC.
Comment #12
katriencI 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)
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?
Comment #13
huzookaComment #14
emma.mariaI'm rerolling this patch now at the DrupalCon sprints.
Comment #15
emma.mariaI'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:
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 🤞
Comment #16
emma.mariaComment #17
emma.mariaComment #18
pminfI will test this on Windows with latest FF, Chrome, Edge and IE11.
Comment #19
pminfI'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.
Comment #20
cedric_aI have tested the patch on #15 on Safari (Version 13.0.3) and confirm that it is fixed as shown in the screenshot.
Comment #21
huzookaThe 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):
Comment #22
Maithri Shetty CreditAttribution: Maithri Shetty at Specbee commentedWYSIWYG editor with error, :focus and :hover changed color to red
Comment #23
shimpyI 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.
Comment #24
shimpyComment #25
huzookaAfter applying #22, editors without any errors get red border, exactly like on the first screenshot in #23.
Comment #26
huzookaComment #27
huzookaScreenshots will be attached later.
Comment #28
huzookaScreenshots (and a new patch with a minor, partially related fix) attached.
Comment #29
Vinodhini.E CreditAttribution: Vinodhini.E at UniMity Solutions Pvt Limited commentedI 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.
Comment #30
DyanneNovaI'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.
Comment #31
lauriiiAs 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.
Comment #32
lauriiiI also opened #3102329: Remove transition from CKEditor border-color as a follow-up.
Comment #33
huzooka@lauriii, did you test my patch in #28?
Comment #34
lauriii@huzooka I think so. Tested #28 again and got the same result.
Comment #35
huzookaComment #36
huzookaComment #37
DyanneNova@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.
Comment #38
lauriiiIt 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.
Comment #39
MeenakshiG CreditAttribution: MeenakshiG at OpenSense Labs for DrupalFit commentedComment #40
mradcliffeThank 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.
Comment #41
fhaeberleI tested the patch provided in #39 and it looks pretty good. I did manual browser testing in all major browsers and therefore RTBC.
Comment #42
fhaeberleComment #43
pranav45 CreditAttribution: pranav45 at OSSeed Technologies commentedHave 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
Comment #44
KondratievaS CreditAttribution: KondratievaS at Skilld commentedTested in Chrome, FF, IE11, IE Edge, Safari and result is OK
Comment #46
mradcliffeI 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.
Comment #47
mradcliffeAdded an event tag.
Comment #48
KondratievaS CreditAttribution: KondratievaS at Skilld commentedComment #49
thallesWorks to me!
RTBC +1
Comment #50
thallesI add opacity to cross browser to #43
Comment #51
thallesComment #52
bnjmnmJust to be redundant I tested this on all OSX browsers plus IE11 on Virtualbox and it worked as hoped, just like the earlier comments.
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.--chrome-border-radius can be the value of --base-border-radius. These would also benefit from a comments explaining what is getting calculated here.
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.Comment #53
kostyashupenkoComment #54
kostyashupenkoComment #55
kostyashupenkoI also think it's better to rename these variables:
(from
/css/theme/ckeditor-editor.pcss.css
)to
Since it is a part of only that component, so better stick these variables to its component.
Comment #56
prabha1997 CreditAttribution: prabha1997 at Valuebound for Valuebound commentedComment #57
prabha1997 CreditAttribution: prabha1997 at Valuebound for Valuebound commentedKindly review patch
Comment #58
KondratievaS CreditAttribution: KondratievaS at Skilld commentedTested patch from #57 and result is OK
Still need dev to check latest changes
Comment #59
bnjmnmNice fast responses on this, thanks!
One tiny thing and this may be ready:
This could still use a comment explaining what is being accomplished by
calc()
as opposed to using a fixed value.Comment #60
neelam_wadhwani CreditAttribution: neelam_wadhwani at Valuebound for Valuebound commentedComment #61
neelam_wadhwani CreditAttribution: neelam_wadhwani at Valuebound for Valuebound commentedKindly review patch.
Comment #62
kostyashupenkoFixed comments and lint.
@neelam_wadhwani please run
yarn build:css && yarn lint:css
before patch next time ;)Comment #63
bnjmnmOne tiny thing related to the most recent change!
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. */
Comment #64
kostyashupenkoComment #65
bnjmnmFeedback is all addressed, manual testing has been backed up by screenshots from contributors, and this patch applies to 9.1 as well.
Comment #66
lauriiiOn IE 11 and Edge the CKEditor isn't hovered when the pointer is on the textarea:
Is this something we believe could be fixed?
Comment #67
bnjmnmRe #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.
Comment #68
lauriiiLet's open follow-up for #66.
Comment #69
bnjmnmFollowup created #3127819: Hover styling in IE11 + Edge not working when pointer over textarea
Comment #70
lauriiiComment #71
lauriiiThis 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.
Comment #75
lauriiiWhile 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!