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

Review Modal Dialog Example #321

Closed
5 tasks done
mcking65 opened this issue Mar 16, 2017 · 32 comments
Closed
5 tasks done

Review Modal Dialog Example #321

mcking65 opened this issue Mar 16, 2017 · 32 comments
Assignees
Labels
Example Page Related to a page containing an example implementation of a pattern

Comments

@mcking65
Copy link
Contributor

mcking65 commented Mar 16, 2017

A modal dialog example
developed for issue #103 is ready for review.

Reviews requested as of March 15, 2016

@terracoda
Copy link

@mcking65, I really like this example, and all the embedded content that explainis the design and design pattern. The example worked very nicely for me with just using the keyboard (in Firefox), and it also worked well in Safari with Voice Over on.

The only visual design issue I noticed (in my quick review) is that while using Voice Over the keyboard focus boarder remains and shows through on the next dialog.

All the examples are very useful!

@annabbott
Copy link

The modal dialog example page uses entirely different formatting (font, font size, table formatting, etc.) than all other example pages.

@annabbott
Copy link

Space needed in first bullet under "Verification Result" dialog (id=dialog2):
Initial focus is set on the first paragraph because the first interactive element is at the bottom, which is out of viewdue to the length of the text. :
Suggested edit:
Initial focus is set on the first paragraph because the first interactive element is at the bottom, which is out of view due to the length of the text.

@annabbott
Copy link

I believe it would be incredibly useful to include text in these examples about preserving the user's point of regard when the modal dialog is closed.
Suggested edit:
When a modal dialog is closed (dismissed), set focus back on the control that launched the dialog thereby preserving the user's point of regard.

mcking65 added a commit that referenced this issue Mar 16, 2017
Per feedback in issue #321 from @annabbott, modified examples/dialog-modal/dialog.html so it matches current example template styles:
1. Added link to w3c style sheet.
2. Add highlight.pack.js script.
3. Add "def" class to keyboard table.
4. Add data and attributes classes to properties table.
mcking65 added a commit that referenced this issue Mar 16, 2017
Per feedback from @annabbott in issue #321, modified:   examples/dialog-modal/dialog.html.
In the "Accessibility Features" section, added bullet points that describe how each dialog maintains the user's point of regard when it closes.
@mcking65
Copy link
Contributor Author

@annabbot, thanks for the great feedback. I think I have addressed all of it.

@tatermelon
Copy link
Contributor

@terracoda, can you please provide a screenshot of the issue you described ("while using Voice Over the keyboard focus boarder remains and shows through on the next dialog")? I haven't noticed any issues that could match that description. Thanks!

@terracoda
Copy link

@tatermelon, ok. On a closer look, there seems to be some serious focus management issues. I do not rely on VoiceOver, but test with it often. Here are a few screenshots.

Step 1: Opening the first dialog with a reduced window, the forms overflow the dialog.
01-safari-inputs-overflow

Step 2: VO focus and Keyboard focus are on Verify button. Everything I hear sounds right.
02-safari-fkbfocus-verify-button-06 50 04

Step 3: Upon pressing the Verify button, the second dialog opens. VO focus seems to stay on the Verify button.
03-safari-vofocus-still-on-verify-and-kbfocus-on-new-dialog-content

Step 4: I am able to use Tab key to move keyboard focus to other button in the second dialog, but VO does not follow and does not read out labels.
04-safari-vofocus-and-kbfocus-mismatch

Step 5: Some mismatch continues
05-safari-add-button-showing-through

Step 6: I use VO hot keys to exit current dialog 2. VO focus moves to dialog 1, but dialog 2 remains open.
06-safari-focus-moves-out-of-current-dialog

The focus behaviour is confusing to me. And obviously, it is not completely contained within the current dialog. This seems to be a tricky thing to do.

Behaviour in Firefox is different, but I find in general Firefox does work well with VO.

Please note, that I was able to read/listen with VO in Safari to the content in the dialogs, but at some point when focus became mismatched, VO became silent.

I hope this helps.
Using Safari 10.0.03 and Mac OS 10.12.3

@mcking65
Copy link
Contributor Author

@terracoda commented

Step 3: Upon pressing the Verify button, the second dialog opens. VO focus seems to stay on the Verify button.

I can verify these problems in Safari but not Chrome. They do not exist on Windows at all.

It seems that the VoiceOver handling of dialogs is not very good in either browser, but it is better with Chrome.

First, to figure out if these behaviors in Safari are script, browser, or VoiceOver bugs, we need to know how it works without VoiceOver. That is something one of you sightees will have to do. We will need to observe what is happening with focus movement, what activeElement returns, and what is in the AX tree.

@tatermelon
Copy link
Contributor

@terracoda I will fix the form overflowing, sorry about that! In terms of the VO focus issues, I suspect that is a VoiceOver bug since the focus/focus ring is getting moved and set properly.

@terracoda
Copy link

@tatermelon, I think it is also a VO bug.

There is one more obvious issue, the user can navigate out of the current dialog without actually closing it. For me this happens in both Safari and Firefox. This seems to be a common issue for many publicly available examples of dialogs.

I'll check back for updates before doing more testing.

@shirsha
Copy link

shirsha commented Mar 20, 2017

It is working fine with JAWS 18 in IE11.
Screen reader is not reading the "Add Delivery Address" heading when model dialog box is opened. It reading the street address label as soon as dialog box is opened. Even when the user tabs to next input field it still reads Street address label.
When user selects enter to activate Close button( focus is on close button), nothing happens.
When user selects the "Link to help" link, screen reader reads the heading and content inside it but again on selecting close button nothing happens. When user tabs now the focus is taken out of the model dialog box.
Same thing happens when user selects "accepting an alternative form" and select close button.
Please see the attached screen shots

Model dialog box-321.docx

@a11ydoer
Copy link
Contributor

"close" button on dialog4(accessed by "accepting an alternative form" button) does not have a focus and can not activate it.

@mcking65
Copy link
Contributor Author

@terracoda commented:

There is one more obvious issue, the user can navigate out of the current dialog without actually closing it.
For me this happens in both Safari and Firefox.
This seems to be a common issue for many publicly available examples of dialogs.

Do you mean you can move keyboard focus outside the dialog? I can not reproduce this. Please provide steps for reproduction.

Or, do you mean with a screen reader reading cursor? If so, this is a screen reader bug and/or feature, depending on which screen reader and how you are using it.

Basically, screen reader users should have the same kind of capabilities as all other users. So, if other users can "see" what is outside the dialog, then screen reader users should at least have some way of doing that. By default, though, screen reader reading should be restricted to the dialog boundaries when focus is inside the dialog.

In general, this pattern is not yet very well supported by screen readers. I think that many screen reader users get by pretty well, but there is a lot, a lot of room for improvement.

@mcking65
Copy link
Contributor Author

@a11ydoer commented:

"close" button on dialog4(accessed by "accepting an alternative form" button) does not have a focus and can not activate it.

Jemma, I cannot reproduce this in Firefox or Chrome. Here are the steps I followed:

  1. Focus and activate "Add Delivery Address" button.
  2. Tab to "Verify Address" button and activate with space.
  3. Tab to "accepting an alternative form" button and press enter or space.

At this point, the "Close" button is focused. I can activate it.

Are you saying that something causes the focus indicator to disappear after step 3?

@a11ydoer
Copy link
Contributor

a11ydoer commented Mar 20, 2017

@mcking65

Now I understand the design that the focus on close button in dialog4 does two things. - it closes dialog4 and move the focus back to the button in dialog2.

mcking65 pushed a commit that referenced this issue Mar 20, 2017
For issue #321, fix issue where underlying page scrolls when dialog scrolls.

1. Move dialog divs to div outside of main container.
2. Only scroll the dialog layer when a dialog is open.
mcking65 added a commit that referenced this issue Mar 20, 2017
For issue #321, because pull #337 moved dialog divs after the main content, the call to scripts for displaying the html source needed to be moved after the main content.

modified examples/dialog-modal/dialog.html: Move call to html source display scripts after the dialog_layer div.
@accdc
Copy link

accdc commented Mar 22, 2017

Hi,
There are issues with the modal dialog. When the first and second modal open, focus is being set to static non-interactive elements, the first being a span tag and the second being a p tag, which should not be the case. No non-interactive static elements should be focusable or in the tab order.

Also, when the Add button is activated a scripting error occurs, as is true when the Close button in the third dialog is activated, discovered in IE11 which prevents the dialogs from acting properly such as closing in the third dialog. E.G The third dialog cannot be closed when the Close button is activated.

mcking65 pushed a commit that referenced this issue Mar 23, 2017
#343)

To fix IE issues raised by @accdc in issue #321, added polyfill for remove and checks for focus.
mcking65 pushed a commit that referenced this issue Mar 23, 2017
For issue #321, change logic so that, when using IE,  focus is set correctly on the last element instead of the first element in the add confirmation dialog.
mcking65 added a commit that referenced this issue Mar 23, 2017
…al focus on static element

For issue #321, to make the example match the design pattern update  made in commit ba0a447, made the following changes.

modified examples/dialog-modal/dialog.html:
1. On first paragraph of verification dialog (dialog2), change tabindex from 0 to -1 to match guidance in pattern.
2.  Change script call on button that opens verification address dialog to specify id of first paragraph.

modified examples/dialog-modal/js/dialog.js:
Fix bug where the window.openDialog method didn't pass through the firstFocus parameter to the dialog constructor.
@mcking65
Copy link
Contributor Author

@accdc, please retest. I think all the IE issues you raised have been addressed.

@accdc
Copy link

accdc commented Mar 23, 2017

Hi,
Issues still remain.

I looked at the JS and the way that the first and last focusable child elements are detected does not work reliably in IE, even if they do in FF. To make this work reliably, it the logic needs to check if the first/last focusable element is 'focusable' by checking the following logic in this order:

  1. Is the element a rendered form field (input, select, or button element )or link (a tag that includes an href attribute), then set focus to it.
  2. Or if the element is a static element does it include tabindex="-1", if yes then set focus to it.
  3. Otherwise if 1 and 2 are false, move to the next element.

This will make it cross browser compatible.

Also, in the third dialog, the following element should not include tabindex="0":

This is just a demonstration. If it were a real application, it would provide a message telling whether the entered address is valid.

Ideally the first link should receive focus, but if the desire is to set focus to this text instead then tabindex="-1" needs to be used instead.

@tatermelon
Copy link
Contributor

@accdc Are you sure you are on the latest version? We just updated the logic a few hours ago to check if something is focusable in that order. We had also updated the focused text to use tabindex="-1" instead of 0.

@mcking65
Copy link
Contributor Author

@accdc, Just before I asked you to retest, we pushed commits 7862b13 and cccb08b that have those changes. I checked travis and the build to gh-pages was successful. It is working for me in IE when I run from gh-pages.

Maybe refresh your cache?

@jnurthen
Copy link
Member

jnurthen commented Mar 27, 2017

trying on iOS10 (iphone) I get the background scrolling with a swipe up/down at a different rate to the foreground. This gives a really annoying parallax effect.

@a11ydoer
Copy link
Contributor

a11ydoer commented Mar 27, 2017

@MichielBijl
Since you are working on css/styling, different dialog sizes/styling of dialogs in the example would help to understand intuitively four different dialog focus management/work flow. Thanks so much for taking care of this, @MichielBijl !

@jnurthen
Copy link
Member

Not seeing the behaviour I stated before but am seeing a different strange behaviour on iphone. Steps to reproduce are as follows:

  • on iPhone (ios 10.3.1) open the modal dialog pattern in safari (note do not have VO running)
  • Click the Add Delivery Address button
  • Click the Verify Address button

Note I cannot scroll all the way to the bottom in order to press the buttons. It bounces back and I cannot activate them.

mcking65 added a commit that referenced this issue Apr 21, 2017
…Small Devices (pull #370)

For issue #321, to address feedback from @jnurthen, changed modal dialog to:

1. Use absolute positioning instead of fixed positioning.
2. Make full screen if screen size less than 640PX.
mcking65 added a commit that referenced this issue Apr 21, 2017
…olling (pull #371)

For issue #321, change the modal dialog example to:

1. Add momentum scrolling.
2. Make dialog layer same height as main window.
mcking65 pushed a commit that referenced this issue Apr 24, 2017
For issue #321, when displayed on screen under 640px, make the dialog full screen with no margins.
@mcking65
Copy link
Contributor Author

Friendly reminder to @jnurthen, @annabbott, @shirsha, please have one last look with latest revisions in place and comment on whether or not you think we are ready to close this review.

@a11ydoer
Copy link
Contributor

a11ydoer commented May 1, 2017

@mcking65
This example template looks different from other example template because of CSS attributes, width 100% and height 100% in newly added <div id="base_window_layer">. This modification was intended to remedy the problem in mobile devices and double scroll issue as far as I remember the discussion at the meeting. I was wondering whether we need to note this example template difference somewhere so that readers do not think this is not an error. Any thought?

@shirsha
Copy link

shirsha commented May 1, 2017

Focus is not visible clearly on the buttons

JAWS18 in IE11:

  • Screen reader is not reading the "Add Delivery Address" heading when model dialog box is opened.

  • It reading the street address label as soon as dialog box is opened. Even when the user tabs to next input field it still reads Street address label.

  • Add role-dialog before the content start so that it put screen reader user in virtual mode

@shirsha
Copy link

shirsha commented May 1, 2017

Tie the additional information after the "Special instructions" label to the input field by using "Aria-describedby " so that screen reader user can hear the additional example text when they tab to the Special instructions input field

@annabbott
Copy link

@mcking65 - please notify me when work is completed on this example and needs further review - Thanks!

@DavidMacDonald
Copy link

DavidMacDonald commented May 30, 2017

Yeah VoiceOver mangles it as it does almost all dialogues. But this one works ok with VoiceOver and focus. https://hanshillen.github.io/jqtest/#goto_dialog

@smartm0use
Copy link

I'm trying to reproduce the same example on Plunker, but it doesn't work: https://plnkr.co/edit/RXJciG

Can you tell me what's wrong with my sample?

mcking65 added a commit that referenced this issue Jun 18, 2017
For issue #321,  fixed the following:
* The link in the description had incorrect ID in the href.
* Link in the footer was missing href and link text.
mcking65 added a commit that referenced this issue Jun 18, 2017
1. Per feedback from @shirsha in issue #321, added aria-describedby to the special instructions input.
 2. Improved text of the special instructions field description.
mcking65 added a commit that referenced this issue Jun 21, 2017
…avior

For issue #321, made the following changes to examples/dialog-modal/dialog.html.

Added the following item to the list of accessibility features:

> To make the content easier to read when displayed on small screens, the dialog fills 100% of the screen.
> Completely covering the background window also hides background movement that occurs on some mobile devices when scrolling content inside the dialog.

Added a row to the keyboard table to document Escape key behavior.
mcking65 added a commit that referenced this issue Jun 21, 2017
…process.

Removed link to issue #321 from examples/dialog-modal/dialog.html.
The internal task force review process is complete.
@mcking65
Copy link
Contributor Author

All feedback has now been addressed. Thank you everyone for all the excellent feedback and testing. Closing!

@mcking65 mcking65 added the Example Page Related to a page containing an example implementation of a pattern label Oct 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Example Page Related to a page containing an example implementation of a pattern
Development

No branches or pull requests