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

Fix regression around CSS stacking contexts and PIP widgets #12094

Merged
merged 16 commits into from
Jan 8, 2024
Merged

Fix regression around CSS stacking contexts and PIP widgets #12094

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
4d116f5
Fix CSS stacking contexts for Dialogs & PersistedElement
t3chguy Dec 29, 2023
53e30e2
Switch to PersistedElement sharing a CSS stacking context for z-index…
t3chguy Dec 29, 2023
c9783de
Fix Widget PIP overlay being under the widget and dragging being broken
t3chguy Dec 29, 2023
1261e9e
Merge branch 'develop' into t3chguy/fix/26805
t3chguy Dec 29, 2023
0a9a97f
Merge branch 'develop' of github.com:matrix-org/matrix-react-sdk into…
t3chguy Jan 2, 2024
02177e9
Fix border-radius on widget pip
t3chguy Jan 2, 2024
76d5d19
Fix majority of tests
t3chguy Jan 2, 2024
7ca123a
Fix jest retryTimes applying outside of CI
t3chguy Jan 2, 2024
fbdcd96
Fix remaining tests
t3chguy Jan 2, 2024
977216c
Fix React unique key warnings
t3chguy Jan 2, 2024
8ad9790
Merge branch 'develop' into t3chguy/fix/26805
t3chguy Jan 2, 2024
1678e4b
Fix sticker picker
t3chguy Jan 2, 2024
d0cf422
Merge remote-tracking branch 'origin/t3chguy/fix/26805' into t3chguy/…
t3chguy Jan 2, 2024
80baf34
id not class
t3chguy Jan 2, 2024
caefcf4
Fix widget pip button colour in light theme
t3chguy Jan 4, 2024
bae9289
Revert unrelated change
t3chguy Jan 8, 2024
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/_common.pcss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,10 @@ limitations under the License.
/* This is required to ensure Compound tooltips correctly draw where they should with z-index: auto */
contain: strict;
}
.mx_Dialog_StaticContainer,
.mx_Dialog_Container {
#mx_ContextualMenu_Container,
#mx_PersistedElement_container,
#mx_Dialog_Container,
#mx_Dialog_StaticContainer {
Comment on lines +59 to +62
Copy link
Member

Choose a reason for hiding this comment

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

why are these changing?

Copy link
Member Author

Choose a reason for hiding this comment

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

So that they layer correctly relative to the PIP widget and tooltips and each other by having their own CSS stacking contexts

/* This is required to ensure Compound tooltips correctly draw where they should with z-index: auto */
isolation: isolate;
}
Expand Down
21 changes: 17 additions & 4 deletions res/css/components/views/pips/_WidgetPip.pcss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,21 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

$width: 320px;
$height: 220px;

.mx_WidgetPip {
width: 320px;
height: 220px;
width: $width;
height: $height;
}

.mx_WidgetPip_overlay {
width: $width;
height: $height;
position: absolute;
top: 0;
border-radius: 8px;
contain: paint;
overflow: hidden;
color: $call-primary-content;
cursor: pointer;
}
Expand All @@ -31,8 +41,11 @@ limitations under the License.
width: 100%;
box-sizing: border-box;
transition: opacity ease 0.15s;
}

.mx_WidgetPip:not(:hover) > & {
.mx_WidgetPip_overlay:not(:hover) {
.mx_WidgetPip_header,
.mx_WidgetPip_footer {
opacity: 0;
}
}
Expand Down
3 changes: 3 additions & 0 deletions res/css/views/rooms/_AppsDrawer.pcss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,9 @@ limitations under the License.
&.mx_AppTileBody--call {
border-radius: 0px;
}
&.mx_AppTileBody--call.mx_AppTileBody--mini {
border-radius: 8px;
}
Comment on lines +328 to +330
Copy link
Member

Choose a reason for hiding this comment

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

why is this now needed (as well as the border-radius on .mx_WidgetPip_overlay ?

(I have no real idea what a &.mx_AppTileBody--call.mx_AppTileBody--mini is, I'm afraid)

Copy link
Contributor

@toger5 toger5 Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Before the widget was always rendered the default app container with rounded borders.
This is a concious design decision to underline that it is not part of Eement web but potentially a thrid party app running as a widget. For calls we want to special case this and show the widget as if its part of Element Web.
So mx_AppTileBody--call has border-radius: 0px;.
For the PiP call view rounded corners are alwasy required. So we set it back to 8px.

Old:
Screenshot 2024-01-05 at 21 49 58
Now:
Screenshot 2024-01-05 at 21 56 58
Issue caused by new design
Screenshot 2024-01-05 at 21 53 16
With:

    &.mx_AppTileBody--call.mx_AppTileBody--mini {
        border-radius: 8px;
    }
Screenshot 2024-01-05 at 21 53 49

Copy link
Member

@richvdh richvdh Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Before the widget was always rendered the default app container with rounded borders.

I don't follow. There don't seem to be enough words in this sentence?

Eement

Element

alwasy

always

}

/* appTileBody is embedded to PersistedElement outside of mx_AppTile,
Expand Down
2 changes: 2 additions & 0 deletions src/components/structures/PipContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ class PipContainerInner extends React.Component<IProps, IState> {
const call = this.state.primaryCall;
pipContent.push(({ onStartMoving, onResize }) => (
<LegacyCallView
key="call-view"
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
onMouseDownOnHeader={onStartMoving}
call={call}
secondaryCall={this.state.secondaryCall}
Expand All @@ -317,6 +318,7 @@ class PipContainerInner extends React.Component<IProps, IState> {
if (this.state.showWidgetInPip && this.state.persistentWidgetId) {
pipContent.push(({ onStartMoving }) => (
<WidgetPip
key="widget-pip"
widgetId={this.state.persistentWidgetId!}
room={MatrixClientPeg.safeGet().getRoom(this.state.persistentRoomId ?? undefined)!}
viewingRoom={this.state.viewedRoomId === this.state.persistentRoomId}
Expand Down
27 changes: 16 additions & 11 deletions src/components/views/elements/AppTile.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ interface IProps {
showLayoutButtons?: boolean;
// Handle to manually notify the PersistedElement that it needs to move
movePersistedElement?: MutableRefObject<(() => void) | undefined>;
// An element to render after the iframe as an overlay
overlay?: ReactNode;
Comment on lines +96 to +97
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't it make more sense for this to be passed as children, rather than a regular prop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Children have different semantics of typically always being included, this only includes it when the iframe is shown, not when the loader or permissions prompt are shown

Copy link
Member

Choose a reason for hiding this comment

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

Children have different semantics of typically always being included

I'm not entirely convinced about that. And if it's true, doesn't the same logic apply to PersistentApp?

But I don't feel strongly about it.

}

interface IState {
Expand Down Expand Up @@ -663,17 +665,20 @@ export default class AppTile extends React.Component<IProps, IState> {
);
} else {
appTileBody = (
<div className={appTileBodyClass} style={appTileBodyStyles}>
{this.state.loading && loadingElement}
<iframe
title={widgetTitle}
allow={iframeFeatures}
ref={this.iframeRefChange}
src={this.sgWidget.embedUrl}
allowFullScreen={true}
sandbox={sandboxFlags}
/>
</div>
<>
<div className={appTileBodyClass} style={appTileBodyStyles}>
{this.state.loading && loadingElement}
<iframe
title={widgetTitle}
allow={iframeFeatures}
ref={this.iframeRefChange}
src={this.sgWidget.embedUrl}
allowFullScreen={true}
sandbox={sandboxFlags}
/>
</div>
{this.props.overlay}
</>
);

if (!this.props.userWidget) {
Expand Down
15 changes: 14 additions & 1 deletion src/components/views/elements/PersistedElement.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,19 @@ export const getPersistKey = (appId: string): string => "widget_" + appId;
// of doing reusable widgets like dialog boxes & menus where we go and
// pass in a custom control as the actual body.

// We contain all persisted elements within a master container to allow them all to be within the same
// CSS stacking context, and thus be able to control their z-indexes relative to each other.
function getOrCreateMasterContainer(): HTMLDivElement {
let container = getContainer("mx_PersistedElement_container");
if (!container) {
container = document.createElement("div");
container.id = "mx_PersistedElement_container";
document.body.appendChild(container);
}

return container;
}

function getContainer(containerId: string): HTMLDivElement {
return document.getElementById(containerId) as HTMLDivElement;
}
Expand All @@ -39,7 +52,7 @@ function getOrCreateContainer(containerId: string): HTMLDivElement {
if (!container) {
container = document.createElement("div");
container.id = containerId;
document.body.appendChild(container);
getOrCreateMasterContainer().appendChild(container);
}

return container;
Expand Down
1 change: 1 addition & 0 deletions src/components/views/elements/PersistentApp.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export default class PersistentApp extends React.Component<IProps> {
showMenubar={false}
pointerEvents={this.props.pointerEvents}
movePersistedElement={this.props.movePersistedElement}
overlay={this.props.children}
/>
);
}
Expand Down
49 changes: 26 additions & 23 deletions src/components/views/pips/WidgetPip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,34 +107,37 @@ export const WidgetPip: FC<Props> = ({ widgetId, room, viewingRoom, onStartMovin

return (
<div className="mx_WidgetPip" onMouseDown={onStartMoving} onClick={onBackClick}>
<Toolbar className="mx_WidgetPip_header">
<RovingAccessibleButton
onClick={onBackClick}
className="mx_WidgetPip_backButton"
aria-label={_t("action|back")}
>
<BackIcon className="mx_Icon mx_Icon_16" />
{roomName}
</RovingAccessibleButton>
</Toolbar>
<PersistentApp
persistentWidgetId={widgetId}
persistentRoomId={room.roomId}
pointerEvents="none"
movePersistedElement={movePersistedElement}
/>
{(call !== null || WidgetType.JITSI.matches(widget?.type)) && (
<Toolbar className="mx_WidgetPip_footer">
<RovingAccessibleTooltipButton
onClick={onLeaveClick}
tooltip={_t("action|leave")}
aria-label={_t("action|leave")}
alignment={Alignment.Top}
>
<HangupIcon className="mx_Icon mx_Icon_24" />
</RovingAccessibleTooltipButton>
</Toolbar>
)}
>
<div onMouseDown={onStartMoving} className="mx_WidgetPip_overlay">
<Toolbar className="mx_WidgetPip_header">
<RovingAccessibleButton
onClick={onBackClick}
className="mx_WidgetPip_backButton"
aria-label={_t("action|back")}
>
<BackIcon className="mx_Icon mx_Icon_16" />
{roomName}
</RovingAccessibleButton>
</Toolbar>
{(call !== null || WidgetType.JITSI.matches(widget?.type)) && (
<Toolbar className="mx_WidgetPip_footer">
<RovingAccessibleTooltipButton
onClick={onLeaveClick}
tooltip={_t("action|leave")}
aria-label={_t("action|leave")}
alignment={Alignment.Top}
>
<HangupIcon className="mx_Icon mx_Icon_24" />
</RovingAccessibleTooltipButton>
</Toolbar>
)}
</div>
</PersistentApp>
</div>
);
};
22 changes: 18 additions & 4 deletions test/components/structures/PipContainer-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,16 @@ import { WidgetType } from "../../../src/widgets/WidgetType";
import { SdkContextClass } from "../../../src/contexts/SDKContext";
import { ElementWidgetActions } from "../../../src/stores/widgets/ElementWidgetActions";

jest.mock("../../../src/stores/OwnProfileStore", () => ({
OwnProfileStore: {
instance: {
isProfileInfoFetched: true,
removeListener: jest.fn(),
getHttpAvatarUrl: jest.fn().mockReturnValue("http://avatar_url"),
},
},
}));

describe("PipContainer", () => {
useMockedCalls();
jest.spyOn(HTMLMediaElement.prototype, "play").mockImplementation(async () => {});
Expand Down Expand Up @@ -91,6 +101,8 @@ describe("PipContainer", () => {

stubClient();
client = mocked(MatrixClientPeg.safeGet());
client.getUserId.mockReturnValue("@alice:example.org");
client.getSafeUserId.mockReturnValue("@alice:example.org");
DMRoomMap.makeShared(client);

room = new Room("!1:example.org", client, "@alice:example.org", {
Expand Down Expand Up @@ -161,6 +173,7 @@ describe("PipContainer", () => {
if (!(call instanceof MockedCall)) throw new Error("Failed to create call");

const widget = new Widget(call.widget);
WidgetStore.instance.addVirtualWidget(call.widget, room.roomId);
WidgetMessagingStore.instance.storeMessaging(widget, room.roomId, {
stop: () => {},
} as unknown as ClientWidgetApi);
Expand All @@ -175,6 +188,7 @@ describe("PipContainer", () => {
cleanup();
call.destroy();
ActiveWidgetStore.instance.destroyPersistentWidget(widget.id, room.roomId);
WidgetStore.instance.removeVirtualWidget(widget.id, room.roomId);
};

const withWidget = async (fn: () => Promise<void>): Promise<void> => {
Expand Down Expand Up @@ -265,7 +279,7 @@ describe("PipContainer", () => {
const widget = WidgetStore.instance.addVirtualWidget(
{
id: "1",
creatorUserId: "@alice:exaxmple.org",
creatorUserId: "@alice:example.org",
type: WidgetType.CUSTOM.preferred,
url: "https://example.org",
name: "Example widget",
Expand All @@ -279,7 +293,7 @@ describe("PipContainer", () => {

// The return button should maximize the widget
const moveSpy = jest.spyOn(WidgetLayoutStore.instance, "moveToContainer");
await user.click(screen.getByRole("button", { name: "Back" }));
await user.click(await screen.findByRole("button", { name: "Back" }));
expect(moveSpy).toHaveBeenCalledWith(room, widget, Container.Center);

expect(screen.queryByRole("button", { name: "Leave" })).toBeNull();
Expand All @@ -295,7 +309,7 @@ describe("PipContainer", () => {
const widget = WidgetStore.instance.addVirtualWidget(
{
id: "1",
creatorUserId: "@alice:exaxmple.org",
creatorUserId: "@alice:example.org",
type: WidgetType.JITSI.preferred,
url: "https://meet.example.org",
name: "Jitsi example",
Expand All @@ -310,7 +324,7 @@ describe("PipContainer", () => {
// The return button should view the room
const dispatcherSpy = jest.fn();
const dispatcherRef = defaultDispatcher.register(dispatcherSpy);
await user.click(screen.getByRole("button", { name: "Back" }));
await user.click(await screen.findByRole("button", { name: "Back" }));
expect(dispatcherSpy).toHaveBeenCalledWith({
action: Action.ViewRoom,
room_id: room.roomId,
Expand Down
8 changes: 5 additions & 3 deletions test/setupTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,11 @@ beforeEach(() => {
});

// Retry to work around our flaky app & tests
jest.retryTimes(2, {
logErrorsBeforeRetry: true,
});
if (process.env.CI) {
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
jest.retryTimes(2, {
logErrorsBeforeRetry: true,
});
}

// Very carefully enable the mocks for everything else in
// a specific order. We use this order to ensure we properly
Expand Down
Loading