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

Add AXCustomContent for DPUB roles. #15

Merged
merged 1 commit into from
Jan 4, 2023
Merged

Add AXCustomContent for DPUB roles. #15

merged 1 commit into from
Jan 4, 2023

Conversation

cookiecrook
Copy link
Contributor

@cookiecrook cookiecrook commented Nov 1, 2022

<ul>
<li>AXRole: <code>AXLink</code></li>
<li>AXSubrole: <code>&lt;nil&gt;</code></li>
<li>AXRoleDescription: <code>'link'</code></li>
Copy link
Contributor Author

@cookiecrook cookiecrook Nov 1, 2022

Choose a reason for hiding this comment

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

TBD... Should 'doc-backlink' use a new subrole and should it include "back" in the role description. Note: DPUB Spec and AAM docs would need Privacy warnings if clients remapped this, because it would potentially allow AT detection. See w3ctag/design-principles#293 for more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving as-is. Can revisit later if needed.

Choose a reason for hiding this comment

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

James couldn't browsers also check to see if the role is doc-backlink and apply different CSS styling to denote a backlink vs. a regular link? Not sure this would only be accessed by AT only.
I personally would like to see "backlink" in an additional

  • AXCustomContent:{ label: "type", value: "backlink" }
  • or
  • AXRoleDescription: 'backlink'
  • Copy link
    Member

    Choose a reason for hiding this comment

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

    Is there a risk of AT detection here beyond what would exist for any link? Back links aren't only for AT users. ebook readers lack back buttons, so they're widely available to everyone in publications. It's used as a means of getting back to a footnote reference, for example.

    I agree the context of getting the user back to the starting reference is helpful here, so having "back" in the description would be useful.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Is there a risk of AT detection here beyond what would exist for any link?

    Yes. Any activation that does not include a prior focus, or other pointer-related events, etc. would indicate the potential for AT detection. Some of the factors could be mitigated, but without further guidance from the TAG on w3ctag/design-principles#293 I would rather address that separately in a standalone issue.

    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    <li>AXRole: <code>AXGroup</code></li>
    <li>AXSubrole: <code>AXLandmarkRegion</code></li>
    <li>AXRoleDescription: <code>'region'</code></li>
    <li>AXCustomContent:<code>{ label: "type", value: "prolog" }</code></li>
    Copy link
    Contributor Author

    @cookiecrook cookiecrook Nov 1, 2022

    Choose a reason for hiding this comment

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

    [sic] "prolog" could be "prologue" in the en-uk l10n

    Choose a reason for hiding this comment

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

    Maybe an issue with this diff preview but it looks like this is modifying "doc-pagebreak" not "doc-preface" ??
    ie.

    Expose ROLE_LANDMARK and object attribute xml-roles:doc-pagebreak.

    +	<td>AXRole: <code>AXGroup</code><br /> AXSubrole: <code>AXLandmarkRegion</code><br />
    +	<td>
    +	<ul>
    +		<li>AXRole: <code>AXGroup</code></li>
    +		<li>AXSubrole: <code>AXLandmarkRegion</code></li>
    +		<li>AXRoleDescription: <code>'region'</code></li>
    +		<li>AXCustomContent:<code>{ label: "type", value: "preface" }</code></li>
    +	</ul>
    
    

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    I don't understand. You're commenting on the section covering prologue (not pagebreak or preface).

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Found it in a different section. For future reference, you can click on the line number to add a new comment rather than overloading an unrelated conversation.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    You can also click the button in the left of the blue collapse row (labeled "Expand All") to see additional diffs. That shows that I am editing the correct "preface" section, but the previous API seems so erroneously include "pagebreak". Will you file a new issue @clapierre? Good catch.

    Choose a reason for hiding this comment

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

    Filed issue #16

    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    Copy link
    Contributor Author

    @cookiecrook cookiecrook left a comment

    Choose a reason for hiding this comment

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

    Ready to convert from Draft PR to PR.

    <ul>
    <li>AXRole: <code>AXLink</code></li>
    <li>AXSubrole: <code>&lt;nil&gt;</code></li>
    <li>AXRoleDescription: <code>'link'</code></li>
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Leaving as-is. Can revisit later if needed.

    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    index.html Show resolved Hide resolved
    @@ -1026,8 +1208,13 @@ <h3>Role Mapping Table</h3>
    </td>
    <td><p>Expose <code>ROLE_LANDMARK</code> and object attribute
    <code>xml-roles:doc-pagebreak</code>. </p></td>
    Copy link
    Contributor Author

    @cookiecrook cookiecrook Nov 29, 2022

    Choose a reason for hiding this comment

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

    In a comment below, I think @clapierre was referring to this errata.... (pagebreak not preface in a different API, not the AX API changes I am modifying). I would recommend filing a new issue for this since it's unrelated to my change.

    Copy link
    Member

    Choose a reason for hiding this comment

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

    @clapierre can you verify if this is the issue you had and open a new issue to correct if so?

    Otherwise, please give the updated PR a review and approve it if you're good with the changes now.

    Choose a reason for hiding this comment

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

    Yes I filed #16 as a separate issue for the page break/preface issue, this PR looks good and I will approve it :)

    Copy link
    Member

    Choose a reason for hiding this comment

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

    Ah, I didn't even see that! Thanks, I'll merge this now so we can move on with that issue.

    Copy link

    @clapierre clapierre left a comment

    Choose a reason for hiding this comment

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

    Thanks for all your hard work on this James. Much appreciated.

    @mattgarrish
    Copy link
    Member

    Yes, many thanks @cookiecrook for all your help getting this done!

    @mattgarrish mattgarrish merged commit 6a20cfb into main Jan 4, 2023
    @mattgarrish mattgarrish deleted the aria-issue-1643 branch January 4, 2023 15:28
    @aleventhal
    Copy link
    Contributor

    Just to be clear, these type strings are localized strings, correct?

    @mattgarrish
    Copy link
    Member

    these type strings are localized strings, correct?

    That's my understanding, yes, going by some of @cookiecrook's earlier comments such as this one: #15 (comment)

    @aleventhal
    Copy link
    Contributor

    @cookiecrook sorry for the late comment — do you want to make that clear, or would you think it's obvious because of developer docs for AXCustomContent?

    @cookiecrook
    Copy link
    Contributor Author

    @aleventhal @mattgarrish That should be a new issue, perhaps to match the ARIA spec section on localizable/translatable attributes?

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