Skip to content

fix(pfNotificationDrawer): Make expand and collapse links accessible #621

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 14, 2017
Merged

fix(pfNotificationDrawer): Make expand and collapse links accessible #621

merged 1 commit into from
Sep 14, 2017

Conversation

amarie401
Copy link
Contributor

@amarie401 amarie401 commented Sep 13, 2017 •
edited
Loading

Description

Added hrefs to the ng-clicks in header of the notification drawer to allow keyboard accessibility to the expand/collapse links. This would close #599

cdcabrera
cdcabrera approved these changes Sep 13, 2017
View reviewed changes
Copy link
Member

@cdcabrera cdcabrera left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Understandable, looks good, functional and looks like we have the same pattern else where in the repo

We should probably take a look at why it causes the issue, seems almost like a hash attr and click but eh...

@jeff-phillips-18
Copy link
Member

This should be a bug fix.

jeff-phillips-18
jeff-phillips-18 requested changes Sep 13, 2017
View reviewed changes
src/notification/notification-drawer.html Outdated
<a ng-if="$ctrl.allowExpand" class="drawer-pf-toggle-expand fa fa-angle-double-left" ng-click="$ctrl.toggleExpandDrawer()"></a>
<a ng-if="$ctrl.onClose" class="drawer-pf-close pficon pficon-close" ng-click="$ctrl.onClose()"></a>
<a href="javascript:void(0);" ng-if="$ctrl.allowExpand" class="drawer-pf-toggle-expand fa fa-angle-double-left" ng-click="$ctrl.toggleExpandDrawer()"></a>
<a href="javascript:void(0);" ng-if="$ctrl.onClose" class="drawer-pf-close pficon pficon-close" ng-click="$ctrl.onClose()"></a>
Copy link
Member

Choose a reason for hiding this comment

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

We need to figure out the unit tests and use href="" or, worst case, use href="#0"

@amarie401
Copy link
Contributor Author

amarie401 commented Sep 13, 2017 •
edited
Loading

When using ng-href="" I get this error:

Component: pfTableView should not show the empty state when items is null FAILED
Expected '' to contain ''.
test/table/tableview/table-view.spec.js:88:67

When using href="" I get this error:

Some of your tests did a full page reload!

@amarie401 amarie401 changed the title chore(pfNotificationDrawer): Make expand and collapse links accessible fix(pfNotificationDrawer): Make expand and collapse links accessible Sep 13, 2017
@dtaylor113
Copy link
Member

Component: pfTableView should not show the empty state when items is null FAILED

What?! NotificationDrawer change shouldn't have any affect on pfTableview.

@amarie401
Copy link
Contributor Author

Maybe there is an error on my part. I will re-check.

@amarie401
Copy link
Contributor Author

@dtaylor113 I commented out that specific unit test and the test passed when using ng-href="" I'm not sure why I am having test failure in pfTableView when I have only touched notification-drawer.html. Is anyone else having this issue?

@dtaylor113
Copy link
Member

@amarie401, I'l clone this PR and test

@amarie401
Copy link
Contributor Author

@dtaylor113 thank you!

@jeff-phillips-18
Copy link
Member

Changing to href="#0" seems to work fine for me.

@amarie401
Copy link
Contributor Author

amarie401 commented Sep 13, 2017 •
edited
Loading

@jeff-phillips-18 @dtaylor113 href="#0" is also working for me now. I was able to fix the issue. My local wasn't up to date with master. Once I did that & ran npm install, it fixed the weird tableView unit test failure issue. Sorry guys!

@dtaylor113
Copy link
Member

I think the issue was in: https://github.com/patternfly/angular-patternfly/blob/master/test/notification/notification-drawer.spec.js#L423
with href="" and clicking on the <a> probably did cause a page reload :-)

@amarie401
Copy link
Contributor Author

So should we stick with href=#0?

@dtaylor113
Copy link
Member

So should we stick with href=#0?

Yes, if that allows accessibility and doesn't break the unit tests

@amarie401
Copy link
Contributor Author

amarie401 commented Sep 13, 2017 •
edited
Loading

@dtaylor113 I agree. Since we want the events to fire and to not use preventDefault then href=#0 would keep the reload from happening that breaks the unit test while also allowing accessibility.

@amarie401
Copy link
Contributor Author

@jeff-phillips-18 thoughts?

@cdcabrera
Copy link
Member

cdcabrera commented Sep 13, 2017 •
edited
Loading

Not necessarily a fan of placing a hash into a link within an Angular app if there's routing involved, but as long as we're handling the click event properly seems ok...

Edit: yeahhh I'm not super psyched about using the hash 0 in there

@amarie401
Copy link
Contributor Author

amarie401 commented Sep 13, 2017 •
edited
Loading

Any work around @cdcabrera ? I would think that eventFire would need to be tweaked if that's what is causing the reload issue.

@cdcabrera
Copy link
Member

I reinstalled/updated through NPM and ran back through the change and testing...

Just running $ npm test then performed the following changes, most passed, couple of fails...

  • placing an empty ng-href="" test passed
  • placing an empty href="" test failed
  • placing just a hash in both test passed
  • placing the hash 0 in both test passed
  • placing the ng-href="javascript:void(0)" test failed
  • placing the href="javascript:void(0)" test passed

From there I decided to start the local server, soooo ran $ npm start and performed the following changes there were 2 failures, the same 2 as before...

  • placing an empty ng-href="" test passed
    • tabbing focus fail in Chrome
  • placing an empty href="" test failed
  • placing just a hash in both test passed
    • tabbing focus success in Chrome
  • placing the hash 0 in both test passed
    • tabbing focus success in Chrome
  • placing the ng-href="javascript:void(0)" test failed
  • placing the href="javascript:void(0)" test passed
    • tabbing focus success in Chrome

And finally, I quit the server and ran the tests again by themselves... they matched again

  • placing an empty ng-href="" test passed
  • placing an empty href="" test failed
  • placing just a hash in both test passed
  • placing the hash 0 in both test passed
  • placing the ng-href="javascript:void(0)" test failed
  • placing the href="javascript:void(0)" test passed

The only thing I could come up with is that possibly when running the tests beside the local serve, if we're using any kind of live reload mechanism that's running parallel to the unit tests and someone hasn't quit say multiple Chrome tabs... all running the project... there may be some funkiness around the testing.

At this point I think it just comes down to consistency for us, or a pattern we want to stick with for the near term. Using the hash 0 works, and as long as we're handling the event properly shouldn't mess with any downstream app routing...

If we're looking for extensions in accessibility it might be a good idea to take a peak at highlighting the rows while a user is tabbing through them (similar to hover state), but I'll leave that as a future UX/Design aspect

So to answer your question @amarie401 for workarounds... eh, lets stick with your PR as is, as long as we all agree and stick to the pattern hopefully for the near term. (which coincidentally also makes search and replacing easy =)

@jeff-phillips-18
Copy link
Member

Agreed @cdcabrera (any thanks for looking into all the options). Recently this came up in Patternfly as well, and the decision was also to go with href="#0" so I think its best to be consistent here and with Patternfly.

@jeff-phillips-18 jeff-phillips-18 merged commit e470eb5 into patternfly:master Sep 14, 2017
@benjaminapetersen
Copy link
Member

href="#0" is interesting, the argument is you should never have <some-elem id="0"> ?

@jeff-phillips-18
Copy link
Member

ID's starting with a number are invalid. so there cannot be an id="0"

@benjaminapetersen
Copy link
Member

Nice. Thats clever.

@amarie401 amarie401 mentioned this pull request Oct 19, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdcabrera cdcabrera cdcabrera approved these changes

@jeff-phillips-18 jeff-phillips-18 jeff-phillips-18 approved these changes

@dtaylor113 dtaylor113 dtaylor113 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accessibility: Notification drawer expand & collapse links are inaccessible via keyboard navigation
5 participants
@amarie401 @jeff-phillips-18 @dtaylor113 @cdcabrera @benjaminapetersen