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

Bump deps and remove remark and strip-markdown #3917

Merged
merged 37 commits into from
Jun 3, 2021

Conversation

compulim
Copy link
Contributor

@compulim compulim commented May 27, 2021

Fixes #3360. Fixes #3615.

Changelog Entry

Fixed

Changed

Description

Bump dependencies with npm audit, and a few more:

We removed remark and strip-markdown. They are used to extract screen reader text from activity:

  • Yesterday, we stripped Markdown syntax from activity.text field using remark and strip-markdown packages
  • Tomorrow
    • If activity.channelData['webchat:fallback-text'] field is available, we will use it as-is
    • Otherwise, we will strip Markdown from activity.text field with best effort
      1. Render text using useRenderMarkdown hook into HTML string
      2. Use new DOMParser().parseFromString() to parse HTML string as HTMLDocument
      3. Walk the HTMLDocument and concatenates all text nodes

Please refer to ACCESSIBILITY.md for details.

Design

Considerations on removing remark

Previously, we use remark to implement the "extract screen reader text from Markdown" feature. But the feature itself is causing a few issues: #3360 and #3615.

Although remark@10 has vulnerability in their dependencies and they fixed it in remark@12, they no longer support IE11.

We have a newer logic for stripping Markdown syntax. It will work with HTML tags with best effort. However, bot developers are strongly recommended to provide speak field for precise narration. See ACCESSIBILITY.md for details.

Specific Changes

  • Bumped dependencies with npm audit and more:
  • Removed remark and strip-markdown to extract screen reader text from text field
    • Instead, we will extract screen reader text from channelData['webchat:fallback-text'] field, fallback to remove Markdown syntax from text field with best effort
    • Strip Markdown logic will use new DOMParser().parseFromString() and walk the HTMLDocument manually
  • I have added tests and executed them locally
  • I have updated CHANGELOG.md
  • I have updated documentation

Review Checklist

This section is for contributors to review your work.

  • Accessibility reviewed (tab order, content readability, alt text, color contrast)
  • Browser and platform compatibilities reviewed
  • CSS styles reviewed (minimal rules, no z-index)
  • Documents reviewed (docs, samples, live demo)
  • Internationalization reviewed (strings, unit formatting)
  • package.json and package-lock.json reviewed
  • Security reviewed (no data URIs, check for nonce leak)
  • Tests reviewed (coverage, legitimacy)

docs/ACCESSIBILITY.md Outdated Show resolved Hide resolved
docs/ACCESSIBILITY.md Outdated Show resolved Hide resolved
Copy link
Contributor

@corinagum corinagum left a comment

Choose a reason for hiding this comment

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

Pending some comments. 'Outdated' status doesn't necessarily mean the comment is obsolete; could you look through them too?

@compulim
Copy link
Contributor Author

compulim commented Jun 3, 2021

Ready for next pass.

Copy link
Contributor

@corinagum corinagum left a comment

Choose a reason for hiding this comment

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

LGTM!

@corinagum corinagum merged commit 3d75dee into microsoft:main Jun 3, 2021
compulim added a commit that referenced this pull request Jun 22, 2021
* Bump deps

* Go back to v1

* Remove remark and strip-markdown

* Revert to v1

* Revert to v1

* Revert to v1

* Supports summary field

* Ignore summary field

* Strip Markdown from HTML

* Add speak field not present

* Update strip Markdown

* Narrate empty text field

* Use different text computation for HTML/Markdown

* Clean up

* Update with samples

* More inline elements

* Clean up

* MessageBack must not be Markdown

* Lockdown performance

* Add snapshots

* Fix MessageBack

* Remove renderMarkdownAsHTML

* Fix ESLint

* Assume plain text if renderMarkdown is null

* Compute text alternatives using alt field

* Change to webchat:fallback-text field

* Bump to [email protected]

* Typo

* Update entry

* Typo

* Update entry

* Link to issue

* Undo package*.json for core/embed

* Move allTextContents into testHelpers

* Add @param

* Apply PR suggestions

* Update text
@compulim compulim mentioned this pull request Jul 8, 2021
65 tasks
@compulim compulim mentioned this pull request Sep 2, 2021
70 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants