-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve focus highlight #2098
Improve focus highlight #2098
Conversation
@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. |
cc @nextcloud/designers |
Can I see a print screen? |
@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? :) |
@jancborchardt |
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. |
@eppfel: What is odd on the buttons? |
@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: |
whats the "nextcloud accent color"? Is that defined somewhere in the CSS? |
@eppfel I like it! |
@LiamHD It is the nextcloud blue by default, but can by set in the theming app. @juliushaertl to the rescue! 😁 |
I have some trouble changing the outline-color on safari. |
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. |
@LiamHD Nice that you're trying to fix it. I just used the 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 |
Better with 9c02917 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with 6663dc4
@@ -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;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed with 6663dc4
#quota:focus, | ||
.pager li a:focus { | ||
color: #333; | ||
border: 2px solid #0082c9; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1px should be enough and more fitting. Also, changing the border-size, changes the size of the element, which creates layout glitches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with 6663dc4
@LiamHD We can address the theming in a next step. |
@LiamHD Right, changes for the theming CSS go in |
@LiamHD is this ready by the way? We have Nextcloud 11 feature freeze this week. :) Please review @nextcloud/accessibility @nextcloud/designers |
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,...
If you need more help with this, let me know. We might have to open on new branch in the nextcloud/server repository. |
I added the "select:focus" style. |
@@ -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; '. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line is obsolete, because it is set to #333
already.
input[type="button"]:focus, | ||
#quota:focus, | ||
.pager li a:focus { | ||
color: #333; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, that I look again why is this needed anyway. Isn't this the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works on my side. Just the minor thing with this |
Ah, and now we have to adjust the tests. 🐝 Totally forgot about that. But the signing worked 🎉 |
@eppfel : I did have a look at the tests you mentioned but did not understand them. What needs to be adapted there? |
@LiamHD Yes, removing 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.
Still happy to help.. |
Okay. Now I have removed the unnecessary color rule. |
Current coverage is 57.01% (diff: 100%)@@ 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
|
Hey, we did it. Glad to help. You could squash and force push again... |
Signed-off-by: Matthias Becker <becker.matthias@gmail.com>
@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. :) |
Any progress here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Hey @LiamHD could you check the issue that @ChristophWurst noticed? Thanks :) |
@ChristophWurst: What would you propose to fix the issue? |
@@ -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 { '. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LiamHD we could exclude selects here
Obsolete due to #3187 |
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).