Restructure accordion example to remove DL, DT and DD elements for issue 815 #838

jongund · 2018-08-16T18:24:28Z

For issue #815, updates the accordion example to remove DL, DT and DD elements. Also changes focus styling.

Preview link

New accordion example page in accordion branch of Jon's repo

Compare to:

LaurenceRLewis · 2018-08-16T20:33:36Z

Just wondering; what is the reason for using role heading aria-level on a div element and not a native hx? Also does this mean the suggestion < section >, for the accordion panel, was dropped? Thanks.

mcking65

@jongund, thank you very much for this PR.

It's going in the desired direction. However, there are some conflicts that need to be resolved and some changes that need to be made. Please see my comments in th HTML file.

mcking65 · 2018-08-20T22:16:50Z

examples/accordion/accordion.html

@@ -49,19 +49,17 @@ <h2 id="ex_label">Example</h2>

          Ex:
            <dl id="accordionGroup" role="presentation" class="Accordion" data-allow-multiple>


This comment needs to be updated to not show ``dl`

mcking65 · 2018-08-20T22:17:34Z

examples/accordion/accordion.html

@@ -49,19 +49,17 @@ <h2 id="ex_label">Example</h2>

          Ex:
            <dl id="accordionGroup" role="presentation" class="Accordion" data-allow-multiple>
-
            <dl id="accordionGroup" role="presentation" class="Accordion" data-allow-toggle>


mcking65 · 2018-08-20T22:19:12Z

examples/accordion/accordion.html

        -->
-        <dl id="accordionGroup" role="presentation" class="Accordion">
-          <dt role="heading" aria-level="3">
+        <div id="accordionGroup" role="presentation" class="Accordion">


We do not need role presentation on a div (yes, sometimes if it has a tooltip you do that to avoid issues on Mac, but don't have that situation here)

mcking65 · 2018-08-20T22:20:40Z

examples/accordion/accordion.html

-        <dl id="accordionGroup" role="presentation" class="Accordion">
-          <dt role="heading" aria-level="3">
+        <div id="accordionGroup" role="presentation" class="Accordion">
+          <div role="heading" aria-level="3">


We want a h3 element here

mcking65 · 2018-08-20T22:22:33Z

examples/accordion/accordion.html

-          </dd>
-          <dt role="heading" aria-level="3">
+          </div>
+          <div role="heading" aria-level="3">


We want a h3 element here

mcking65 · 2018-08-20T22:23:18Z

examples/accordion/accordion.html

@@ -128,15 +126,15 @@ <h2 id="ex_label">Example</h2>
                </p>
              </fieldset>
            </div>
-          </dd>
-          <dt role="heading" aria-level="3">


We want a h3 element here

mcking65 · 2018-08-20T22:24:57Z

examples/accordion/accordion.html

@@ -248,7 +245,7 @@ <h2 id="rps_label">Role, Property, State, and Tabindex  Attributes</h2>
        <tr>
          <th scope="row"><code>presentation</code></th>
          <td></td>
-          <td><code>dl</code></td>
+          <td><code>div</code></td>
          <td>
            <ul>
              <li>Indicates that the <code>dl</code> element is being used to control presentation and does not represent a definition list.</li>


We can delete this row of the table

mcking65 · 2018-08-20T22:26:11Z

examples/accordion/accordion.html

@@ -259,13 +256,13 @@ <h2 id="rps_label">Role, Property, State, and Tabindex  Attributes</h2>
        <tr>
          <th scope="row"><code>heading</code></th>
          <td></td>
-          <td><code>dt</code></td>
+          <td><code>div</code></td>
          <td>Identifies the element as a heading that serves as an accordion header.</td>


We can delete this row of table; no ARIA required to make the accordion header into a heading.

mcking65 · 2018-08-20T22:27:02Z

examples/accordion/accordion.html

          <td>Identifies the element as a heading that serves as an accordion header.</td>
        </tr>
        <tr>
          <td></td>
          <th scope="row"><code>aria-level=<q>3</q></code></th>
-          <td><code>dt</code></td>
+          <td><code>div</code></td>
          <td>
            <ul>
              <li>Specifies heading level for the accordion header.</li>


We can delete this row of table

jongund · 2018-09-10T23:27:29Z

Matt,

I update the accordion example to fix the heading issue and added improved focus styling.
Let me know if there are any problems with the pull request or the code changes.

mcking65

@jongund, this is much closer.

mcking65 · 2018-09-14T21:23:09Z

examples/accordion/accordion.html

@@ -38,30 +38,17 @@ <h2 id="ex_label">Example</h2>
    <div role="separator" id="ex_start_sep" aria-labelledby="ex_start_sep ex_label" aria-label="Start of"></div>
    <div id="coding-arena">
      <div class="demo-block">
-        <!-- Accordion Configuration Options


Was this accidental removal? We do not want to remove these config options or their documentation.

mcking65 · 2018-09-14T21:25:20Z

examples/accordion/accordion.html

-            >
-              <span class="Accordion-title">Personal Information</span><span class="Accordion-icon"></span>
+              aria-controls="sect1" id="accordion1id">
+              <div class="Accordion-title">


What is reason for changing from span to div? The validator is failing this; says div not allowed in this context as child of button.

mcking65 · 2018-09-14T21:30:46Z

@sh0ji, could you please look at the css/js changes for focus styling?

jongund · 2018-09-18T01:05:04Z

I add the source code documentation back in.
I was unable to remove the outline style from standard buttons in Firefox, it seems to be a bug in Firefox, since the DOM inspector said the computed style was 0 width. Can't seem to override the default color of the outline for a button either.
Updated the accessibility feature section.

jongund · 2018-09-18T13:56:09Z

Matt,

I have created a new branch that adds the accordion behavior options, so when your ready I can add them to the example:
https://rawgit.com/jongund/aria-practices/accordion-with-options/examples/accordion/accordion.html

Jon

sh0ji

I appreciate the visual updates here, especially the move toward giving a visual indicator that the parent container gives context for interior focus. Discoverability of roving/activedescendant focus management is often awful for sighted users, and this helps a lot.

My feedback is mostly just nitpicky stuff that doesn't have a huge impact on accessibility (visual or otherwise).

examples/accordion/css/accordion.css

sh0ji · 2018-09-19T19:26:05Z

examples/accordion/css/accordion.css

+    padding: 0;
+}
+
+.Accordion *:first-child .Accordion-trigger {


The child elements are still colliding with the .Accordion border in the corners, despite this:

Removing the .Accordion-trigger child selector should fix it:

.Accordion :first-child { border-radius: 5px 5px 0 0; }

sh0ji · 2018-09-19T19:38:27Z

examples/accordion/css/accordion.css

+}
+
+.Accordion-trigger:focus .Accordion-title {
+    border-color: hsl(216, 94%, 73%);


The .Accordion-trigger already has a visual change on focus, so I'm not entirely clear what purpose this serves. It also gives the visual impression that only the .Accordion-title is focused, or that the .Accordion-title is the clickable area, neither of which is true.

sh0ji · 2018-09-19T19:46:11Z

Another note that I forgot: I'd like to be able to click into the widget with my mouse, and then navigate around it with my keyboard. This is currently not possible because the click listener is preventing focus on the buttons.

mcking65

@jongund, Please address the 4 changes needed to remove role presentation and fix the HTML validation issues. That's the minimum we need before @spectranaut works on the tests. Ideally, if you have time, address the comments from @sh0ji.

mcking65 · 2018-09-20T16:50:41Z

examples/accordion/accordion.html

-            <dl id="accordionGroup" role="presentation" class="Accordion" data-allow-multiple>
-
-            <dl id="accordionGroup" role="presentation" class="Accordion" data-allow-toggle>
+            <div id="accordionGroup" role="presentation" class="Accordion" data-allow-multiple>


Please remove role presentation

mcking65 · 2018-09-20T16:53:56Z

examples/accordion/accordion.html


+            <div id="accordionGroup" role="presentation" class="Accordion" data-allow-toggle>


Please remove role presentation

mcking65 · 2018-09-20T16:54:44Z

examples/accordion/accordion.html

        -->
-        <dl id="accordionGroup" role="presentation" class="Accordion">
-          <dt role="heading" aria-level="3">
+        <div id="accordionGroup" role="presentation" class="Accordion">


Please remove role presentation

mcking65 · 2018-09-20T16:55:31Z

examples/accordion/accordion.html

@@ -248,7 +268,7 @@ <h2 id="rps_label">Role, Property, State, and Tabindex  Attributes</h2>
        <tr>


Please remove this row

…x focus issue

jongund · 2018-09-20T22:02:06Z

I have updated the example to fix the outstanding issues, please review to check my changes

carmacleod · 2018-09-20T22:45:43Z

examples/accordion/accordion.html

-            </ul>
-          </td>
+              <li>The accordion design pattern is defined by a button is contained within an element with <code>role=heading</code>.</li>
+              <li>The default role for the <code>h3</code> element is <code>heading</code>.</li>


This sentence: "The accordion design pattern is defined by a button is contained within an element with role=heading."

either needs "that" added after "button", i.e. "...a button that is contained within..."

OR the "is" after "button" can be deleted, i.e. "...a button contained within..."

Resolves merge conflicts with base branch for pr #838.

mcking65 · 2018-09-21T04:19:06Z

@jongund, I just pushed commit 45ad1e0 to your fork to:

Merge base into compare branch and resolve conflicts in accordion.html.
Fix issue raised by @carmacleod.
Specify what needs to be tested for the accordion heading element.

nschonni · 2018-09-21T04:24:04Z

examples/accordion/accordion.html

@@ -299,7 +301,11 @@ <h2 id="rps_label">Role, Property, State, and Tabindex  Attributes</h2>
          </td>
        </tr>
        <tr data-test-id="region-role">
+<<<<<<< HEAD


Merge conflict here

Yap, noticed soon as the test build completed; fixed in next commit along with editorial corrections. Thanks.

1. Remove role presentation from example divs in the comments. 2. Revise wording of accessibility features section. 3. Fix merge conflict overlooked in previous commit.

mcking65 · 2018-09-21T05:20:14Z

@sh0ji, are the issues you raised resolved as well?

1. Add missing `ul` closing tags 2. Revise landmark region documentation in roles, states, and properties.

sh0ji · 2018-09-21T07:04:34Z

@mcking65, I'm still unsure what the inner focus-ring's role is since it gives the impression that the clickable area is smaller than it actually is, but I wouldn't consider that a blocking issue. Everything else appears to be resolved.

@Decrepidos, the ::-moz-focus-inner was adding a dotted box around the .Accordion-trigger's text content, which already has a custom focus border around the .Accordion-title. It was not conveying new visual information, and was adding to the busyness of the design.

LaurenceRLewis · 2018-09-21T07:52:52Z

@sh0ji Thank you

jongund and others added 8 commits March 18, 2017 07:07

update button example

9e304a1

fixed references to external file

4e59e79

added auto rebase

76ff0ed

Merge branch 'master' of https://github.com/jongund/aria-practices

75a87f2

removed gitcongif file

0df77d2

updated the html

84dacfe

unfinished

9cf9c86

update the docs and div element exchange

ccc49d8

jongund changed the title ~~Accordion example update to remove DL, DT and DD elements~~ Accordion example update to remove DL, DT and DD elements (Issue #830) Aug 16, 2018

jongund changed the title ~~Accordion example update to remove DL, DT and DD elements (Issue #830)~~ Issue 830: Accordion example update to remove DL, DT and DD elements Aug 16, 2018

mcking65 requested changes Aug 20, 2018

View reviewed changes

mcking65 mentioned this pull request Aug 20, 2018

Accordion Example: Add animation on expand/collapse #843

Closed

jongund added 3 commits September 10, 2018 18:16

made edits to accordion and improved focus styling

b932417

made edits to include test refrence

3c9413f

made edits to include test refrence

996e12d

Remove extra quote

2af9917

mcking65 requested changes Sep 14, 2018

View reviewed changes

mcking65 requested a review from sh0ji September 14, 2018 21:30

mcking65 changed the title ~~Issue 830: Accordion example update to remove DL, DT and DD elements~~ Restructure accordion example to remove DL, DT and DD elements for issue 815 Sep 14, 2018

Updated documentation

3f19144

sh0ji reviewed Sep 19, 2018

View reviewed changes

mcking65 changed the base branch from master to issue815-accordion-restructure September 20, 2018 16:35

mcking65 requested changes Sep 20, 2018

View reviewed changes

removed divs from buttons, removed role=presentation and fixed firefo…

bd695b1

…x focus issue

carmacleod reviewed Sep 20, 2018

View reviewed changes

Merge branch 'issue815-accordion-restructure' into accordion

45ad1e0

Resolves merge conflicts with base branch for pr #838.

nschonni reviewed Sep 21, 2018

View reviewed changes

Accordion: editorial revisions

6a83331

1. Remove role presentation from example divs in the comments. 2. Revise wording of accessibility features section. 3. Fix merge conflict overlooked in previous commit.

mcking65 approved these changes Sep 21, 2018

View reviewed changes

mcking65 added 2 commits September 20, 2018 23:35

Accordion example: fix validation and editorial revisions

e542a5c

1. Add missing `ul` closing tags 2. Revise landmark region documentation in roles, states, and properties.

Fix quotes inside code elements

bdbeb71

mcking65 merged commit 2985b97 into w3c:issue815-accordion-restructure Sep 21, 2018

		@@ -49,19 +49,17 @@ <h2 id="ex_label">Example</h2>

		Ex:
		<dl id="accordionGroup" role="presentation" class="Accordion" data-allow-multiple>


		<div id="accordionGroup" role="presentation" class="Accordion" data-allow-toggle>

		@@ -248,7 +268,7 @@ <h2 id="rps_label">Role, Property, State, and Tabindex Attributes</h2>
		<tr>

Restructure accordion example to remove DL, DT and DD elements for issue 815 #838

Restructure accordion example to remove DL, DT and DD elements for issue 815 #838

Conversation

jongund commented Aug 16, 2018 • edited by mcking65 Loading

Preview link

LaurenceRLewis commented Aug 16, 2018 • edited Loading

mcking65 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongund commented Sep 10, 2018

mcking65 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcking65 commented Sep 14, 2018

jongund commented Sep 18, 2018

jongund commented Sep 18, 2018

sh0ji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sh0ji commented Sep 19, 2018

mcking65 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongund commented Sep 20, 2018

Choose a reason for hiding this comment

mcking65 commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcking65 commented Sep 21, 2018

sh0ji commented Sep 21, 2018

LaurenceRLewis commented Sep 21, 2018

jongund commented Aug 16, 2018 •

edited by mcking65

Loading

LaurenceRLewis commented Aug 16, 2018 •

edited

Loading