docs(frameMessenger): clarify advanced use cases #2885

WilcoFiers · 2021-04-23T12:34:57Z

Processing some feedback from @dbjorge. Thanks for the thorough review.

dbjorge · 2021-04-23T19:32:28Z

doc/frame-messenger.md

@@ -8,10 +8,13 @@ Tools like browser extensions and testing environments often have different chan
 axe.frameMessenger({
  // Called to initialize message handling
  open(topicHandler) {
+    // Map data from the bridge to


missing end of sentence?

dbjorge · 2021-04-23T19:33:49Z

doc/frame-messenger.md

@@ -8,10 +8,13 @@ Tools like browser extensions and testing environments often have different chan
 axe.frameMessenger({
  // Called to initialize message handling
  open(topicHandler) {
+    // Map data from the bridge to
+    function subscriber(frameWin, data, response) {
+      // Data serializations / validation / etc. here


nit: this would probably be where an implementer would be deserializing data, not serializing it

dbjorge · 2021-04-23T19:35:02Z

doc/frame-messenger.md

@@ -38,6 +41,65 @@ The `open` function can `return` an optional cleanup function, which is called w

 ## axe.frameMessenger({ post })

-`post` is a function that dictates how axe-core communicates with frames. It is passed three arguments: `frameWindow`, which is the frames `contentWindow`, the `data` object, and a `replyHandler` that must be called when responses are received.
+`post` is a function that dictates how axe-core communicates with frames. It is passed three arguments: `frameWindow`, which is the frames `contentWindow`, the `data` object, and a `replyHandler` that must be called when responses are received. To inform axe-core that no message was sent, return `false`. This informs axe-core not to await for the ping to time out.


nit: frames -> frame's

dbjorge · 2021-04-23T19:44:41Z

doc/frame-messenger.md

+
+  open(topicHandler) {
+    function subscriber(frameWin, data, response) {
+      const { channelId, message, messageId } = data;


messageId isn't used, it can probably be omitted to simplify the example

dbjorge · 2021-04-23T19:44:48Z

doc/frame-messenger.md

+  },
+
+  open(topicHandler) {
+    function subscriber(frameWin, data, response) {


response isn't used, it can probably be omitted to simplify the example

doc/frame-messenger.md

dbjorge · 2021-04-23T20:07:05Z

axe.d.ts

-    replyHandler: ReplyHandler
+    message: any | Error,
+    keepalive?: boolean,
+    replyHandler?: ReplyHandler
  ) => void;
  type TopicData = { topic: String } & ReplyData;
  type ReplyData = { channelId: String; message: any; keepAlive: Boolean };


This uses a keepAlive property (uppercase A); based on the rest of the docs, L330 of the typings, and L29 of respondable.js, I think it should probably be lowercase A

dbjorge · 2021-04-23T20:08:57Z

axe.d.ts

-    replyHandler: ReplyHandler
+    message: any | Error,
+    keepalive?: boolean,
+    replyHandler?: ReplyHandler
  ) => void;
  type TopicData = { topic: String } & ReplyData;
  type ReplyData = { channelId: String; message: any; keepAlive: Boolean };


Should keepAlive be using the primitive boolean type here, or is Boolean really intended?

dbjorge

Thanks, this is a substantial improvement! A few more comments inline

doc/frame-messenger.md

axe.d.ts

@iamrafan

#4155) #### Details This PR updates `axe-core` to the recently-released v4.2.0. The bulk of the code changes here are an implementation of a custom `axe.frameMessenger` based on `browser.runtime.sendMessage` (`axe-frame-messenger.ts`), which allows us to avoid issues with target pages that intercept `window.postMessage` messages. Other notable changes include: * `test-status-choice-group.ts`: moved an `aria-label` which was violating a new `aria-allowed-attrs` rule variant (ARIA 1.2 disallows this attribute on `<span>` elements. Verified this change visually and under NVDA (the component is used both for radio buttons within assisted assessment instance tables and for the initial "pass/fail" radio buttons in manual requirements). * `webpack.config.js` required a change to work around dequelabs/axe-core#2873 * `get-rule-inclusions.test.ts.snap` enumerates the rule delta with the new version (@iamrafan is putting together fuller change notes that we'll be including with release notes when we release this) ##### Motivation See 1810733 ##### Context There are a few comments/typing workarounds in `axe-frame-messenger.ts` due to dequelabs/axe-core#2885 not quite making it in time for 4.2.0; we'll need to update to account for that when we pick up [email protected] #### Pull request checklist  - [x] Addresses an existing issue: 1810733 - [x] Ran `yarn fastpass` - [x] Added/updated relevant unit test(s) (and ran `yarn test`) - [x] Verified code coverage for the changes made. Check coverage report at: `<rootDir>/test-results/unit/coverage` - [x] PR title *AND* final merge commit title both start with a semantic tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See `CONTRIBUTING.md`. - [n/a] (UI changes only) Added screenshots/GIFs to description above - [x] (UI changes only) Verified usability with NVDA/JAWS #### Co-authors Co-authored-by: JGibson2019 <[email protected]> Co-authored-by: lisli1 <[email protected]> Co-authored-by: Madalyn <[email protected]>

straker

@dbjorge should probably also give an approval this before it's merged

doc/frame-messenger.md

straker · 2021-06-15T15:37:59Z

doc/frame-messenger.md

+
+If for some reason the frameMessenger fails to open, post, or close you should not throw an error. Axe-core will handle missing results by reporting on them in the `frame-tested` rule. It should not be possible for the `topicHandler` and `replyHandler` callbacks to throw an error. If this happens, please file an issue.
+
+Axe-core has a timeout mechanism built in, which pings frames to see if they respond before instructing them to run. There is no retry behavior in axe-core, which assumes that whatever channel is used is stable. If this isn't the case, this will need to be built into frameMessenger.


This timeout is only applicable when using respondable for the messaging and not in frame messenger itself.

Not really. It's axe-core's engine that does the ping. Respondable doesn't have that. I think I'm going to leave this as is.

dbjorge · 2021-06-15T20:12:53Z

Thanks for the update! It looks like there's still one unaddressed comment (the inconsistency between typings and usage on ReplyHandler), otherwise the updates look good to me

Co-authored-by: Steven Lambert <[email protected]>

Co-authored-by: Dan Bjorge <[email protected]>

I think that's it

* docs(frameMessenger): clarify advanced use cases * chore: address feedback * Update doc/frame-messenger.md Co-authored-by: Steven Lambert <[email protected]> * Update axe.d.ts Co-authored-by: Dan Bjorge <[email protected]> Co-authored-by: Steven Lambert <[email protected]> Co-authored-by: Dan Bjorge <[email protected]>

docs(frameMessenger): clarify advanced use cases

52eb995

WilcoFiers requested a review from a team as a code owner April 23, 2021 12:34

dylanb previously approved these changes Apr 23, 2021

View reviewed changes