Improve focus highlight #2098

LiamHD · 2016-11-13T13:44:27Z

The currently focused element (e.g. input field) is only hardly visible because the css files disable drawing of the outline. This is really bad for usability especially when using keyboard navigation (with the TAB key).

mention-bot · 2016-11-13T13:44:28Z

@LiamHD, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Henni, @jancborchardt and @MorrisJobke to be potential reviewers.

MorrisJobke · 2016-11-14T09:00:32Z

cc @nextcloud/designers

Espina2 · 2016-11-14T09:18:14Z

Can I see a print screen?

LiamHD · 2016-11-14T09:53:28Z

This is on google chrome on macOS

jancborchardt · 2016-11-14T10:10:01Z

@LiamHD thank you for the improvement! :) The default outline was overridden because in lots of cases it’s visually unpleasant and inconsistent between systems and browsers. For many elements we do have hover/focus styles but inputs apparently not.

It would be good to keep it overridden but replace it with a proper style which looks nice and is consistent. Do you want to have a look at that? :)

LiamHD · 2016-11-14T10:27:39Z

@jancborchardt
Can you please provide examples where it is "visually unpleasant"? And on which browsers is it inconsistent? At least on chrome and safari on mac the defaults look very similar (light blow glow effect).

eppfel · 2016-11-14T11:08:36Z

I agree with @jancborchardt that, instead of using the browser default, we should find a consistent style for this.

@LiamHD The glow color is actually the selection color set by the user in the system settings of Mac OS and therefore could yield some weird results. And Firefox on Mac does not have these outlines/glows. And that's just on Mac...

A quick and easy solution would be using the nextcloud accent color for the border. Buttons look odd, though.

LiamHD · 2016-11-14T11:46:27Z

@eppfel: What is odd on the buttons?

eppfel · 2016-11-14T12:21:11Z

@LiamHD They look like too similar to text inputs. And we don't use this button style...

Maybe changing the font-color helps:

But personally I consider the grey buttons too minimalistic anyway:

But thats something for another issue...

LiamHD · 2016-11-14T15:59:57Z

whats the "nextcloud accent color"? Is that defined somewhere in the CSS?

skjnldsv · 2016-11-14T16:06:44Z

@eppfel I like it!

eppfel · 2016-11-14T17:16:29Z

@LiamHD It is the nextcloud blue by default, but can by set in the theming app. @juliushaertl to the rescue! 😁
...and with buttons we might want to distinguish :hover and :focus states. 🤔

LiamHD · 2016-11-15T08:41:49Z

I have some trouble changing the outline-color on safari.
See https://jsfiddle.net/LiamHD/15m22c1k/
This is working fine in chrome and in firebox, but safari still shows the blue outline color. Any ideas?

LiamHD · 2016-11-15T09:38:00Z

b540259 uses the nextcloud blue as outline-color. How does that theming work from a technical point of view? When I change the color in the theming settings the outline-color is not adapted at the moment.

eppfel · 2016-11-15T11:49:58Z

@LiamHD Nice that you're trying to fix it. I just used the border-color in the mockups. I think outline is problematic.

The Theming App loads a CSS file generated with the custom color, that overwrites prior CSS properties. The CSS is generated in server/apps/theming/lib/Controller/ThemingController.php

LiamHD · 2016-11-15T13:19:49Z

Better with 9c02917 ?
So do I also have to adapt server/apps/theming/lib/Controller/ThemingController.php to also override the border-color?

eppfel

I think we should stick to the inputs and mabye buttons for now. Other opinions @nextcloud/designers ?

Should we choose 2px borders, we need to address the size glitches.

eppfel · 2016-11-15T18:02:37Z

core/css/styles.css

@@ -128,7 +129,7 @@ body {
 	max-width: 50%;
 	cursor: text;
 	background-color: #0082c9;
-	border: 1px solid rgba(255, 255, 255, .5);
+	border: 1px solid #0082c9;


This is searchbox in the header. And would remove the border, because it has the same color as the header.

Understood.

Fixed with 6663dc4

eppfel · 2016-11-15T18:16:22Z

core/css/styles.css

@@ -2,7 +2,8 @@
 This file is licensed under the Affero General Public License version 3 or later.
 See the COPYING-README file. */

-html, body, div, span, object, iframe, h1, h2, h3, h4, h5, h6, p, blockquote, pre, a, abbr, acronym, address, code, del, dfn, em, img, q, dl, dt, dd, ol, ul, li, fieldset, form, label, legend, table, caption, tbody, tfoot, thead, tr, th, td, article, aside, dialog, figure, footer, header, hgroup, nav, section { margin:0; padding:0; border:0; outline:0; font-weight:inherit; font-size:100%; font-family:inherit; vertical-align:baseline; cursor:default; }
+html, body, div, span, object, iframe, h1, h2, h3, h4, h5, h6, p, blockquote, pre, a, abbr, acronym, address, code, del, dfn, em, img, q, dl, dt, dd, ol, ul, li, fieldset, form, label, legend, table, caption, tbody, tfoot, thead, tr, th, td, article, aside, dialog, figure, footer, header, hgroup, nav, section { margin:0; padding:0; border:0; outline: 0; font-weight:inherit; font-size:100%; font-family:inherit; vertical-align:baseline; cursor:default; }
+a:focus { 	color: #333; border: 2px solid #0082c9;}


This is a nice idea, but quite the change. And layout glitches, due to changes in border size.

fixed with 6663dc4

eppfel · 2016-11-15T19:08:36Z

core/css/inputs.css

+#quota:focus,
+.pager li a:focus {
+	color: #333;
+	border: 2px solid #0082c9;


1px should be enough and more fitting. Also, changing the border-size, changes the size of the element, which creates layout glitches.

Fixed with 6663dc4

eppfel · 2016-11-15T19:12:35Z

@LiamHD We can address the theming in a next step.

juliushaertl · 2016-11-16T13:22:13Z

@LiamHD Right, changes for the theming CSS go in apps/theming/lib/Controller/ThemingController.php but make sure those are also addressed in the unit tests at apps/theming/tests/Controller/ThemingControllerTest.php

jancborchardt · 2016-11-17T21:47:55Z

@LiamHD is this ready by the way? We have Nextcloud 11 feature freeze this week. :)

Please review @nextcloud/accessibility @nextcloud/designers

eppfel · 2016-11-18T10:15:07Z

There are a lot of unclean effects right now:

I would really advise to limit this PR to text inputs and maybe buttons. It would be an improvement, without breaking other stuff (to my knowledge). These rules could easily added to theming app.

@LiamHD To finally merge this PR you have to sign your commits and best would be, if you could squash them to one commit.

So,...

finalize changes
squash commits
sign the commit
push force

If you need more help with this, let me know. We might have to open on new branch in the nextcloud/server repository.

LiamHD · 2016-11-21T13:38:00Z

I added the "select:focus" style.
In a second commit I also added support for the theming. I hope I understood the styling-stuff correctly.

eppfel · 2016-11-21T13:57:41Z

apps/theming/lib/Controller/ThemingController.php

@@ -347,6 +347,16 @@ public function getStylesheet() {
 				'opacity: 0.4;' .
 				'color: '.$textColor.';'.
 				"}\n";
+
+			$responseCss .= 'input[type="text"]:focus, input[type="password"]:focus, input[type="search"]:focus, input[type="number"]:focus, input[type="email"]:focus, input[type="tel"]:focus, input[type="url"]:focus, input[type="time"]:focus, input[type="date"]:focus, textarea:focus, select:focus, button:focus, .button:focus, input[type="submit"]:focus, input[type="button"]:focus, #quota:focus, .pager li a:focus { '.
+							'	color: #333; '.


I think this line is obsolete, because it is set to #333 already.

eppfel · 2016-11-21T13:58:14Z

core/css/inputs.css

+input[type="button"]:focus,
+#quota:focus,
+.pager li a:focus {
+	color: #333;


Now, that I look again why is this needed anyway. Isn't this the default?

You mean the color: #333;? Yes that could be. The important part is the following:border: 1px solid #0082c9;
I was not sure if I have to repeat the color: #333;.

There is rule in place already, so yeah, I think it's obsolete:

eppfel · 2016-11-21T14:00:05Z

Works on my side. Just the minor thing with this color: #333 , maybe there is a reason for this, that i don't see.

eppfel · 2016-11-21T15:23:47Z

Ah, and now we have to adjust the tests. 🐝 Totally forgot about that. But the signing worked 🎉
server/apps/theming/tests/Controller/ThemingControllerTest.php

LiamHD · 2016-11-21T17:15:15Z

@eppfel : I did have a look at the tests you mentioned but did not understand them. What needs to be adapted there?
Should I remove the color: #333 ?

eppfel · 2016-11-22T10:02:29Z

@LiamHD Yes, removing color: #333 would save a line of code and a CSS rule to render.

I only changed one nextcloud unit test so far, so no expert either. But to me, it looks like the four functions that fail, check if the generated CSS is as expected. Because we change the CSS, we have to adapt what the test have to expect.
So, I think we just have to add the additional CSS rules to the tests:

testGetStylesheetWithOnlyColor
testGetStylesheetWithOnlyColorInvert
testGetStylesheetWithAllCombined
testGetStylesheetWithAllCombinedInverted

See: https://github.com/nextcloud/server/blob/master/apps/theming/tests/Controller/ThemingControllerTest.php#L437

Still happy to help..

LiamHD · 2016-11-22T11:42:48Z

Okay. Now I have removed the unnecessary color rule.
Also the test are fixed.
@eppfel : Thanks for you help.

codecov-io · 2016-11-22T12:19:04Z

Current coverage is 57.01% (diff: 100%)

Merging #2098 into master will increase coverage by <.01%

@@             master      #2098   diff @@
==========================================
  Files          1192       1192          
  Lines         71649      71655     +6   
  Methods        7293       7293          
  Messages          0          0          
  Branches       1212       1212          
==========================================
+ Hits          40849      40856     +7   
+ Misses        30800      30799     -1   
  Partials          0          0

Diff Coverage	File Path
•••••••••• 100%	apps/theming/lib/Controller/ThemingController.php

Powered by Codecov. Last update df21562...99005eb

eppfel · 2016-11-22T13:10:22Z

Hey, we did it. Glad to help. You could squash and force push again...
LGTM

Signed-off-by: Matthias Becker <becker.matthias@gmail.com>

jancborchardt · 2016-11-22T16:29:36Z

@LiamHD @eppfel since we just had feature freeze and this has the potential for breakage, I’d be more confident if we wait with merging this until we have stable11 branched off. I’ll schedule this for Nextcloud 12 then, where we can get it in with the beginning of the development cycle so there’s plenty of time to test. :)
Is that OK with you?

LiamHD · 2016-12-13T16:00:43Z

Any progress here?

LukasReschke

Master is free for merges again. I didn't really test this intensively but @jancborchardt statement above indicates that this should be good to go in.

Jan, please give this a final review and either decide to merge or not merge. Thanks.

ChristophWurst

Works nicely except for HTML selects. The text of the items in the dropdown is colored according to the themed color. This works with dark ones, but bright ones make it impossible to read.
Tested with FF on Linux

MorrisJobke · 2016-12-22T10:03:30Z

Hey @LiamHD could you check the issue that @ChristophWurst noticed? Thanks :)

LiamHD · 2017-01-05T09:30:47Z

@ChristophWurst: What would you propose to fix the issue?

ChristophWurst · 2017-01-05T10:22:45Z

apps/theming/lib/Controller/ThemingController.php

@@ -347,6 +347,15 @@ public function getStylesheet() {
 				'opacity: 0.4;' .
 				'color: '.$textColor.';'.
 				"}\n";
+
+			$responseCss .= 'input[type="text"]:focus, input[type="password"]:focus, input[type="search"]:focus, input[type="number"]:focus, input[type="email"]:focus, input[type="tel"]:focus, input[type="url"]:focus, input[type="time"]:focus, input[type="date"]:focus, textarea:focus, select:focus, button:focus, .button:focus, input[type="submit"]:focus, input[type="button"]:focus, #quota:focus, .pager li a:focus { '.


@LiamHD we could exclude selects here

eppfel · 2017-01-22T19:29:39Z

Obsolete due to #3187

MorrisJobke added 3. to review Waiting for reviews design Design, UI, UX, etc. labels Nov 14, 2016

eppfel added 1. to develop Accepted and waiting to be taken care of help wanted and removed 3. to review Waiting for reviews labels Nov 14, 2016

LiamHD force-pushed the patch-1 branch from 3718386 to b540259 Compare November 15, 2016 09:33

LiamHD force-pushed the patch-1 branch from b540259 to 9c02917 Compare November 15, 2016 13:15

eppfel previously requested changes Nov 15, 2016

View reviewed changes

jancborchardt modified the milestone: Nextcloud 12.0 Nov 18, 2016

eppfel reviewed Nov 21, 2016

View reviewed changes

eppfel added 3. to review Waiting for reviews enhancement and removed 1. to develop Accepted and waiting to be taken care of help wanted labels Nov 22, 2016

Add focus highlight to radio buttons and checkboxes

99005eb

Signed-off-by: Matthias Becker <becker.matthias@gmail.com>

LiamHD force-pushed the patch-1 branch from f52e581 to 99005eb Compare November 22, 2016 13:30

jancborchardt added this to the Nextcloud 12.0 milestone Nov 22, 2016

LukasReschke approved these changes Dec 16, 2016

View reviewed changes

eppfel requested a review from jancborchardt December 16, 2016 12:49

ChristophWurst requested changes Dec 20, 2016

View reviewed changes

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 22, 2016

ChristophWurst reviewed Jan 5, 2017

View reviewed changes

eppfel mentioned this pull request Jan 20, 2017

New inputs.scss design #3121

Closed

eppfel closed this Jan 22, 2017

Improve focus highlight #2098

Improve focus highlight #2098

Conversation

LiamHD commented Nov 13, 2016

mention-bot commented Nov 13, 2016

MorrisJobke commented Nov 14, 2016

Espina2 commented Nov 14, 2016

LiamHD commented Nov 14, 2016 • edited Loading

jancborchardt commented Nov 14, 2016

LiamHD commented Nov 14, 2016

eppfel commented Nov 14, 2016

LiamHD commented Nov 14, 2016

eppfel commented Nov 14, 2016

LiamHD commented Nov 14, 2016

skjnldsv commented Nov 14, 2016

eppfel commented Nov 14, 2016

LiamHD commented Nov 15, 2016

LiamHD commented Nov 15, 2016

eppfel commented Nov 15, 2016

LiamHD commented Nov 15, 2016

eppfel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eppfel Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eppfel commented Nov 15, 2016

juliushaertl commented Nov 16, 2016

jancborchardt commented Nov 17, 2016

eppfel commented Nov 18, 2016

LiamHD commented Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eppfel commented Nov 21, 2016

eppfel commented Nov 21, 2016

LiamHD commented Nov 21, 2016

eppfel commented Nov 22, 2016

LiamHD commented Nov 22, 2016

codecov-io commented Nov 22, 2016 • edited Loading

Current coverage is 57.01% (diff: 100%)

eppfel commented Nov 22, 2016

jancborchardt commented Nov 22, 2016 • edited Loading

LiamHD commented Dec 13, 2016

LukasReschke left a comment

Choose a reason for hiding this comment

ChristophWurst left a comment

Choose a reason for hiding this comment

MorrisJobke commented Dec 22, 2016

LiamHD commented Jan 5, 2017

Choose a reason for hiding this comment

eppfel commented Jan 22, 2017

LiamHD commented Nov 14, 2016 •

edited

Loading

eppfel left a comment •

edited

Loading

eppfel Nov 15, 2016 •

edited

Loading

LiamHD commented Nov 21, 2016 •

edited

Loading

codecov-io commented Nov 22, 2016 •

edited

Loading

jancborchardt commented Nov 22, 2016 •

edited

Loading