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

Chat: use resizeObserver to update scrollbar size and position #28111

Merged
Merged
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
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
84 changes: 83 additions & 1 deletion e2e/testcafe-devextreme/tests/chat/messageList.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { createScreenshotsComparer } from 'devextreme-screenshot-comparer';
import Chat from 'devextreme-testcafe-models/chat';
import { createUser, generateMessages } from './data';
import { ClientFunction } from 'testcafe';
import { User } from 'devextreme/ui/chat';
import TabPanel from 'devextreme-testcafe-models/tabPanel';
import { createUser, generateMessages, getLongText } from './data';
import url from '../../helpers/getPageUrl';
import { createWidget } from '../../helpers/createWidget';
import { testScreenshot } from '../../helpers/themeUtils';
Expand Down Expand Up @@ -45,8 +48,48 @@ test('Messagelist empty view scenarios', async (t) => {
test('Messagelist appearance with scrollbar', async (t) => {
const { takeScreenshot, compareResults } = createScreenshotsComparer(t);

const chat = new Chat('#container');

await t
.hover(chat.messageList)
.wait(400);

await testScreenshot(t, takeScreenshot, 'Messagelist with a lot of messages.png', { element: '#container' });

await ClientFunction(
() => {
const instance = chat.getInstance();
instance.renderMessage({
author: instance.option('user') as User,
text: 'Lorem ipsum dolor sit amet, \nconsectetur adipiscing elit. Sed do eiusmod tempor \nincididunt ut labore et dolore magna aliqua. Ut enim ad minim \nveniam, quis nostrud exercitation ullamco laboris nisi ut aliquip \nnex ea commodo consequat.',
});
},
{ dependencies: { chat } },
)();

await testScreenshot(t, takeScreenshot, 'Messagelist scrollbar position after call renderMessage().png', { element: '#container' });

await t
.typeText(chat.getInput(), getLongText())
.pressKey('shift+enter');

await testScreenshot(t, takeScreenshot, 'Messagelist scrollbar position after typing in textarea.png', { element: '#container' });

await t
.pressKey('enter');

await testScreenshot(t, takeScreenshot, 'Messagelist scrollbar position after send.png', { element: '#container' });

const scrollable = chat.getScrollable();
const topOffset = (await scrollable.scrollOffset()).top;

await scrollable.scrollTo({ top: topOffset - 100 });

await t
.typeText(chat.getInput(), getLongText());

await testScreenshot(t, takeScreenshot, 'Messagelist scrollbar middle position after typing in textarea.png', { element: '#container' });

await t
.expect(compareResults.isValid())
.ok(compareResults.errorMessages());
Expand All @@ -63,3 +106,42 @@ test('Messagelist appearance with scrollbar', async (t) => {
height: 600,
});
});

test('Messagelist should scrolled to the latest messages after being rendered inside an invisible element', async (t) => {
const { takeScreenshot, compareResults } = createScreenshotsComparer(t);

const tabPanel = new TabPanel('#container');

await t
.click(tabPanel.tabs.getItem(1).element);

await testScreenshot(t, takeScreenshot, 'Messagelist scroll position after rendering in invisible container.png', { element: '#container' });

await t
.expect(compareResults.isValid())
.ok(compareResults.errorMessages());
}).before(async () => {
const userFirst = createUser(1, 'First');
const userSecond = createUser(2, 'Second');

const items = generateMessages(17, userFirst, userSecond, true, false, 2);

return createWidget('dxTabPanel', {
width: 400,
height: 500,
deferRendering: true,
templatesRenderAsynchronously: true,
dataSource: [{
title: 'Tab_1',
collapsible: true,
text: 'Tab_1 content',
}, {
title: 'Tab_2',
collapsible: true,
template: ClientFunction(() => ($('<div>') as any).dxChat({
items,
user: userSecond,
}), { dependencies: { items, userSecond } }),
}],
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,19 @@
flex-grow: 1;
overflow: hidden;

.dx-scrollable-container {
overscroll-behavior: contain;
will-change: scroll-position;
}

.dx-scrollable-content {
display: flex;
flex-direction: column;
width: 100%;
}

.dx-scrollable-native {
&.dx-scrollable-native-ios{
&.dx-scrollable-native-ios {
.dx-scrollable-content {
min-height: 100%;
}
Expand Down
6 changes: 0 additions & 6 deletions packages/devextreme/js/__internal/ui/chat/chat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,6 @@ class Chat extends Widget<Properties> {

this._renderMessageList();
this._renderMessageBox();

this._messageList.update();
}

_renderHeader(title: string): void {
Expand Down Expand Up @@ -138,10 +136,6 @@ class Chat extends Widget<Properties> {
return $input;
}

_dimensionChanged(): void {
this._messageList.update();
}

_optionChanged(args: OptionChanged<Properties>): void {
const { name, value } = args;

Expand Down
3 changes: 0 additions & 3 deletions packages/devextreme/js/__internal/ui/chat/messagebox.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import $ from '@js/core/renderer';
import type { NativeEventInfo } from '@js/events';
import { triggerResizeEvent } from '@js/events/visibility_change';
import type { ClickEvent } from '@js/ui/button';
import Button from '@js/ui/button';
import type { Properties as DOMComponentProperties } from '@ts/core/widget/dom_component';
Expand Down Expand Up @@ -91,8 +90,6 @@ class MessageBox extends DOMComponent<MessageBox, Properties> {
const shouldButtonBeDisabled = !this._isValuableTextEntered();

this._toggleButtonDisableState(shouldButtonBeDisabled);

triggerResizeEvent(this.$element().parent());
},
onEnterKey: (e: EnterKeyEvent): void => {
if (!e.event?.shiftKey) {
Expand Down
77 changes: 65 additions & 12 deletions packages/devextreme/js/__internal/ui/chat/messagelist.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
import domAdapter from '@js/core/dom_adapter';
import type { dxElementWrapper } from '@js/core/renderer';
import $ from '@js/core/renderer';
import resizeObserverSingleton from '@js/core/resize_observer';
import { contains } from '@js/core/utils/dom';
import { hasWindow } from '@js/core/utils/window';
import messageLocalization from '@js/localization/message';
import {
isReachedBottom,
} from '@js/renovation/ui/scroll_view/utils/get_boundary_props';
import { getScrollTopMax } from '@js/renovation/ui/scroll_view/utils/get_scroll_top_max';
import type { Message } from '@js/ui/chat';
import Scrollable from '@js/ui/scroll_view/ui.scrollable';
import type { WidgetOptions } from '@js/ui/widget/ui.widget';
import type { OptionChanged } from '@ts/core/widget/types';
import Widget from '@ts/core/widget/widget';

import { isElementVisible } from '../splitter/utils/layout';
import type { MessageGroupAlignment } from './messagegroup';
import MessageGroup from './messagegroup';

Expand All @@ -23,7 +32,11 @@ export interface Properties extends WidgetOptions<MessageList> {
}

class MessageList extends Widget<Properties> {
_messageGroups?: MessageGroup[];
private _messageGroups?: MessageGroup[];

private _containerClientHeight = 0;

private _suppressResizeHandling?: boolean;

private _scrollable!: Scrollable<unknown>;

Expand All @@ -50,7 +63,47 @@ class MessageList extends Widget<Properties> {

this._renderMessageListContent();

this.update();
this._attachResizeObserverSubscription();
Copy link
Contributor

Choose a reason for hiding this comment

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

[SSR logic]
I believe it should be done in _renderContent, not _initMarkup.
Then u don't need if (hasWindow()) { inside of this method


this._suppressResizeHandling = true;
}

_attachResizeObserverSubscription(): void {
if (hasWindow()) {
const element = this._getScrollContainer();

resizeObserverSingleton.unobserve(element);
resizeObserverSingleton.observe(element, (entry) => this._resizeHandler(entry));
}
}

_isAttached(element: Element): boolean {
return !!contains(domAdapter.getBody(), element);
Copy link
Contributor

Choose a reason for hiding this comment

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

[copypaste, shadowDOM]
I think we should use isElementInDom from utils/dom instead of creating this new method.
Moreover, now the solution does now work in shadowDOM as far as I see

}

_resizeHandler({ contentRect, target }: ResizeObserverEntry): void {
const newHeight = contentRect.height;

if (this._suppressResizeHandling
Copy link
Contributor

Choose a reason for hiding this comment

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

[naming, ?excess cache?]
u name it _suppressResizeHandling, but i see u don't suppress, u scroll to last message.
So, what is this for, just for indicating it's a first call, right?

Then maybe we can get rid of this private cache and just do it this way?

const isAfterFirstRendering = this._containerClientHeight === 0; // or even undefined/null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can simplified it this way:

if (!isElementInDom(target) || !isElementVisible(target as HTMLElement)) {
      return;
    }

    if (!isDefined(this._containerClientHeight)) {
      this._scrollContentToLastMessage();
    }

&& this._isAttached(target)
&& isElementVisible(target as HTMLElement)
Copy link
Contributor

Choose a reason for hiding this comment

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

[refactoring]
Can we isolate these conditions? Let it be a first if and return if element is invisible or detached.
Now i don't like we do smth actions in this case + update cached height

) {
this._scrollContentToLastMessage();

this._suppressResizeHandling = false;
} else {
const heightChange = this._containerClientHeight - newHeight;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could u please add a Unit test for this scenario? Smth like "scrollable should keep scrolling position after container resize if scrolling position is not bottom"
It was very difficult for me now to understand without unit tests what it's written for


let { scrollTop } = target;

if (heightChange >= 1 || !isReachedBottom(target as HTMLDivElement, target.scrollTop, 0, 1)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[excess code?]
I don't understand why we need heightChange.
U're trying to say "we need some scroll if we are now on bottom and height is increased" but i see no difference with and without this code

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (heightChange >= 1 || !isReachedBottom(target as HTMLDivElement, target.scrollTop, 0, 1)) {
const isReachedBottom = isReachedBottom(target as HTMLDivElement, target.scrollTop, 0, 1);
if (!isReachedBottom) {

scrollTop += heightChange;
}

this._scrollable.scrollTo({ top: scrollTop });
}

this._containerClientHeight = newHeight;
}

_renderEmptyViewContent(): void {
Expand Down Expand Up @@ -162,23 +215,30 @@ class MessageList extends Widget<Properties> {

if (sender?.id === lastMessageGroupUserId) {
lastMessageGroup.renderMessage(message);
this.update();
this._scrollContentToLastMessage();

return;
}
}

this._createMessageGroupComponent([message], sender?.id);

this.update();
this._scrollContentToLastMessage();
}

_$content(): dxElementWrapper {
return $(this._scrollable.content());
}

_scrollContentToLastMessage(): void {
this._scrollable.scrollTo({ top: this._$content().get(0).scrollHeight });
const scrollOffsetTopMax = getScrollTopMax(this._getScrollContainer());

this._scrollable.scrollTo({ top: scrollOffsetTopMax });
}

_getScrollContainer(): HTMLElement {
Copy link
Contributor

Choose a reason for hiding this comment

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

[refactoring]
do we really need it? Maybe subscribe just on root?

// @ts-expect-error
return $(this._scrollable.container()).get(0);
}

_clean(): void {
Expand Down Expand Up @@ -238,13 +298,6 @@ class MessageList extends Widget<Properties> {
super._optionChanged(args);
}
}

update(): void {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this._scrollable.update();

this._scrollContentToLastMessage();
}
}

export default MessageList;
Loading
Loading