Skip to content
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

Accessibility ♿ #9862

Merged
merged 19 commits into from
Jun 26, 2018
Merged

Accessibility ♿ #9862

merged 19 commits into from
Jun 26, 2018

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Jun 14, 2018

Fixes #1692
Requires #9723

themes

theme-highcontrast
theme-dark

fonts

font-opendyslexic2

@skjnldsv skjnldsv self-assigned this Jun 14, 2018
@skjnldsv skjnldsv force-pushed the accessibility branch 2 times, most recently from ba680ee to 2b00d70 Compare June 14, 2018 09:25
@nextcloud nextcloud deleted a comment from codecov bot Jun 14, 2018
@codecov
Copy link

codecov bot commented Jun 14, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@ab266a7). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #9862   +/-   ##
=========================================
  Coverage          ?   51.97%           
  Complexity        ?    26017           
=========================================
  Files             ?     1660           
  Lines             ?    96167           
  Branches          ?     1290           
=========================================
  Hits              ?    49982           
  Misses            ?    46185           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
apps/accessibility/lib/AccessibilityProvider.php 0% <0%> (ø) 3 <3> (?)
...pps/accessibility/lib/Settings/PersonalSection.php 0% <0%> (ø) 5 <5> (?)
apps/accessibility/appinfo/routes.php 0% <0%> (ø) 0 <0> (?)
apps/accessibility/templates/settings-personal.php 0% <0%> (ø) 0 <0> (?)
apps/accessibility/lib/Settings/Personal.php 0% <0%> (ø) 4 <4> (?)
apps/accessibility/appinfo/app.php 0% <0%> (ø) 0 <0> (?)
.../accessibility/lib/Controller/ConfigController.php 0% <0%> (ø) 8 <8> (?)
apps/accessibility/lib/AppInfo/Application.php 0% <0%> (ø) 4 <4> (?)
...ibility/lib/Controller/AccessibilityController.php 0% <0%> (ø) 9 <9> (?)

@MorrisJobke
Copy link
Member

Analysing /drone/src/github.com/nextcloud/server/apps/accessibility/lib/Controller/AccessibilityController.php
line 95: OC_App - Static method of private class must not be called
App is not compliant

@MorrisJobke
Copy link
Member

Tests look good - only this one is failing:

  Scenario: create folder in a public editable shared folder        # /drone/src/github.com/nextcloud/server/tests/acceptance/features/app-files.feature:64
    Given I act as John                                             # ActorContext::iActAs()
    And I am logged in                                              # LoginPageContext::iAmLoggedIn()
    And I create a new folder named "Editable shared folder"        # FileListContext::iCreateANewFolderNamed()
    And I close the details view                                    # FilesAppContext::iCloseTheDetailsView()
      │ Close current section details view in Files app could not be clicked
      │ Exception message: Offset within element cannot be scrolled into view: (0, 0): http://acceptance-app-files/index.php/apps/files/?dir=/&fileid=32#
      │ Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
      │ System info: host: '49293c4fe7c5', ip: '172.17.0.20', os.name: 'Linux', os.arch: 'amd64', os.version: '4.15.0-22-generic', java.version: '1.8.0_91'
      │ Driver info: driver.version: unknown
      │ Trying again
      │ 
      Element not found with xpath, (((//html//*[starts-with(@id, 'app-content-')  and not(contains(concat(' ', normalize-space(@class), ' '), ' hidden '))])[1]/preceding-sibling::*[position() = 1 and @id = 'app-sidebar'])[1]/descendant-or-self::*[@class and contains(concat(' ', normalize-space(@class), ' '), ' icon-close ')])[1]
      
      Unable to locate element: {"method":"xpath","selector":"(((//html//*[starts-with(@id, 'app-content-')  and not(contains(concat(' ', normalize-space(@class), ' '), ' hidden '))])[1]/preceding-sibling::*[position() = 1 and @id = 'app-sidebar'])[1]/descendant-or-self::*[@class and contains(concat(' ', normalize-space(@class), ' '), ' icon-close ')])[1]"}
      For documentation on this error, please visit: http://seleniumhq.org/exceptions/no_such_element.html
      Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
      System info: host: '49293c4fe7c5', ip: '172.17.0.20', os.name: 'Linux', os.arch: 'amd64', os.version: '4.15.0-22-generic', java.version: '1.8.0_91'
      Driver info: driver.version: unknown (WebDriver\Exception\NoSuchElement)
    And I see that the details view is closed                       # FilesAppContext::iSeeThatTheDetailsViewIsClosed()
    And I share the link for "Editable shared folder"               # FilesAppContext::iShareTheLinkFor()
    And I set the shared link as editable                           # FilesAppContext::iSetTheSharedLinkAsEditable()
    And I write down the shared link                                # FilesAppContext::iWriteDownTheSharedLink()
    When I act as Jane                                              # ActorContext::iActAs()
    And I visit the shared link I wrote down                        # FilesSharingAppContext::iVisitTheSharedLinkIWroteDown()
    And I see that the current page is the shared link I wrote down # FilesSharingAppContext::iSeeThatTheCurrentPageIsTheSharedLinkIWroteDown()
    And I create a new folder named "Subfolder"                     # FileListContext::iCreateANewFolderNamed()
    Then I see that the file list contains a file named "Subfolder" # FileListContext::iSeeThatTheFileListContainsAFileNamed()

Looks like a temporal hiccup, right @danxuliu?

@skjnldsv
Copy link
Member Author

@MorrisJobke Yes, timeout issue by the tests.
Unrelated

@jancborchardt
Copy link
Member

Really great stuff!

Another thing we should probably do when accessibility is enabled, to move the fonts from Light+Semibold (300+600) to Regular+Bold (400+700).

Also, the buttons for the avatar change look very dark? The contrast with the icon is not enough. We should only make the border darker, like with the inputs.

@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 15, 2018

Another thing we should probably do when accessibility is enabled, to move the fonts from Light+Semibold (300+600) to Regular+Bold (400+700).

Really nice idea!
The dyslexia font is set like this already btw :)

Also, the buttons for the avatar change look very dark? The contrast with the icon is not enough. We should only make the border darker, like with the inputs.

Yes, I increased the opacity to the max for the highcontrast theme and removed the slight variations we have on the background. Basically, I only allowed 2 tones: background and slightly darker background and the font foreground.

@jancborchardt
Copy link
Member

Also cc @nextcloud/accessibility @nextcloud/designers for review :)

@skjnldsv
Copy link
Member Author

@jancborchardt not ready for review :)
Soon though ;)

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 21, 2018
@skjnldsv skjnldsv added this to the Nextcloud 14 milestone Jun 21, 2018
@skjnldsv
Copy link
Member Author

Okay now! :)

@jancborchardt
Copy link
Member

So, everyone in @nextcloud/accessibility @nextcloud/designers – any review is welcome! :)

if (!is_null($loggedUser)) {
$userValues = $this->config->getUserKeys($loggedUser->getUID(), $this->appName);
if(count($userValues) > 0) {
\OCP\Util::addStyle($this->appName, 'user-' . md5(implode('-', $userValues)), true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't work when index.php is present in the URL:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(It should be /index.php/apps/accessibility... )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliushaertl hum, tricky!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that will help:

$linkToCSS = \OC::$server->getURLGenerator()->linkToRoute(
'theming.Theming.getStylesheet',
[
'v' => \OC::$server->getConfig()->getAppValue('theming', 'cachebuster', '0'),
]
);
\OCP\Util::addHeader(
'link',
[
'rel' => 'stylesheet',
'href' => $linkToCSS,
]
);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you cannot prepend with addHeader :/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, apparently it works without the prepend now.

@MariusBluem
Copy link
Member

Why do we want to have this enabled by default and shipped with the core-server?

Love the contrast-mode :)

skjnldsv and others added 16 commits June 25, 2018 17:12
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
@skjnldsv
Copy link
Member Author

Okay, so let get this merge then! :)
@nextcloud/designers @rullzer

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it 👍 Lets get it in and ask for some feedback

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 lets do this and iterate later.

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 26, 2018
@skjnldsv skjnldsv merged commit f148e3f into master Jun 26, 2018
@skjnldsv skjnldsv deleted the accessibility branch June 26, 2018 07:04
@jancborchardt jancborchardt added the design Design, UI, UX, etc. label Jun 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish design Design, UI, UX, etc. enhancement feature: accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants