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

fix: link-name to be deciphered from children #1134

Closed
wants to merge 1 commit into from
Closed

Conversation

jeeyyy
Copy link
Contributor

@jeeyyy jeeyyy commented Sep 12, 2018

This PR fixes the rule link-name to take into consideration, nested children text content for deciphering accessibleTextVirtual.

This allows this rule to pass in scenarios like:

<a href="link"><span role="presentation">Link text</span></a>

<a id="pass11" href="link"><div>Some content</div><span role="presentation"></span></a>

Closes issue:

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: << Name here >>

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Bug day? I don't think this is the right fix. There seems to be a bug in our implementation of this spec: https://www.w3.org/TR/accname-1.1/#mapping_additional_nd_name

We need to figure out where our implementation is flawed, rather than to just hack in a solution for this one particular case. There is another case open around figure if I recall right that has the same issue.

@jeeyyy
Copy link
Contributor Author

jeeyyy commented Sep 13, 2018

@WilcoFiers

Cool. There seems to be a lot of context on some of these bugs, that is not detailed in the issue tickets. I came up with this fix, after stepping through the current code base.

Can you link me to the issue for figure? In the interim I will go through the spec.

@WilcoFiers
Copy link
Contributor

This one: #835. I've been thinking of rewriting this one ever since accname 1.1 hit candidate rec. I don't like the existing implementation. It's outdated, clunky and difficult to understand. Here's a draft of what I've been thinking of doing:

text.computationSteps = Object.freeze([
  getAriaLabelledbyText, // Step 2B.1
  getAriaDescribedbyText, // Step 2B.2
  getAriaLabelText, // Step 2C
  getNativeTextAlternative, // Step 2D
  getControlInLabelText, // Step 2E
  getNameFromContent, // Step 2F
  getTextNodeContent, // Step 2G
  getDescendingContentText, // Step 2H
  getTooltipText // Step 2I
])

text.accessibleTextVirtual = function accessibleTextVirtual (node, context = {}) { 
  // Step 2 A, check visibility
  if (!aria.isHidden(node, context)) {
    return ''
  }

  // Return the value of the first step that returns with text
  return text.computationSteps.reduce((text, step) => {
    if (test !== null) {
      return text
    }
    return text.sanitize(step(node, context))
  }, null)
}

/// ------- These can go into their own files:

function getAriaLabelledbyText () {}

function getAriaDescribedbyText () {}

function getAriaLabelText ({ actualNode }) {
  return actualNode.getAttribute('aria-label')
}

function getNativeTextAlternative () {}

function getControlInLabelText () {}

function getNameFromContent () {}

function getTextNodeContent ({ actualNode }) {
  if (actualNode.nodeType !== 3) {
    return null;
  }
  return actualNode.textContent;
}

function getDescendingContentText () {}

function getTooltipText ({ actualNode }) {
  return actualNode.getAttribute('title')
}

@WilcoFiers
Copy link
Contributor

The bug seems to be a thing that was incorrect in an earlier version of the algorithm. The steps don't match anymore. Didn't dive into it too deeply, but it seems Step 2H was added to resolve the bug that we're seeing here. That step clearly wasn't part of the spec when we implemented it.

@WilcoFiers
Copy link
Contributor

Some more thoughts. Most of these methods in my draft rewrite up there already exist in one for or another in checks. With this approach we deduplicate much of what we're doing in acc name computation.

@jeeyyy
Copy link
Contributor Author

jeeyyy commented Sep 17, 2018

Decided to rewrite accessible-name rule. So closing PR

@jeeyyy jeeyyy closed this Sep 17, 2018
@jeeyyy jeeyyy deleted the fix-link-name branch November 6, 2018 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants