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 updates made to link example page #228

Closed
7 tasks done
mcking65 opened this issue Dec 14, 2016 · 17 comments
Closed
7 tasks done

Review updates made to link example page #228

mcking65 opened this issue Dec 14, 2016 · 17 comments
Assignees
Labels
editorial Changes to prose that don't alter intended meaning, e.g., phrasing, grammar. May fix inaccuracies. Example Page Related to a page containing an example implementation of a pattern

Comments

@mcking65
Copy link
Contributor

mcking65 commented Dec 14, 2016

The Link Examples page
needs the following updates.

  • Restructure content to use the current example template
  • Change the page title to "Link Examples".
  • Update title tag.
  • Fix misspellings.

Requested Reviews

@mcking65 mcking65 added editorial Changes to prose that don't alter intended meaning, e.g., phrasing, grammar. May fix inaccuracies. needs edits labels Dec 14, 2016
@mcking65 mcking65 added this to the 1.1 PR milestone Dec 14, 2016
@mcking65 mcking65 modified the milestones: Feb 2017 Heartbeat Draft, 1.1 PR Jan 6, 2017
mcking65 pushed a commit that referenced this issue Feb 6, 2017
Made the following updates for issue #228:
* updated link examples to use new example template
* fixed bug in CSS
* updated description
* Editorial revisions to link example page.
@mcking65
Copy link
Contributor Author

mcking65 commented Feb 6, 2017

The pattern says that shift+f10 opens the link context menu. Our examples do not have a context menu to open the link in a new tab, etc. Should we include this feature?

@ZoeBijl
Copy link
Contributor

ZoeBijl commented Feb 6, 2017

Might be worth it to at least implement a keyboard shortcut for “open in new window”. On macOS this would be cmd+enter. Regardless of whether we implement the context menu.

mcking65 added a commit that referenced this issue Feb 7, 2017
Made the following changes Per discussion of issue #228 in Feb 6 authoring practices meeting.

modified aria-practices.html:
In the link pattern section, keyboard subsection, marked the context menu key (shift+f10) as optional.

modified examples/link/link.html:
In the example introduction, added the following sentence to the note encouraging authors to use native HTML links:
"Browsers provide valuable functionality for native HTML links, e.g., open the target in a new window and copy the target URL to the system clipboard."
@mcking65
Copy link
Contributor Author

mcking65 commented Feb 7, 2017

@michiel, I created issue #275 to address the adding of context menu with keyboard shortcuts. I don't think we should do one without the other. I assigned this to the release 2 milestone.

@mcking65
Copy link
Contributor Author

mcking65 commented Feb 7, 2017

@annabbott, @jongund,

Question: does link number 4 disappear in high contrast mode?

@mcking65 mcking65 changed the title Updates needed for link example page Review updates made to link example page Feb 7, 2017
@mcking65 mcking65 added Example Page Related to a page containing an example implementation of a pattern Needs Review labels Feb 7, 2017
@jongund
Copy link
Contributor

jongund commented Feb 7, 2017 via email

@jongund
Copy link
Contributor

jongund commented Feb 8, 2017

For the context menu to open link in a new window or new tab. Is there any other markup that needs to be added to the link like: aria-haspopup, aria-expanded and/or aria-controls.

I am assuming the context menu follows the ARIA menu pattern.

Jon

@shirsha
Copy link

shirsha commented Feb 8, 2017

Example#3 doesn't have focus.

Thanks,
Siri

@shirsha
Copy link

shirsha commented Feb 8, 2017

If a link takes to another webpage(outside the current website) , should they be informed through the screen reader text.

Thanks
Siri

@mcking65
Copy link
Contributor Author

mcking65 commented Feb 9, 2017

@jongund wrote:

For the context menu to open link in a new window or new tab. Is there any other markup that needs to be added to the link like: aria-haspopup, aria-expanded and/or aria-controls.

No.

As with menubutton, aria-controls and aria-expanded are not necessary.

While it would not be incorrect to add aria-haspopup, all links already have context menus so there is no value in doing so.

I am assuming the context menu follows the ARIA menu pattern.

Yes.

@mcking65
Copy link
Contributor Author

mcking65 commented Feb 9, 2017

@jongund wrote:

The link 4 example will disappear it uses a CSS background image technique.

OK, that's what I thought. I wonder if we should flag that example as not recommended and put a note for the reason? Thoughts?

@mcking65
Copy link
Contributor Author

mcking65 commented Feb 9, 2017

@shirsha wrote:

Example#3 doesn't have focus.

You mean, the focus ring disappears when link 3 gets focus? What browser? Others seeing this?

@mcking65
Copy link
Contributor Author

mcking65 commented Feb 9, 2017

@shirsha wrote:

If a link takes to another webpage(outside the current website) , should they be informed through the screen reader text.

Some sites have such a policy, but that is not needed for accessibility. Some sites use an icon to indicate that, and if they do, they should make sure that icon is accessible. One thing I did at IBM was to make it more screen reader friendly by having the accessible text that represented that icon appended to the end of the link text even though the icon preceeded the link. But, again, that is not core to this pattern.

Now, you might be thinking of a WCAG technique that suggests that links should indicate whether they open a new window. While that is not an essential aspect of this pattern, we could have a note that references it from the design pattern. We reference WCAG in a few other places.

@shirsha
Copy link

shirsha commented Feb 9, 2017

@mcking65 wrote:
You mean, the focus ring disappears when link 3 gets focus? What browser? Others seeing this?
When I tabbed to Example #3, I can't see focus around it. This is in Internet Explorer 11.

linkfocus

mcking65 pushed a commit that referenced this issue Feb 14, 2017
Per issue #228, make changes to examples/link/link.html:

1. Remove link 4, which used a background image that disappears in Windows high contrast mode.
2. Edit introduction, states and properties table, and source code section  to reflect removal of link 4.
3. Change CSS to fix disappearance of custom focus ring on link 3.
4. Change CSS to make the custom focus ring more visually distinct.
@annabbott
Copy link

When using only the keyboard, the link for Example 3 gets an odd and unexpected indication of focus:
issue 228_example 3 odd unexpected focus indicator

@ZoeBijl
Copy link
Contributor

ZoeBijl commented Feb 21, 2017

@annabbott I’ll fix that a bit later today.

Update: Fixed!

@annabbott
Copy link

Confirmed fixed

mcking65 added a commit that referenced this issue Mar 11, 2017
The review process is now complete for this example page.

modified examples/link/link.html:
Removed paragraph with link to issue #228, which was the review issue for this example.
@mcking65
Copy link
Contributor Author

All reviewer feedback has been addressed. Closing to complete the review process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Changes to prose that don't alter intended meaning, e.g., phrasing, grammar. May fix inaccuracies. Example Page Related to a page containing an example implementation of a pattern
Development

No branches or pull requests

5 participants