Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

Fix outstanding UX issues with replies/mentions/keyword notifs #85

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions res/css/views/elements/_Pill.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ Please see LICENSE files in the repository root for full details.
}

&.mx_UserPill_me,
&.mx_AtRoomPill {
&.mx_AtRoomPill,
&.mx_KeywordPill {
background-color: var(--cpd-color-bg-critical-primary) !important; /* To override .markdown-body */
}

Expand All @@ -45,7 +46,8 @@ Please see LICENSE files in the repository root for full details.
}

/* We don't want to indicate clickability */
&.mx_AtRoomPill:hover {
&.mx_AtRoomPill:hover,
&.mx_KeywordPill:hover {
background-color: var(--cpd-color-bg-critical-primary) !important; /* To override .markdown-body */
cursor: unset;
}
Expand Down
14 changes: 8 additions & 6 deletions res/css/views/rooms/_EventTile.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,6 @@ $left-gutter: 64px;
}
}

&.mx_EventTile_highlight,
&.mx_EventTile_highlight .markdown-body,
&.mx_EventTile_highlight .mx_EventTile_edited {
color: $alert;
}

&.mx_EventTile_bubbleContainer {
display: grid;
grid-template-columns: 1fr 100px;
Expand Down Expand Up @@ -580,6 +574,14 @@ $left-gutter: 64px;
padding-inline-start: calc(var(--EventTile_group_line-spacing-inline-start) + 20px);
}
}

&.mx_EventTile_highlight,
&.mx_EventTile_highlight .markdown-body {
> .mx_EventTile_line {
box-shadow: inset var(--EventTile-box-shadow-offset-x) 0 0 var(--EventTile-box-shadow-spread-radius)
$event-highlight-edge-color;
}
}
}

&[data-layout="bubble"] {
Expand Down
1 change: 1 addition & 0 deletions res/themes/dark/css/_dark.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ $spacePanel-bg-color: rgba(38, 39, 43, 0.82);
$panel-gradient: rgba(34, 38, 46, 0), rgba(34, 38, 46, 1);
$h3-color: $primary-content;
$event-highlight-bg-color: #25271f;
$event-highlight-edge-color: #f0b132;
$header-panel-text-primary-color: $text-secondary-color;
/* ******************** */

Expand Down
1 change: 1 addition & 0 deletions res/themes/legacy-dark/css/_legacy-dark.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ $widget-menu-bar-bg-color: $header-panel-bg-color;
$widget-body-bg-color: #1a1d23;

$event-highlight-bg-color: #25271f;
$event-highlight-edge-color: #f0b132;

/* event timestamp */
$event-timestamp-color: $text-secondary-color;
Expand Down
1 change: 1 addition & 0 deletions res/themes/legacy-light/css/_legacy-light.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ $widget-body-bg-color: #fff;
$yellow-background: #fff8e3;

$event-highlight-bg-color: $yellow-background;
$event-highlight-edge-color: var(--cpd-color-yellow-500);

/* event timestamp */
$event-timestamp-color: #acacac;
Expand Down
1 change: 1 addition & 0 deletions res/themes/light-custom/css/_custom.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ $button-danger-disabled-bg-color: var(--warning-color-50pct); /* still needs alp
/* --timeline-highlights-color */
$event-selected-color: var(--timeline-highlights-color);
$event-highlight-bg-color: var(--timeline-highlights-color);
$event-highlight-edge-color: var(--timeline-highlights-color);

/* redirect some variables away from their hardcoded values in the light theme */
$settings-grey-fg-color: $primary-content;
Expand Down
1 change: 1 addition & 0 deletions res/themes/light/css/_light.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ $spacePanel-bg-color: rgba(232, 232, 232, 0.77);
$panel-gradient: rgba(242, 245, 248, 0), rgba(242, 245, 248, 1);
$h3-color: #3d3b39;
$event-highlight-bg-color: $yellow-background;
$event-highlight-edge-color: var(--cpd-color-yellow-500);
$header-panel-text-primary-color: #91a1c0;
/* ******************** */

Expand Down
26 changes: 24 additions & 2 deletions src/components/views/elements/Pill.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export enum PillType {
AtRoomMention = "TYPE_AT_ROOM_MENTION", // '@room' mention
EventInSameRoom = "TYPE_EVENT_IN_SAME_ROOM",
EventInOtherRoom = "TYPE_EVENT_IN_OTHER_ROOM",
Keyword = "TYPE_KEYWORD", // Used to highlight keywords that triggered a notification rule
}

export const pillRoomNotifPos = (text: string | null): number => {
Expand Down Expand Up @@ -77,14 +78,32 @@ export interface PillProps {
room?: Room;
// Whether to include an avatar in the pill
shouldShowPillAvatar?: boolean;
// Explicitly-provided text to display in the pill
text?: string;
}

export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room, shouldShowPillAvatar = true }) => {
const { event, member, onClick, resourceId, targetRoom, text, type } = usePermalink({
export const Pill: React.FC<PillProps> = ({
type: propType,
url,
inMessage,
room,
shouldShowPillAvatar = true,
text: customPillText,
}) => {
const {
event,
member,
onClick,
resourceId,
targetRoom,
text: linkText,
type,
} = usePermalink({
room,
type: propType,
url,
});
const text = customPillText ?? linkText;

if (!type || !text) {
return null;
Expand All @@ -97,6 +116,7 @@ export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room
mx_UserPill: type === PillType.UserMention,
mx_UserPill_me: resourceId === MatrixClientPeg.safeGet().getUserId(),
mx_EventPill: type === PillType.EventInOtherRoom || type === PillType.EventInSameRoom,
mx_KeywordPill: type === PillType.Keyword,
});

let avatar: ReactElement | null = null;
Expand Down Expand Up @@ -132,6 +152,8 @@ export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room
case PillType.UserMention:
avatar = <PillMemberAvatar shouldShowPillAvatar={shouldShowPillAvatar} member={member} />;
break;
case PillType.Keyword:
break;
default:
return null;
}
Expand Down
63 changes: 62 additions & 1 deletion src/components/views/messages/TextualBody.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ Please see LICENSE files in the repository root for full details.

import React, { createRef, SyntheticEvent, MouseEvent } from "react";
import ReactDOM from "react-dom";
import { MsgType } from "matrix-js-sdk/src/matrix";
import { MsgType, PushRuleKind } from "matrix-js-sdk/src/matrix";
import { globToRegexp } from "matrix-js-sdk/src/utils";

import * as HtmlUtils from "../../../HtmlUtils";
import { formatDate } from "../../../DateUtils";
Expand Down Expand Up @@ -39,6 +40,7 @@ import { getParentEventId } from "../../../utils/Reply";
import { EditWysiwygComposer } from "../rooms/wysiwyg_composer";
import { IEventTileOps } from "../rooms/EventTile";
import { MatrixClientPeg } from "../../../MatrixClientPeg";
import { Pill, PillType } from "../elements/Pill";

const MAX_HIGHLIGHT_LENGTH = 4096;

Expand Down Expand Up @@ -124,6 +126,16 @@ export default class TextualBody extends React.Component<IBodyProps, IState> {
}, 10);
}
}

// Highlight notification keywords using pills
const pushDetails = this.props.mxEvent.getPushDetails();
if (
pushDetails.rule?.enabled &&
pushDetails.rule.kind === PushRuleKind.ContentSpecific &&
pushDetails.rule.pattern
) {
this.pillifyNotificationKeywords([content], this.regExpForKeywordPattern(pushDetails.rule.pattern));
}
}

private addCodeElement(pre: HTMLPreElement): void {
Expand Down Expand Up @@ -351,6 +363,55 @@ export default class TextualBody extends React.Component<IBodyProps, IState> {
}
}

/**
* Marks the text that activated a push-notification keyword pattern.
*/
private pillifyNotificationKeywords(nodes: ArrayLike<Element>, exp: RegExp): void {
let node: Node | null = nodes[0];
while (node) {
if (node.nodeType === Node.TEXT_NODE) {
const text = node.nodeValue;
if (!text) {
node = node.nextSibling;
continue;
}
const match = text.match(exp);
if (!match || match.length < 3) {
node = node.nextSibling;
continue;
}
const keywordText = match[2];
const idx = match.index! + match[1].length;
const before = text.substring(0, idx);
const after = text.substring(idx + keywordText.length);

const container = document.createElement("span");
const newContent = (
<>
{before}
<Pill text={keywordText} type={PillType.Keyword}>
{keywordText}
</Pill>
{after}
</>
);
ReactDOM.render(newContent, container);

node.parentNode?.replaceChild(container, node);
} else if (node.childNodes && node.childNodes.length) {
this.pillifyNotificationKeywords(node.childNodes as NodeListOf<Element>, exp);
}

node = node.nextSibling;
}
}

private regExpForKeywordPattern(pattern: string): RegExp {
// Reflects the push notification pattern-matching implementation at
// https://github.com/matrix-org/matrix-js-sdk/blob/dbd7d26968b94700827bac525c39afff2c198e61/src/pushprocessor.ts#L570
return new RegExp("(^|\\W)(" + globToRegexp(pattern) + ")(\\W|$)", "i");
}

private findLinks(nodes: ArrayLike<Element>): string[] {
let links: string[] = [];

Expand Down
19 changes: 18 additions & 1 deletion test/components/views/messages/TextualBody-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ Please see LICENSE files in the repository root for full details.
*/

import React from "react";
import { MatrixClient, MatrixEvent } from "matrix-js-sdk/src/matrix";
import { MatrixClient, MatrixEvent, PushRuleKind } from "matrix-js-sdk/src/matrix";
import { mocked, MockedObject } from "jest-mock";
import { render } from "@testing-library/react";

Expand Down Expand Up @@ -228,6 +228,23 @@ describe("<TextualBody />", () => {
const content = container.querySelector(".mx_EventTile_body");
expect(content.innerHTML.replace(defaultEvent.getId(), "%event_id%")).toMatchSnapshot();
});

it("should pillify a keyword responsible for triggering a notification", () => {
const ev = mkRoomTextMessage("foo bar baz");
ev.setPushDetails(undefined, {
actions: [],
pattern: "bar",
rule_id: "bar",
default: false,
enabled: true,
kind: PushRuleKind.ContentSpecific,
});
const { container } = getComponent({ mxEvent: ev });
const content = container.querySelector(".mx_EventTile_body");
expect(content.innerHTML).toMatchInlineSnapshot(
`"<span>foo <bdi><span tabindex="0"><span class="mx_Pill mx_KeywordPill"><span class="mx_Pill_text">bar</span></span></span></bdi> baz</span>"`,
);
});
});

describe("renders formatted m.text correctly", () => {
Expand Down
Loading