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

brailleroledescription update #1097

Merged
merged 15 commits into from Mar 4, 2020
Merged

Conversation

pkra
Copy link
Member

@pkra pkra commented Oct 15, 2019

Continues #924

Addressing review comments, part 1


Preview | Diff

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking very good. Have some editorial suggestions.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@cookiecrook
Copy link
Contributor

LGTM including Matt's editorial.

@pkra
Copy link
Member Author

pkra commented Oct 21, 2019

Thanks so much @mcking65 and @cookiecrook! I'll merge those changes in before the next telco and also adopt @joanmarie's phrasing from #924 (comment).

@pkra pkra marked this pull request as ready for review October 23, 2019 13:06
@pkra
Copy link
Member Author

pkra commented Oct 23, 2019

I think this is ready.

As mentioned, I've left the author warning as edited by @mcking65.

There's one more question leftover from #924 (comment) where @cookiecrook had asked:

This line invalidates using img as a short braille description. Was that intentional? Same thing with "toc" if that one makes it from the DPUB module into the core spec.

and I had responded:

I admit I struggle to answer that. The relevant commit is from June and the commit message explicitly mentions this change. I would have said I added it after feedback from the group, but I can't find any minutes around the time that match this idea.

Maybe we can decide that on the next call.

@pkra pkra self-assigned this Oct 23, 2019
@pkra pkra added the Agenda label Oct 23, 2019
@jnurthen jnurthen added this to Needs triage in Braile Support Nov 13, 2019
@pkra pkra moved this from Needs triage to High priority in Braile Support Nov 21, 2019
@pkra
Copy link
Member Author

pkra commented Dec 3, 2019

@jnurthen @joanmarie can we add this to the next call's agenda? There's only one minor question left that I'd like to bring up with the group.

@pkra
Copy link
Member Author

pkra commented Dec 5, 2019

Neil at #924 (comment)

I commented on the use of the phrase "Unicode Braille Patterns" earlier and the distinction between the block name and an individual character -- strings consist of characters not patterns, symbols, or glyphs. I'm happy to see that in one instance that distinction is properly used. However, since the time of my comment, there have been several changes to text and I think improper usage has creeped in again.

Examples where I think changes are warranted so that the sentence is technically correct:

1. The value of aria-brailleroledescription consists of a string of either Unicode with no Unicode Braille Patterns (U+2800..U+28FF) or consists of a string of only Unicode Braille Patterns (U+2800..U+28FF) while not only containing Braille Pattern dots-0 (U+2800).

2. If the value of aria-brailleroledescription consists only of Unicode characters that are not Unicode Braille Patterns, translate the value according to the user's preferred translation table

I suggest these become:

1. The string value of aria-brailleroledescription does not contain any characters in Unicode Braille Patterns (U+2800..U+28FF) or consists of only characters in the range U+2801..U+28FF (Unicode Braille Pattern block excluding dots-0).


* Note: the previous statement mentioned it must contain "either Unicode or...". In HTML, all attributes value strings much contain Unicode values, so that statement is not a restriction. In fact, [not all Unicode values are legal,](https://www.w3.org/TR/2012/WD-html-markup-20120329/syntax.html#syntax-text) so it isn't even correct.


1. If the value of aria-brailleroledescription does not contain Unicode Braille Pattern characters, translate the value according to the user's preferred translation table.


* Note: The above is the first clause. I think the second clause would be better written as "Otherwise, pass the value to the user without translation." I don't think it is helpful to write a condition on the second clause as there is no alternative third clause (i.e., AT should do one of those two things).

@pkra
Copy link
Member Author

pkra commented Dec 5, 2019

@NSoiffer thanks for those. I'll make an update to address this.

The only detail I don't plan to address right now is the part in (1) where your version would forbid the use of Dot-0 entirely. That's only because I didn't see anything similar for the "regular" role description, i.e., it doesn't forbid whitespace entirely.

@NSoiffer
Copy link

NSoiffer commented Dec 5, 2019

Oops, you are correct that I blew it for excluding Dot-0. I was too aggressive in shortening the language. Hopefully you can keep the gist of the wording change that uses the word "character" where appropriate (i.e., when referring to values in a string).

@pkra
Copy link
Member Author

pkra commented Dec 5, 2019

@NSoiffer thanks for confirming.

@pkra
Copy link
Member Author

pkra commented Dec 10, 2019

I've pushed two commits

  • addressing @NSoiffer's editorial comments
  • addd @mcking65's comment from last telco that it should not duplicate role or role-description
  • removed problematic line identified by @cookiecrook

For the last part, my reasoning was:

a) we noticed it didn't match role-description (but I've filed #1131 as promised),
b) we weren't really sure what it should have said (e.g., "a WAI-ARIA role" seems like it should have been "its WAI-ARIA role"),
c) it would add burden on UAs,
d) we now have an authoring SHOULD NOT.

So I think, this is ready.

@jnurthen jnurthen added this to the ARIA 1.3 milestone Dec 19, 2019
@pkra pkra mentioned this pull request Jan 14, 2020
@sinabahram
Copy link

sinabahram commented Jan 15, 2020

I'm excited to get this finalized, but I do have the following questions and observations.

1. For the intro summary, we have "Defines a human-readable, author-localized abbreviated description for the role of an element, which is intended to be converted into Braille."

Should we say presented in Braille? Conversion seems loaded with questions around translation VS not, which are handled later on in the definition, so should we avoid ambiguity here?

2. We have "Authors MUST NOT use aria-brailleroledescription without providing a more specific aria-roledescription."

I understand the point, but can we clarify more specific than what? Right now, it kind of reads like the aria-roledescription must be more specific than aria-brailleroledescription, which is most definitely not the point of that statement.

Maybe we just want to drop the words "more specific" or at least "more".

3. This sentence really raised eyebrows for me. "In general, aria-brailleroledescription should only be used in rare cases when a aria-roledescription is excessively verbose when rendered in Braille."

Yes, I agree that is a majority use case, but should we really restrict all possible uses? I know it says "should" not "must", but still, that seems restrictive. Do we know for sure there are not cases, for example in educational contexts, where a particular Braille role is simply more appropriate independent of its length?

What about in a simple case where I wish to explore the most appropriate UI principles for two different user groups, speech users and Braille users. So, I construct an experiment to measure the efficacy of particular roles expressed in speech and others in Braille, and length is not the only variable I care to explore? For example, I could argue that speech generally should follow semantic prioritization, or putting important stuff up front, which is why most screen readers announce roles first in a virtual cursor context but not in a focus traversal context. But, do we actually have data on this for Braille? For all we know the geometric advantages of edge based detection would imply that a longer label, but with consistent spacing offers efficiencies in certain working contexts.

It just seems a bit too restrictive without offering evidence, but please don't misunderstand me. I know how valuable and prime a principle it is to be efficient on a Braille display, so I think it is a healthy default position, but I just wish to make sure we feel very certain that length should really surpass all else.

4. I humbly submit that the cognitive load to parse this sentence is exponentially higher than anything else in the description. "The value of aria-brailleroledescription does not contain any characters in Unicode Braille Patterns (U+2800..U+28FF) or consists of only characters in Unicode Braille Patterns (U+2800..U+28FF) while not only containing Braille Pattern dots-0 (U+2800)."

It is crystal clear, but I feel it requires thinking like an engineer, technical writer, or attorney. I feel like we can do a little bit better with those three requirements by explicitly enumerating them. In addition to enumerating them, perhaps we can offer an example to illustrate the two possibilities, one with unicode Braille and one without, along with it must not only consist of 2800. Even a four row good/bad table could go a long way?

5. Just a quick grammar thing here. "Instead, aria-brailleroledescription is meant be used only when aria-roledescription cannot provide an adequate braille representation, "

I believe we meant "meant to be"

6. In the code example, the image has an empty alt with aria-label labeling the planet. Is there a reason why this is done instead of the alt being set to Jupiter and reducing the use of aria attributes by 1? Does accessible name calc result in different results in those two cases? If not, then shouldn't the approach that always minimizes aria usage be preferred?

7. The code, when viewed as HTML, exposes escape characters, like so:

<img alt="" src="images/jupiter.jpg"/>

I didn't quote the above since it's code. Notice the backslash character seemingly escaping the equal sign.

8. We have the following sentence. "In the previous example, a braille display may display "Jupiter, pln" in Braille rather than the verbose "Jupiter, planet"."

Wouldn't a Braille display actually show those reversed e.g. "pln Jupiter"?

Thanks for entertaining this long comment.

@pkra
Copy link
Member Author

pkra commented Jan 15, 2020

Thanks, @sinabahram. I don't have a strong opinion on any of these but some editorial comments below.

Regarding point 2., I probably copied the "more specific" from aria-roledescription (where it refers to being more specific than the role). I agree "more specific" can be confusing; "valid" is probably also sufficient (unless that's considered extraneous/implied in a spec context).

Points 5-8, I think, are simply mistakes that I'll be happy to fix.

@pkra
Copy link
Member Author

pkra commented Jan 16, 2020

Note to self: address #1142 (comment) (prohibit on role=generic)

pkra and others added 8 commits January 16, 2020 09:18
* rephrase opening line - w3c#924 (comment)
* Braille/braille capitalization - w3c#924 (comment)
* suffice => overly verbose - w3c#924 (comment)
* two examples show => example shows - w3c#924 (comment)
Opening: add reference to aria-roledescription.

Co-Authored-By: Matt King <a11yThinker@Gmail.com>
Simplified phrasing for better understanding.

Co-Authored-By: Matt King <a11yThinker@Gmail.com>
remove "rare", replace "excessively verbose" with phrasing more appropriate for braille displays.

Co-Authored-By: Matt King <a11yThinker@Gmail.com>
fix "either" position; clarify "string of empty cells"

Co-Authored-By: Matt King <a11yThinker@Gmail.com>
Improve grammar and language in author warning.

Co-Authored-By: Matt King <a11yThinker@Gmail.com>
Move section to be just before the aria-busy section as these sections
are arranged alphabetically.
Addresses comments from NSoiffer
@pkra
Copy link
Member Author

pkra commented Jan 23, 2020

Just to recap today's call:

Re #1097 (comment), I will fix point 2) as well since "more specific" was copied from roledescription.

Since points 1, 3, and 4 have been revised several times to find consensus, I'd suggest to leave them as is. I suspect we'll work out more details down the road.

@sinabahram
Copy link

As promised on today's call, I reviewed my previous feedback. Yes I'm willing to live with 1, 3, and 4 not being addressed, so I'm going to wait for a commit addressing 2, and then hopefully that means 2, 5, 6, 7, and 8 are all addressed. At which point, I'm happy to review/approve this.

Copy link

@sinabahram sinabahram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to approve once final changes per comment thread are in.

@pkra
Copy link
Member Author

pkra commented Jan 30, 2020

@sinabahram thank you for the review! I'll work those in for next week.

@jnurthen jnurthen removed the Agenda label Feb 4, 2020
pkra added a commit to pkra/aria that referenced this pull request Feb 7, 2020
@pkra
Copy link
Member Author

pkra commented Feb 7, 2020

@sinabahram I've just pushed a fix for point 2), removing the "more specific" part as discussed. I hope this addresses everything. As with the braillelabel, I'd like to first merge before addressing the complex sentence about braille patterns.

@sinabahram
Copy link

sinabahram commented Feb 14, 2020 via email

@pkra
Copy link
Member Author

pkra commented Feb 14, 2020

Thanks. @sinabahram commit 39af2b3 adressed those points.

@sinabahram
Copy link

sinabahram commented Feb 17, 2020 via email

@carmacleod
Copy link
Contributor

@sinabahram We use pr-preview on this repo, so the easiest way to see a current preview is to look for a link called "Preview" and a link called "Diff" that are automatically generated (and kept up-to-date) by a friendly bot and inserted at the end of the initial comment. HTH!

@pkra
Copy link
Member Author

pkra commented Feb 18, 2020

@carmacleod thanks for helping out! @sinabahram sorry, I misunderstood your earlier question. Let me know if there's anything else I can do to make review easier.

@sinabahram
Copy link

sinabahram commented Feb 18, 2020 via email

@pkra
Copy link
Member Author

pkra commented Feb 18, 2020

@sinabahram thanks for taking the time to go through it again. I'm afraid I misread an earlier comment of yours.

Originally, your review had pointed out extra backslashes in the code sample, in particular the alt text. I had addressed this by removing the backslashes. Now I see that later on you pointed out empty alt-text which I completely misread, thinking of the backslash issue.

Perhaps I can pretend to have an excuse since the sample is based on your playground repo. In any case, the button has an aria-label so it seems to me the img might have an empty alt. But I don't mind changing it.

Let me know what you'd prefer.

@sinabahram
Copy link

sinabahram commented Feb 18, 2020 via email

@pkra
Copy link
Member Author

pkra commented Feb 18, 2020

@sinabahram I've updated the code sample (and re-applied a commit that was on the wrong branch on my end).

@pkra
Copy link
Member Author

pkra commented Feb 18, 2020

Oh, posting just as you approved. Thanks!

@sinabahram
Copy link

I've approved the changes. We are good to go on this one.

@pkra
Copy link
Member Author

pkra commented Feb 19, 2020

Just to link this here as well: #1205 has been opened to improve the note.

@jnurthen jnurthen merged commit db5a8c5 into w3c:master Mar 4, 2020
Braile Support automation moved this from In Progress to Closed Mar 4, 2020
@pkra pkra deleted the brailleroledescription branch March 4, 2020 08:00
carmacleod pushed a commit that referenced this pull request May 7, 2020
Co-Authored-By: Matt King <a11yThinker@Gmail.com>
@pkra pkra mentioned this pull request Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Braile Support
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

8 participants