Clean up button example code

[kleinfreund]

In preparation for developing examples for tri-state toggle buttons (see issue #535).

This pull request addresses/changes the following:

• Fixes indentation issues in the markup
• Removing type="text/javascript" (some scripts have it, some don’t. I chose one.)
• Removed / in the meta charset tag (some examples have it, some don’t).

Please note: Due to indentation changes, the GitHub diff doesn’t show some changes. For example,I changed <th> to <td> elements when inside <tbody>. It may have been incorrect to do so.

[mcking65]

@kleinfreund, I apologize again for such slow response.

As a screen reader user, I have a really hard time culling all the whitespace changes to find the material changes. I have tried all the git options for producing diffs that ignore whitespace, but they still produced large diff files that were slow to crawl through. That's my problem, though, not yours. It is something I need to learn to manage. And, I will.

Based on your comments, though, there is one material change that needs to be reversed. We need the <th scope="row"> elements in the bodies of the role, state, and property tables. Those enable the screen reader user to hear the correct role, state, or property change when reading down the right hand columns. Similarly, when reading the keyboard table, the <th> elements on the key command makes the key command a row header so a screen reader user can hear the key command when reading down the description column.

[kleinfreund]

@mcking65 Oh no. I did not consider that. Apologies for that.

Ideally, I would have made two separate commits; one for whitespace, one for other changes. I will provide you with a list of changes that are hidden by whitespace changes. I will refer to lines by updated line numbers.

• line 4: Omitting slash in self-closing tag
• line 10, 16: Omitting type="text/javascript" in script tag.
• line 15: Moving rel attribute before the href attribute to match lines 8 and 9.
• line 76, 80: Replaced th tags with td. I will revert this change as per your review. I will have a look into the side-effect of having the th there overriding the kbd styles.

A single extra newline was added in the following lines: 47, 51, 60, 66, 89, 156, 171, 173, 175, 177 and 184.

A personal note: You’re not to blame for not being able to make out the changes. The diff does not provide any cue for some of the changes made inside of blocks that also change whitespace. Also, there isn’t a visual cue. One would have to read the actual contents in order to notice. In the future, I will try not to mix whitespace changes with other changes to avoid such issues.

Thank you for reviewing this.

[kleinfreund]

I pushed two commits with the following changes:

• Reverted the change from the first commit where some th tags were incorrectly replaced with td.
• Wrapped an occurence of aria-activedescendant in a code tag
• The core CSS was changed so that kbd elements are always styled. Previously, the selector only targetted kbd elements that were not inside a table heading. Please note that I’m concerned about this change as there might be a good reason for the rule being as explicit as it was. From my stand point, it just stopped kbd elements from being styled in places where they should be.

[kleinfreund]

I just rebased my fork on the current state of the upstream repository. That was scary, but it looks like it worked. Do you want me to squash these 3 commits together?

[mcking65]

Thank you for rebasing on latest! That always helps.

When multiple commits come from a fork, I squash when I merge ... no need to do it on your end.

I want to have a sightee or two review the core.css change before merging. The rest all looks good to me!

[mcking65]

@jnurthen, @sh0ji, would you please review the changes to core.css in commit e7c83a2?

[sh0ji]

I don't see any issue with including kbd styling when inside a th, but I am now curious why @MichielBijl explicitly excluded it in e0ab6d6.

Here's the visual change:

• Before:
• After:

[sh0ji]

Ah, I hadn't seen @jnurthen's comment when I approved this. I defer to him on the style requirements.

In that case, I would recommend keeping the current change and just doing a th > kbd override to undo the default styling. It's weird to have default styling inside a :not(...) selector.

[kleinfreund]

First of all, I agree with @sh0ji: A default styling should be applied to kbd, not :not(th) > kbd.

With a default styling for kbd, it seems odd to reset these for th kbd. It would be easy if all: unset could be used, but the support for that is lacking (e.g. not available in Edge or Safari). Instead, we would need to set almost all properties from the default styling to initial. That seems unnecessary. Also, I don’t think keeping the styles inside table headers makes the table too busy.

Anyway, should I just revert back to :not(th) > kbd (avoids unnecessarily overwriting styles) or keep the default styles for kbd and reset them explicitly for th kbd?

[ZoeBijl]

@sh0ji I changed that because the column is called “Key”. There’s no need for a visual cue to indicate these are keys.

---

As for the CSS changes, @sh0ji why is it weird to have default styling applied via :not(…)? We want the styling everywhere expect when they’re the direct descendent of a th-element. If there’s a shorter way to write this that offers the same support I would love to hear it.

@kleinfreund why did you change background to background-color? Since we define new styles for the buttons it’s important to reset all old styles—this is exactly what happens when we use the shorthand background.

We could combine it with the separate background-image to make the code shorter. I prefer it separate, but don’t have strong feelings about it.

[sh0ji]

@MichielBijl You're absolutely right that the current selector is the shortest way of expressing "everywhere except X." I prefer a more verbose approach for two reasons:

1. It takes me longer to understand what :not(th) > kbd means compared to individual rules for kbd, and then another for th > kbd. It didn't take me too long to figure it out, but I can imagine someone less familiar with CSS spending significantly longer trying to understand it.
2. Keeping the specificity for root element styles as low as possible can help decrease the likelihood of future specificity errors.

"Base statement with low specificity -> override of base statement with higher specificity" is almost idiomatic of CSS.

[kleinfreund]

@kleinfreund why did you change background to background-color? Since we define new styles for the buttons it’s important to reset all old styles—this is exactly what happens when we use the shorthand background.

@MichielBijl I wasn’t aware that some user agents set other background properties than the color. Is that the case? I read that code as "set a background color", not "reset all background properties and set a background color". In fact, background: <color> resetting the other properties was the reason for that change. It seemed like an unintentional side-effect.

As for the selector, I agree with @sh0ji.

[ZoeBijl]

The reset is intentional. From the CSS Backgrounds and Borders spec:

Given a valid declaration, for each layer the shorthand first sets the corresponding layer of each of background-image, background-position, background-size, background-repeat, background-origin, background-clip and background-attachment to that property’s initial value, then assigns any explicit values specified for this layer in the declaration. Finally background-color is set to the specified color, if any, else set to its initial value.

There’s a note with an example in the section I link to.

---

As for the kbd thing, it's a core style with supplied with a comment meant to help the reader understand what it does. That piece of CSS is only used on the example pages—it's reader base is already pretty small I reckon. I really don't want to add more CSS to that than necessary. If you feel the current notation is unclear even with the provided comment we should address that comment.

When it comes down to specificity it doesn't really matter in this case—the specificity of the original selector is already really low and can easily be overwritten where necessary. Here's a CodePen example that demonstrates the specificity.

To add to this, there's currently no need to overwrite the base style anywhere on the example page. So, why change it right now? It's not like it's hurting anyone.

[kleinfreund]

The reset is intentional. From the CSS Backgrounds and Borders spec:

I’m aware of the behavior and its intentions with regards to the specification. I was talking about the author perspective: Was it intentional resetting the background styles? Why? Presumably due to some user agents using background properties that are not background-color. I’m not sure there are any, but I can only check a few (Chrome, Firefox and Edge just have a color, nothing else).

---

Regarding specificity, this is really not an issue in this project.

Thank you for reviewing this. I reckon much was discussed around a very small issue/non-issue without any big necessity. I will revert the changes to the CSS file tomorrow.

[kleinfreund]

I reverted the changes to the CSS file by adding a new commit so that the review comments are not invalidated. It’s probably necessary to squash all current commits into one upon merging.

[jnurthen]

Looks ok to me.