Feed Display Example Broken Star Images

[alflennik]

There are broken links in the feed example on the dedicated display page: there should be images representing the number of stars each restaurant has received.

We discussed the issue in the task force and we think the issue is caused by the fact that the links to the images are generated by JavaScript, and we think a good course of action would be to use an inline SVG instead of an external link.

[a11ydoer]

@mcking65 How about having the discussion about updating feed example in the APG meeting? Issue 565 is present about design idea.

[frozenzia]

The "correct" path for the star images, btw, is e.g. https://www.w3.org/WAI/content-assets/wai-aria-practices/patterns/feed/examples/imgs/rating-4.png

Hopefully that's not a "yeah, we already knew that" sort of thing, but thought I'd throw it out there.

[mcking65]

Hi @frozenzia, Thank you for the PR. Per Howard's comment, I am going to close that PR.

However, I am wondering if you would be willing to work on the solution proposed at the top of this issue, which is to change the example code to use inline SVG for the ratings stars?

[frozenzia]

@mcking65, I don't want to make any promises, but I'm definitely interested in giving it a shot. As I just mentioned elsewhere, though, I'm not very experienced with contributing to projects - may need a bit of hand-holding or at least don't be surprised if I end up asking "dumb" questions along the way.

[frozenzia]

@mcking65, hey, sure, sign me up for this issue. If I've understood things properly, the next step here will be that "An editor will confirm there are no conflicting plans and, if needed, provide guidance."

[mcking65]

@frozenzia wrote:

@mcking65, hey, sure, sign me up for this issue. If I've understood things properly, the next step here will be that "An editor will confirm there are no conflicting plans and, if needed, provide guidance."

OK, super! As editor, I can confirm there are no conflicting plans, and this is a good time to move forward with a PR for this issue.

The proposed solution that the Task Force is landed on is to use inline SVG for the rating star images. You might take inspiration from content/patterns/radio/examples/radio-rating.html.

[mcking65]

@frozenzia

The APG Task Force is working to encourage contributions from the larger community. So, please don't hesitate to ask any questions. We will do our best to provide helpful guidance.

[frozenzia]

@mcking65, ok, having looked at this a VERY little bit, I have questions/problems...

1. where exactly to ask about these -- just here and mention you, or preferably leave OFF mention? (does including a mention tend to mean others won't respond?) OR someplace completely different (the mailing list for public-aria-practices?) At this point it certainly feels like these problems are things that would be answered in a "Getting started w/contributing to this project - FAQ". (Which I feel is certainly the intent of the README)

2. Just basically got as far as trying to check that my setup is correct and tried to run npm test -> results were not particularly encouraging...

  6 tests failed
  6 known failures
  29 tests remained pending after a timeout

Having a quick look at one of the failed tests, I see Key.CONTROL, and having just dealt with something similar (on my Mac!), I gave a quick try to see if changing this to Key.COMMAND would have the desired effect -- sure enough, that particular test goes thru after the change. So tests don't seem to be (at least automatically/by default) suited for running on a Mac.

3. Actually one of my first concerns was that according to the README, "Note: this may take a few minutes to run and will open several browser windows during the test that will gain focus.", and on my Mac not only did I first get a notification that my Firefox file was corrupt or something, but after re-installing it, I had to go thru a couple of reboots before it seemed to want to start. AND I have at no point witnessed any browser windows being opened, even after getting Firefox to start.

So that's basically where we are -- stopped dead before reaching the starting line. Any advice / tips / ideas / links to info are greatly appreciated.

TIA!

[frozenzia]

Alright, had a chance to do some more looking, and have an interesting finding.

1st test that failed for me was data-test-id="key-tab". Made absolutely no sense. I opened the appropriate HTML in Firefox and tried to do by hand what the test simulates. Tried the same thing in Chrome and Brave browsers. Chrome and Brave worked just like expected -- w/ the 2nd dialog opened, the 1st paragraph was focused, and tabbing once put the focus on the link at the bottom of the dialog. But on Firefox: initial focus was on the 1st paragraph, as expected, but tabbing once completely skipped the link, and put focus on the first button.

While digging around looking at some about:config settings, I came across something that sent me looking at Mac settings. In System Preferences -> Keyboard -> Shortcuts -> Keyboard [again], there is a shortcut ^F7 when one wants to "Change the way Tab moves focus".

As far as I can tell, there is no visible change or indication anywhere which would tell what sort of "way" is currently being used, BUT, hitting ^F7 worked the magic and now tabbing in Firefox works as the test expects it to.

Naturally, though, when I then bothered to try the test with Safari, it showed the same erroneous focusing that Firefox had previously. ^F7 seems, btw, to only have 2 settings, one which makes Firefox work like we want it to, and the other where it works differently. Neither setting has an affect on Safari, as far as I can tell.

[mcking65]

@frozenzia

Yes, please ask questions of this nature here in GitHub. No need for a mention unless you are responding to a specific person.

I am curious to know if others developing on macOS have these same problems running the tests. @jongund do you see the issues that @frozenzia describes here?

I have to admit that I've had so many challenges with my Windows environment that I don't run the tests locally. I commit with the -n option and push to a remote branch for which I have created a draft PR. Then, I look at the test results from the remote run. Not efficient, but I'm so used to my workaround that I probably don't even realize how much time I'm losing over it. It's less of a hit for me because I rarely touch the js or css.

[frozenzia]

My first thought is to write "determineControlKey", which somehow checks platform and if Mac returns Key.COMMAND, otherwise Key.CONTROL. Then replace all references to Key.CONTROL with a call to determineControlKey. Not imm. figuring out how to get that platform info, however. Looks like window.navigator.platform wld be the (albeit deprecated) way, but can't get window w/Selenium Webdriver?

An easy workaround cld be to flip Control and Command on my Mac in settings before trying to run tests.

But on that topic of the tests, I'm ASSUMING that running tests on the main branch will have all tests passing (except those which are intended to fail), but is that a faulty assumption?

[mcking65]

Thinking about this more, I'm not sure why tests would pass if you swap command for control. The js is not listening for command, which is meta, it is specifically listening for control. We don't have any key assignments in the examples that use the meta key.

We have an open issue related to the fact that some patterns should have different keys assigned for macOS. For example, in some cases, home should be ctrl+up if using a mac. in edit fields, home should be replaced with cmd-left. We haven't written the patterns or scripts that way ... yet.

[css-meeting-bot]

The ARIA Authoring Practices (APG) Task Force just discussed Feed Display Example Broken Star Images.

The full IRC log of that discussion

<jugglinmike> subtopic: Feed Display Example Broken Star Images
<jugglinmike> github: https://github.com//issues/2628
<jugglinmike> Matt_King: Paul Brown (GitHub handle "frozenzia") has volunteered to take this up, but they're having trouble running the tests on their Mac
<jugglinmike> andrea_cardona: I develop on a Mac. I can try to run these tests and comment on this issue about my experience
<jugglinmike> Matt_King: It appears as though there might be some documentation missing for folks who want to run the tests on Mac. The "command" versus "control" thing may be a red herring
<jugglinmike> jugglinmike: I can bring this to Carmen and see that someone from Bocoup takes a look at this alongside andrea_cardona

[frozenzia]

Thinking about this more, I'm not sure why tests would pass if you swap command for control. The js is not listening for command, which is meta, it is specifically listening for control.

@mcking65 , well, I certainly can't claim to 100% understand how the tests are working, but for instance there's a test where we simulate being in a text area and send "ctrl"+a to select all. If I try to pull that off by hand, ctrl+a does nothing as I recall, whereas command+a does exactly what is expected.

[frozenzia]

We have an open issue related to the fact that some patterns should have different keys assigned for macOS. For example, in some cases, home should be ctrl+up if using a mac. in edit fields, home should be replaced with cmd-left. We haven't written the patterns or scripts that way ... yet.

🤔 Wondering about this now I re-read it. My own interpretation of things is that e.g. in this text area box in GitHub, if I send HOME, that should move my cursor to the beginning of a line. Likewise CMD+HOME takes me to the beginning of the very first line. And the main issue is that on my Mac there is no HOME key. I do have one on my larger Mac keyboard, but right now while just using my laptop w/o my external keyboard, the physical key combo I have to hit in order to generate a HOME keypress is Fn+ArrowLeft. But that is interpreted by my browser (and I guess possibly even at a deeper level?) as HOME. But I would not expect to need to write tests that send Fn+ArrowLeft on a Mac vs. HOME everywhere else.

But maybe I'm missing the forest for the trees or something. Totally possible.

[frozenzia]

....and by the way, CMD+ArrowLeft just sends the cursor back one word at a time for me.

[frozenzia]

Alright. Referring to the recent IRC discussion, yes, I think actually @mcking65 is right, and the "command" vs. "control" thing is a red herring. Sheesh. I honestly cannot fathom how I've managed to mess up doing this testing. I would swear that I've gone thru the failing tests, tried to do them by hand, and CTRL did nothing, but CMD did something. Now I've gone back, double-checked things, and sure enough, CTRL seems to be working just fine in most cases.

HOWEVER. In toolbar_toolbar.js, there are a couple of tests where CTRL+A are currently used to select all the text in a textarea. This does nothing on a Mac, and copying similar code from alertdialog_alertdialog.js, I've added an extra line to do the selecting in case the test is run on a Mac. After making this one and only change, running the tests still results in 2 test failures (+ 6 known failures):

combobox_grid-combo › content/patterns/combobox/examples/grid-combo.html [data-test-id="popup-key-home"]: Home from focus on list puts focus on grid and moves cursor

test/tests/combobox_grid-combo.js:880

 879:                                                           
 880:     t.true(                                               
 881:       await confirmCursorIndex(t, ex.comboboxSelector, 0),

Cursor should be at index 0 after one ARROW_HOME key

Value is not `true`:

false

› test/tests/combobox_grid-combo.js:880:11



menu-button_actions-active-descendant › content/patterns/menu-button/examples/menu-button-actions-active-descendant.html [data-test-id="menu-end"]: "end" on role="menu"

Rejected promise returned by test. Reason:

AssertionError {
  actual: 'mi3',
  code: 'ERR_ASSERTION',
  expected: 'mi4',
  generatedMessage: false,
  operator: 'strictEqual',
  message: 'aria-activedescendant should be set to mi4 for item: #ex1 [role="menu"]',
}

› assertAriaSelectedAndActivedescendant (test/util/assertAriaActivedescendant.js:24:10)
› async test/tests/menu-button_actions-active-descendant.js:471:3

─

2 tests failed
6 known failures

[frozenzia]

If I DON'T add those 2 lines in the toolbar test, I actually end up with 5 tests failed:

combobox_grid-combo › content/patterns/combobox/examples/grid-combo.html [data-test-id="popup-key-home"]: Home from focus on list puts focus on grid and moves cursor

test/tests/combobox_grid-combo.js:880

 879:                                                           
 880:     t.true(                                               
 881:       await confirmCursorIndex(t, ex.comboboxSelector, 0),

Cursor should be at index 0 after one ARROW_HOME key

Value is not `true`:

false

› test/tests/combobox_grid-combo.js:880:11



menu-button_actions-active-descendant › content/patterns/menu-button/examples/menu-button-actions-active-descendant.html [data-test-id="menu-end"]: "end" on role="menu"

Rejected promise returned by test. Reason:

AssertionError {
  actual: 'mi3',
  code: 'ERR_ASSERTION',
  expected: 'mi4',
  generatedMessage: false,
  operator: 'strictEqual',
  message: 'aria-activedescendant should be set to mi4 for item: #ex1 [role="menu"]',
}

› assertAriaSelectedAndActivedescendant (test/util/assertAriaActivedescendant.js:24:10)
› async test/tests/menu-button_actions-active-descendant.js:471:3



disclosure_navigation › content/patterns/disclosure/examples/disclosure-navigation.html [data-test-id="link-aria-current"]: "aria-current" attribute on links

Rejected promise returned by test. Reason:

ElementNotInteractableError {
  remoteStacktrace: `RemoteError@chrome://remote/content/shared/RemoteError.sys.mjs:8:8␊
  WebDriverError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:183:5␊
  ElementNotInteractableError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:293:5␊
  webdriverClickElement@chrome://remote/content/marionette/interaction.sys.mjs:150:11␊
  interaction.clickElement@chrome://remote/content/marionette/interaction.sys.mjs:119:11␊
  clickElement@chrome://remote/content/marionette/actors/MarionetteCommandsChild.sys.mjs:214:29␊
  receiveMessage@chrome://remote/content/marionette/actors/MarionetteCommandsChild.sys.mjs:97:31␊
  `,
  message: 'Element <a href="#mythical-page-content"> could not be scrolled into view',
}

› Object.throwDecodedError (node_modules/selenium-webdriver/lib/error.js:524:15)
› parseHttpResponse (node_modules/selenium-webdriver/lib/http.js:587:13)
› Executor.execute (node_modules/selenium-webdriver/lib/http.js:515:28)
› async thenableWebDriverProxy.execute (node_modules/selenium-webdriver/lib/webdriver.js:745:17)
› async test/tests/disclosure_navigation.js:76:9



toolbar_toolbar › content/patterns/toolbar/examples/toolbar.html [data-test-id="toolbar-button-enter-or-space"]: Test key enter on copy/paste/cut keys

test/tests/toolbar_toolbar.js:1119

 1118:                                                         
 1119:     t.is(                                               
 1120:       await buttons[copy].getAttribute('aria-disabled'),

After selecting text

Difference (- actual, + expected):

- 'true'
+ 'false'

› test/tests/toolbar_toolbar.js:1119:7



toolbar_toolbar › content/patterns/toolbar/examples/toolbar.html [data-test-id="toolbar-button-enter-or-space"]: Test key space on copy/paste/cut keys

test/tests/toolbar_toolbar.js:1211

 1210:                                                         
 1211:     t.is(                                               
 1212:       await buttons[copy].getAttribute('aria-disabled'),

After selecting text

Difference (- actual, + expected):

- 'true'
+ 'false'

› test/tests/toolbar_toolbar.js:1211:7

─

5 tests failed
6 known failures

[frozenzia]

Of the 2 tests that are still failing for me (combobox_grid-combo "popup-key-home" and menu-button_actions-active-descendant "menu-end"), popup-key-home at least works for me by hand, so not sure what the issue is there.

The 2nd one works by hand as well, but it does seem to me that there is a mistake in the test, albeit one that doesn't actually affect the outcome. The second part of the test, according to the comment, intends to "Send END to the menu while aria-activedescendant is the second item", but based on the keys sent, that only places the focus on the 1st item after the 1st part of the test has succeeded. Unless I'm missing something and there actually is a "before" bit I'm not understanding. But again, this works by hand according the test as written, OR as intended.

[frozenzia]

....and of course the added lines break automatic tests, possibly because they're prefixed by "await" (vs. alertdialog test), and Key.COMMAND doesn't return properly / at all in non-mac environment...? Removing the lines and using the same workaround that @mcking65 mentioned -- just letting the draft PR do the work for me. Annoying.

[mcking65]

@frozenzia

You are right that in an edit field where the keyboard commands are managed by the browser, they are platform and even potentially browser dependent. So, I used your comments to raise #2735.