Page MenuHomePhabricator

[web] Scrolling to the edited message when it overflows
ClosedPublic

Authored by kuba on May 30 2023, 4:45 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 5:29 PM
Unknown Object (File)
Thu, Dec 19, 1:32 PM
Unknown Object (File)
Wed, Dec 18, 1:53 PM
Unknown Object (File)
Wed, Dec 18, 1:29 PM
Unknown Object (File)
Wed, Dec 18, 1:27 PM
Unknown Object (File)
Wed, Dec 18, 1:08 PM
Unknown Object (File)
Wed, Dec 18, 1:03 PM
Unknown Object (File)
Wed, Dec 18, 12:57 PM

Details

Summary

Currently, if the user wants to edit a message which overflows (near the top or bottom of the ChatMessageList), the edit box may also overflow.
This diff addresses that, if the message edit box may overflow, we first scroll so that the edited message is in the center, and then open the edit box.

Test Plan

Run the app on Chrome, and checked:

  • entered edit mode for a message which overflows, checked if the browser scrolled to it, and opened the edit box,
  • entered edit mode for a message in the middle, checked if the edit box was opened instantly and without scrolling,
  • added new lines for the edited message, and checked if it still fits in the container.

Videos:

Diff Detail

Repository
rCOMM Comm
Branch
kuba/message-editing-web
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
web/chat/chat-message-list.react.js
225 ↗(On Diff #27260)

I would change this name to something that suggests that this has something to do with edits. Like isMessageEditWindowOverflowed

Just some nits, not a full review. This one is pretty complicated, so would appreciate a detailed review by one of the listed reviewers!

web/chat/chat-message-list.react.js
72–74 ↗(On Diff #27260)

If we don't care about the return type of a prop, we should use mixed

212 ↗(On Diff #27260)

Use mixed when return type doesn't matter

kuba marked 3 inline comments as done.

Respond to the review

web/chat/chat-message-list.react.js
72 ↗(On Diff #27714)

I don't think this is used anywhere?

217–220 ↗(On Diff #27714)

Shouldn't this be called when the message edit window is NOT overflowed?
I think the code is right, just the function name makes it seem like this is the case when it does overflow

221–222 ↗(On Diff #27714)

Shouldn't these be in reverse order? Can't the scrollIntoView finish before the state is set and it will result in callback not being called? Or actually shouldn't scrollIntoView be set in a callback in setState to ensure the state is set before we scroll?

247 ↗(On Diff #27714)

Maybe return messageBottom > containerBottom || messageTop < containerTop so that the function name makes sense?

web/utils/tooltip-action-utils.js
236–238 ↗(On Diff #27714)
kuba marked 4 inline comments as done.

Added dynamic maximum height, addressed review

After a discussion with @inka we decided to add dynamic height calculation. Now, the maximum height of the edit box will be adjusted to the container height. We will scroll to the edited message only when there is insufficient space for the 150px text area in the edit box.

Video showing how it works:

web/chat/chat-message-list.react.js
224–225 ↗(On Diff #27724)

This is the same as what scrollingEndCallbackWrapper returns, can we not duplicate this?

228 ↗(On Diff #27724)

Is this still necessary?

238 ↗(On Diff #27724)

I feel like this might be confusing to other people who will look at this file. Can we write a short comment on why this is here?
Or extract some code from onScroll to an informatively named function

283 ↗(On Diff #27724)

Shouldn't this use the value from getMaxEditTextAreaHeight?

kuba marked 3 inline comments as done.

Address review

web/chat/chat-message-list.react.js
283 ↗(On Diff #27724)

I don't think so. Currently, the idea behind this is:

  • here we check if we have enough space for a minimum 150px text area box,
  • if there is not -> we want to scroll to the center.

The getMaxEditTextAreaHeight function is to determine the actual maximum TextArea height in two cases:

  • if the message in the first scenario will not overflow (there is enough place for 150px textarea),
  • if the message will overflow: then we scroll to the center, and we need to calculate the new maximum space (it might be greater than 150px).

Address @inka comments, disable anchor-overflow when in edit mode

web/chat/chat-message-list.react.js
317–318

We need to disable overflow-anchor for browsers that support it. Otherwise, when the edit box appeared, it jumped outside the message list container.

web/chat/chat-message-list.react.js
237
This revision is now accepted and ready to land.Jun 22 2023, 3:24 AM
ashoat requested changes to this revision.Jun 28 2023, 4:40 PM

High-level approach makes sense, but it's unfortunate that we're complicating a component that is already too complicated. Feedback below should be relatively easy to address

web/chat/chat-message-list.react.js
348 ↗(On Diff #28135)

You are introducing a lot of complicated code to a component that is already complicated. There should be extra attention paid to readability. If somebody reads onScroll on its own (without reading the whole component), they will not understand what you are doing here.

Can you please rename this function to explain what it is doing? Eg. debounceEditModeAfterScrollToMessage or something

351 ↗(On Diff #28135)

You appear to be implementing debounce from scratch. Can you instead use the _debounce utility from Lodash that we use elsewhere in the codebase?

web/chat/composed-message.react.js
195 ↗(On Diff #28135)

I think we should use a more specific id, eg. ComposedMessageBox${messageKey(item.messageInfo)} or something

web/chat/edit-message-provider.js
145–159 ↗(On Diff #28135)

setState can take a function, which allows us to avoid regenerating these every time scrollToMessageCallbacks changes

web/chat/edit-text-message.react.js
31–32 ↗(On Diff #28135)

You are creating a contract for developers to maintain these numbers. It is very likely this contract will be broken in the future, as it's not documented and not discoverable.

Ideally we can enforce that these numbers are correct somehow, by using them directly where the margins / height is defined.

If you are sure we cannot do that, then the next best option is to add code comments at both locations to make sure that if somebody changes one place, they know that they need to change the other.

This revision now requires changes to proceed.Jun 28 2023, 4:40 PM
ashoat added inline comments.
web/chat/chat-message-list.react.js
351 ↗(On Diff #28135)

@kamil recently implemented a debounce in D8294, so he might be a good person to ask about this

kuba marked 5 inline comments as done.

Address review

web/chat/edit-text-message.react.js
31–32 ↗(On Diff #28135)

The problem here is that this height is the sum of:

  • main padding from .editMessage (2x 10px),
  • bottom row height (buttons) (22px):
    • padding (2x 3px),
    • the font size (12px),
    • borderWidth (2x 2px),
  • padding between bottom row (buttons) and textarea .bottomRow (10px),
  • textarea padding (2x 8px - used to be 4x, comment below).

I can force the bottom height to be exactly 22px, but forcing the overall height might be cumbersome, especially, because it is used only for calculations.

I can either:

  • make the editBoxHeight a sum of the above (I would need to extract the paddings/margins from CSS files into some constants),
  • add the comments to each place (buttons and all paddings).

For now, I went for a combination of two (enforced bottom row height, and added comments for the rest), but if needed I can change it.

31–32 ↗(On Diff #28135)

I found out that I added the 8px padding two times:

Screenshot 2023-07-03 at 16.51.02.png (246×1 px, 21 KB)

I removed the redundant div.inputBarTextInput from the edit-text-message.react.js:

Screenshot 2023-07-03 at 16.52.40.png (238×1 px, 21 KB)

ashoat requested changes to this revision.Jul 4 2023, 11:34 AM

Thanks for the update! This is really close, but going to request changes one last time due to the number of comments.

The biggest feedback is to fix Fast Refresh by moving the non-React exports to a separate file from the React exports. Two candidate files might be web/chat/chat-constants.js or web/chat/message-list-types.js, or you can create a new one if you'd like.

web/chat/chat-input-text-area.react.js
18 ↗(On Diff #28356)

It breaks Fast Refresh in React Native when you export a constant from the same file as a React component – see here:

For example, maybe your component also exports a constant, and a non-React utility module imports it. In that case, consider migrating the constant to a separate file and importing it into both files. This will re-enable Fast Refresh to work.

Sorry I didn't point this out earlier!

web/chat/chat-message-list.react.js
6 ↗(On Diff #28356)

We have a convention in our codebase to import only the parts of Lodash that we need:

import _debounce from 'lodash/debounce.js';
web/chat/composed-message.react.js
30–34 ↗(On Diff #28356)

I am not sure, but I think this will break Fast Refresh too since it's a "non-React component" being exported from a file that also exports a React component. Can you move it out to another file?

web/chat/edit-text-message.react.js
31–32 ↗(On Diff #28135)

Thanks for explaining! This seems reasonable to me

32–33 ↗(On Diff #28356)

Same feedback about exports and Fast Refresh

157 ↗(On Diff #28356)

The object being passed to style will get recreated on every render. Instead, we can define it just once at the top-level scope

This revision now requires changes to proceed.Jul 4 2023, 11:34 AM
kuba marked 5 inline comments as done.

Move constants to seperate file

The biggest feedback is to fix Fast Refresh by moving the non-React exports to a separate file from the React exports. Two candidate files might be web/chat/chat-constants.js or web/chat/message-list-types.js, or you can create a new one if you'd like.

I think there are more places where this issue is present. I created a task https://linear.app/comm/issue/ENG-4309/search-for-places-where-fast-refresh-is-broken-and-fix-it to fix them.

Looks good, but I want to make sure that constants are kept in sync with CSS. I don't see any comments.

Would normally request changes here, but I really want to unblock @kuba, so I will accept. @kuba, please make sure that you address the inline comments! I'm guessing you need to add some additional code comments.

web/chat/chat-constants.js
40–45 ↗(On Diff #28392)

Do any of these constants need to be kept "in sync" with some place in CSS (or otherwise)?

If yes, can you:

  1. Add comments here explaining what needs to be kept in sync, and with what place
  2. Add comments in the CSS explaining the same, but in the other direction
web/chat/chat-message-list.react.js
34 ↗(On Diff #28392)

This constant appears to only be used in this file.

Can you share some context on why it's defined elsewhere, instead of being defined here?

If the constant needs to be kept in sync with some location, we should add a comments on both sides explaining that.

This revision is now accepted and ready to land.Jul 5 2023, 10:07 AM
kuba marked 2 inline comments as done.

Added comments

ashoat added inline comments.
web/chat/chat-message-list.react.js
49 ↗(On Diff #28431)

Can you clarify whether or not this constant needs to be kept in sync with some other location, such as CSS?

If so, please do two things:

  1. Add a comment here explicitly mentioning the location of the CSS that must be kept in sync
  2. Add a comment to the CSS explicitly mentioning that this location must be kept in sync
web/components/button.css
12–13 ↗(On Diff #28431)

Please keep all lines to a max of 80 chars

kuba marked 2 inline comments as done.

Fix comments

web/chat/chat-message-list.react.js
49 ↗(On Diff #28431)

The idea behind editBoxTopMargin is to how much space should we leave between the top of the edit box (with maximum height), and the top of the container (screenshot). In that way, it has a 10px margin, so it looks much better.

Screenshot 2023-07-11 at 13.33.27.png (792×1 px, 43 KB)

I believe that it doesn't need to be kept in sync with any CSS.

web/chat/chat-message-list.react.js
49 ↗(On Diff #28431)

Thanks for the clarification! Heads-up that the image doesn't seem to have successfully attached

kuba added inline comments.
web/chat/chat-message-list.react.js
49 ↗(On Diff #28431)

I attached the image, hope it works now