instructure / canvas-lms

The open LMS by Instructure, Inc.
https://github.com/instructure/canvas-lms/wiki
GNU Affero General Public License v3.0
5.4k stars 2.42k forks source link

Fixed search highlighting in discussions #2364

Closed seanbeirnes closed 14 hours ago

seanbeirnes commented 3 weeks ago

Overview

The addSearchHighlighting() method in SearchSpan.jsx highlights the search term provided by a user when they use the search functionality in the Discussion/Announcements Redesign. This method also removes all HTML tags from the original discussion post when it is called, making the search results not look like the original post. This results in a degraded user experience. The proposed change maintains the structure of the HTML in the original post while highlighting the search term.

Reasons for Proposal

Degraded User Experience

Currently, the addSearchHighlighting() method removes all HTML tags (except iFrame tags) from the discussion post when highlighting the search term. This causes a degraded user experience because the method converts a user's original post into a single paragraph, no matter how the HTML was originally structured. All inline styling, line breaks, and unordered/ordered lists are removed. Because of this behavior, the addSearchHighlighting() method arguably works against its intended purpose of providing a useful feature by instead offering a confusing user experience. If a user is familiar with the visual appearnce of an original post, it may be more difficult for the user to recognize the same post in the search results since it will look vastly different.

Accessibility Concerns

WCAG 2.2 SC 1.3.1 Info and Relationships

Removing the all tags from the search result removes the seamntics provided in the original post, such as bulleted/numbered lists, paragraphs, and headings. This results in failure to meet WCAG 2.2 SC 1.3.1 Info and Relationships (Level A) The search result version of the post may appear to be very different from the original post to someone using a screen reader.

WCAG 2.2 SC 1.4.1 Use of Color

The highlighted color provides some contrast, but it may not be enough for some users. To improve conformance with WCAG 2.2 SC 1.4.1 Use of Color (Level A), the highlighted text could use a bold font weight.

Proposed Change

Maintaining HTML Structure

I rewrote the addSearchHighlighting() method so that it maintains the HTML structure of the original discussion post. The new method will first check that a '<' is present in the searchArea argument. If so, then the method applies the highlightText method since there is no underlying HTML structure to maintain.

Otherwise, the searchArea argument is parsed to separate the HTML tags from plaintext elements. These elements are stored separately in the textAndTags array. While the searchArea argument is parsed, all plaintext elements are checked to see if they contain the searchTerm argument. If the searchTerm is in a plaintext element, the index of that element in the textAndTags array is added to the textAndTagsMatches array. This helps improve performance so the textAndTags array does not need to be iterated over later.

Finally, indecies in textAndTagsMatches are used to apply the highlightText method to the corresponding element in textAndTags before being concatenated in the return statement.

Improving Accessibility

Maintaining the HTML structure helps increase conformance to WCAG 2.2 SC 1.3.1. A font-weight: bold inline style was added in the return statement of highlightText to provide additional contrast and increase conformance with WCAG 2.2 SC 1.4.1.

Test Plan

Because the addSearchHighlighting() method was rewritten to not remove HTML tags, the tests in SearchSpanText.test.jsx will need to be modified accordingly. The current tests will fail until they are set to accomodate a longer length. I also suggest adding an additional test that includes some div, ul, ol, li, and h2 tags with and without inline styling created by the Rich Content Editor.

CLAassistant commented 3 weeks ago

CLA assistant check
All committers have signed the CLA.