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

Disclosure: Updated alt text of image, text content of figure caption #315

Merged
merged 11 commits into from Apr 16, 2017
Merged

Disclosure: Updated alt text of image, text content of figure caption #315

merged 11 commits into from Apr 16, 2017

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Mar 13, 2017

I also fixed the problem with the figcaption tag, it was in the document as caption

Merge branch 'master' into disclosure
Merge branch 'master' into disclosure
Merge remote-tracking branch 'w3c/master' into disclosure
@mcking65
Copy link
Contributor

mcking65 commented Mar 13, 2017

Looking at
spec for figcaption,
the figcaption has to be the first or last element inside the figure.

I fixed this with commit 0ea92b4.

The result can be seen here:
https://rawgit.com/jongund/aria-practices/disclosure/examples/disclosure/disclosure-img-long-description.html

This reveals a pretty nasty JAWS bug: only a portion of the figcaption is rendered. The disclosure is not available to JAWS users. So, this is a pretty useful test case.

It works reasonably well with NVDA in FF, Chrome, and IE11.

@jongund, are you OK with these changes?

@jnurthen, please have a look.

In examples/disclosure/disclosure-img-long-description.html:
1. Structured text in figcaption element with `<p>` elements.
2. Moved disclosure button and disclosure content inside of the `<figcaption>` element.

Fixed JSCS error in examples/disclosure/js/disclosureButton.js.
@jnurthen
Copy link
Member

I think the center align on the caption makes the text really hard to read. Any objection to left aligning it?

@jnurthen
Copy link
Member

Looking at the spec for figcaption it states that it can include flow elements or text. Button is not a flow element so I'm not sure this example is valid. Of course we could put the button inside a div and that would be valid (I think) but I think we need a change.

@mcking65
Copy link
Contributor

@jnurthen, the spec for button
says it is in the flow content category and the
spec for flow content
lists button. So, I think we are OK with a button inside of a figcaption.

@jnurthen
Copy link
Member

jnurthen commented Mar 17, 2017

You are right. Serves me right for looking at an inaccurate document.... http://w3c.github.io/html-reference/common-models.html#common.elem.flow

Happy for you to squash and merge.

@jongund
Copy link
Contributor Author

jongund commented Mar 27, 2017

Matt I have made the changes and it looks like this branch is clean for a merge

@jnurthen jnurthen removed their assignment Apr 10, 2017
@mcking65
Copy link
Contributor

@jongund, yes, this pull does indeed make the needed changes and tests clean. Thanks! I am merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants