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.

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

Fix issue 2293: click on read notification focus bug #2299

Merged
merged 1 commit into from
Oct 20, 2017
Merged

Fix issue 2293: click on read notification focus bug #2299

merged 1 commit into from
Oct 20, 2017

Conversation

benjaminapetersen
Copy link
Contributor

Fix issue #2293

You can still click the notification to clear it, but now there is a sr-only button for tabbing. The gif below shows the tabbing experience. There is clearly a hidden element in the flow.

2017-10-18 14 08 43

@jwforres @spadgett

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 18, 2017
@benjaminapetersen benjaminapetersen force-pushed the issue/2293/notification-click-handler branch from 81324af to 37ea856 Compare October 18, 2017 18:22
@jwforres
Copy link
Member

I'm ok with this, I doubt a typical user would tab through that part of the UI

jwforres
jwforres reviewed Oct 18, 2017
View reviewed changes
app/views/directives/notifications/notification-body.html Outdated
@@ -10,6 +9,11 @@
<span class="sr-only">Clear notification</span>
<span aria-hidden="true" class="pficon pficon-close"></span>
</button>
<button
Copy link
Member

Choose a reason for hiding this comment

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

I would only expect this button to exist if the notification is unread

Copy link
Member

Choose a reason for hiding this comment

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

Maybe move it down after the notification message, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@jwforres
Copy link
Member

Are we doing anything from an accessibility perspective to notify the user that the event is unread? Bolding the text but not providing sr-only text somewhere isn't accessible.

spadgett
spadgett reviewed Oct 18, 2017
View reviewed changes
dist/scripts/templates.js Outdated
@@ -7785,11 +7785,14 @@ angular.module('openshiftConsoleTemplates', []).run(['$templateCache', function(


$templateCache.put('views/directives/notifications/notification-body.html',
"<div class=\"drawer-pf-notification-inner\" tabindex=\"0\" ng-click=\"$ctrl.customScope.markRead(notification)\">\n" +
"<div class=\"drawer-pf-notification-inner\" tabindex=\"0\" ng-click=\"notification.unread && $ctrl.customScope.markRead(notification)\">\n" +
Copy link
Member

Choose a reason for hiding this comment

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

dist looks wrong

@benjaminapetersen benjaminapetersen changed the title Fix issue 2293: click on read notification focus bug [WIP] Fix issue 2293: click on read notification focus bug Oct 19, 2017
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 19, 2017
@benjaminapetersen benjaminapetersen force-pushed the issue/2293/notification-click-handler branch from 37ea856 to 73000d2 Compare October 19, 2017 13:30
@benjaminapetersen
Copy link
Contributor Author

benjaminapetersen commented Oct 19, 2017 •
edited
Loading

@jwforres I can do something like this to also indicate to a screen reader that the message is unread:

<a
       ng-if="notification.unread"
       href=""
       class="sr-only sr-only-focusable"
       ng-click="$ctrl.customScope.markRead(notification)">
       <span class="sr-only">Message Unread.</span><!-- this never becomes visible -->
       <span>Mark Read?</span><!-- this does become visible -->
</a>

However, this indicates "Read" by omission. Is that acceptable?

We support a list of links following the message (though in practice I've not seen any with more than one link), so adding the "mark read" message here seems to make sense to me:

2017-10-19 09 26 01

Though @spadgett you mentioned margins. Think this could be better?

Lastly, strong feelings about Mark Read vs Mark Read? I am ignorant about screen readers & inflection.

@spadgett
Copy link
Member

Your screenshot looks good to me, although I'd leave off the ?

@benjaminapetersen benjaminapetersen force-pushed the issue/2293/notification-click-handler branch 2 times, most recently from c82bf7f to ae57692 Compare October 19, 2017 13:39
@jwforres
Copy link
Member

jwforres commented Oct 19, 2017 via email

spadgett
spadgett reviewed Oct 19, 2017
View reviewed changes
app/views/directives/notifications/notification-body.html Outdated
href=""
class="sr-only sr-only-focusable"
ng-click="$ctrl.customScope.markRead(notification)">
<span class="sr-only">Message Unread. </span>
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd pull this span outside of the a since it's not part of the action.

@benjaminapetersen
Copy link
Contributor Author

Dropping the ?, also adjusting the pointer icon, will be good to remove [WIP] shortly.

@benjaminapetersen benjaminapetersen force-pushed the issue/2293/notification-click-handler branch from ae57692 to 4286cae Compare October 19, 2017 14:23
@benjaminapetersen benjaminapetersen changed the title [WIP] Fix issue 2293: click on read notification focus bug Fix issue 2293: click on read notification focus bug Oct 19, 2017
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 19, 2017
@spadgett
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2017
@benjaminapetersen benjaminapetersen force-pushed the issue/2293/notification-click-handler branch from 4286cae to 33758b1 Compare October 20, 2017 20:35
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2017
@benjaminapetersen
Copy link
Contributor Author

rebased

/test

@jwforres
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2017
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 90017bf into openshift:master Oct 20, 2017
@benjaminapetersen benjaminapetersen deleted the issue/2293/notification-click-handler branch October 25, 2017 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett left review comments

@jwforres jwforres jwforres left review comments

Assignees

@spadgett spadgett

@jwforres jwforres

Labels
lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@benjaminapetersen @jwforres @spadgett @openshift-merge-robot @openshift-ci-robot