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.

Sign up for GitHub

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

Fix for: Web - Triple clicking an edited message select’s the (edited) label along with the actual message #17704

Merged
merged 13 commits into from
May 2, 2023
+28 −15
Merged

Fix for: Web - Triple clicking an edited message select’s the (edited) label along with the actual message #17704

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0951986
Mirror text layout from EditedRenderer
Ollyws Apr 20, 2023
861fc7c
Added dInlineFlex style
Ollyws Apr 20, 2023
18274bd
Add alignItemsBaseline,dInlineFlex to edited label
Ollyws Apr 20, 2023
93f3662
Moved MiniReportActionContextMenu to top of view
Ollyws Apr 20, 2023
45646d5
Removed userSelectNone from emoji text
Ollyws Apr 20, 2023
2619a9b
Merge branch 'main' into fix-15194
Ollyws Apr 21, 2023
db83d74
Added dInlineFlex index/index.native
Ollyws Apr 22, 2023
06b6454
Merge branch 'main' into fix-15194
Ollyws Apr 28, 2023
fcf4630
Added editedLabelStyles
Ollyws May 1, 2023
a994917
Revert remove userSelectNone
Ollyws May 2, 2023
133d4d5
Revert move MiniReportActionContextMenu
Ollyws May 2, 2023
7633f88
Move MiniReportActionContextMenu to top of view
Ollyws May 2, 2023
44bbe3c
Added userSelectNone to space Text
Ollyws May 2, 2023
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
1 change: 1 addition & 0 deletions src/components/HTMLEngineProvider/HTMLRenderers/EditedRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const EditedRenderer = (props) => {
{...defaultRendererProps}
fontSize={variables.fontSizeSmall}
color={themeColors.textSupporting}
style={[styles.alignItemsBaseline, styles.dInlineFlex]}
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that this is a Text Component where we are applying inline flex.

Copy link
Contributor Author

@Ollyws Ollyws Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@parasharrajat Yes, is this an issue?

Copy link
Member

Choose a reason for hiding this comment

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

Not yet.

>
{/* Native devices do not support margin between nested text */}
<Text style={styles.w1}>{' '}</Text>
Expand Down
1 change: 0 additions & 1 deletion src/components/Reactions/EmojiReactionBubble.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ const EmojiReactionBubble = (props) => {
>
<Text style={[
styles.emojiReactionBubbleText,
styles.userSelectNone,
Copy link
Member

Choose a reason for hiding this comment

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

Checking on it where it was added originally.

Copy link
Member

Choose a reason for hiding this comment

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

@Ollyws Which part does this fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat it fixes copying the emoji reaction.

Copy link
Member

Choose a reason for hiding this comment

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

Which one is Context or Underneath the message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat Underneath the message.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Ollyws Ah I see, In case we don’t want the emojis to be selectable or copiable , this PR #18228 will handle that case.

Copy link
Member

Choose a reason for hiding this comment

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

It means that this change will be against #18228 as that PR uses userSelect:none.

Copy link
Member

Choose a reason for hiding this comment

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

I think this change is not needed for the current issue and is delaying the PR. I don't want anyone to be penalized for figuring this out on this PR.

So should we remove this change and merge as it is @marcochavezf?

Copy link
Contributor Author

@Ollyws Ollyws May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@parasharrajat It seems #18228 will fix the problem that us removing userSelectNone is meant to solve. So it makes sense to revert the removal of userSelectNone.
Also changing the position of the context-menu will also be unnecessary, that PR fixes all of it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes please revert it. @Ollyws

StyleUtils.getEmojiReactionBubbleTextStyle(props.isContextMenu),
]}
>
Expand Down
25 changes: 12 additions & 13 deletions src/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,18 @@ class ReportActionItem extends Component {
<Hoverable>
{hovered => (
<View accessibilityLabel={this.props.translate('accessibilityHints.chatMessage')}>
<MiniReportActionContextMenu
reportID={this.props.report.reportID}
reportAction={this.props.action}
isArchivedRoom={ReportUtils.isArchivedRoom(this.props.report)}
displayAsGroup={this.props.displayAsGroup}
isVisible={
hovered
&& !this.props.draftMessage
}
draftMessage={this.props.draftMessage}
isChronosReport={ReportUtils.chatIncludesChronos(this.props.report)}
/>
Comment on lines +278 to +289
Copy link
Member

Choose a reason for hiding this comment

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

Why the refactor?

Copy link
Contributor Author

@Ollyws Ollyws Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Moving this to the top of the view changes its position in the DOM to be first (which makes sense since visually it is at the top). This means it won't be selected on triple-click as it is no longer after the (edited) label. This resolves the MiniReportActionContextMenu part of #15810

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with it as it does not logically change anything.

{this.props.shouldDisplayNewMarker && (
<UnreadActionIndicator reportActionID={this.props.action.reportActionID} />
)}
Expand Down Expand Up @@ -336,19 +348,6 @@ class ReportActionItem extends Component {
)}
</OfflineWithFeedback>
</View>
<MiniReportActionContextMenu
reportID={this.props.report.reportID}
reportAction={this.props.action}
isArchivedRoom={ReportUtils.isArchivedRoom(this.props.report)}
displayAsGroup={this.props.displayAsGroup}
isVisible={
hovered
&& !this.props.draftMessage
&& !hasErrors
}
draftMessage={this.props.draftMessage}
isChronosReport={ReportUtils.chatIncludesChronos(this.props.report)}
/>
</View>
)}
</Hoverable>
Expand Down
4 changes: 3 additions & 1 deletion src/pages/home/report/ReportActionItemFragment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,10 @@ const ReportActionItemFragment = (props) => {
<Text
fontSize={variables.fontSizeSmall}
color={themeColors.textSupporting}
style={[styles.alignItemsBaseline, styles.dInlineFlex]}
>
{` ${props.translate('reportActionCompose.edited')}`}
<Text style={styles.w1}>{' '}</Text>
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member

@parasharrajat parasharrajat Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see same as EditedRenderer. Correct?

Copy link
Contributor Author

@Ollyws Ollyws Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@parasharrajat This is necessary for our solution to work on Firefox and also fixes this problem with overflow.
Yes, correct the same code as EditedRenderer.

Copy link
Member

Choose a reason for hiding this comment

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

How does this work?

Copy link
Contributor Author

@Ollyws Ollyws Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the way Firefox deals with selection using flex, having a <span> between the two pieces of text stops them from both being selected at once.
It's a similar principle that causes them to break onto a second line properly.

Copy link
Member

@parasharrajat parasharrajat May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This has a drawback where the edited label does not start from the start of the line.
Screenshot 2023-05-01 at 8 55 35 PM

Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts?

Copy link
Contributor Author

@Ollyws Ollyws May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@parasharrajat I'm having a look at it, but I'm not sure there is any easy solution. Do you think it is a big issue?
We could do something like setting the space width to 0px and adding margin-right on the preceding text, but that changes where the preceding text breaks.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think @marcochavezf?

Copy link
Member

Choose a reason for hiding this comment

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

Another issue with this change is that this space is selectable via triple click.

  • triple click on only the space and notice that it is selectable.

Can we just replace it with a margin?

Copy link
Contributor Author

@Ollyws Ollyws May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@parasharrajat We could just add userSelectNone to prevent selection on triple click. Note: in this PR it will also copy the space but when #18228 is merged it shouldn't.
I'm concerned how removing the space would affect the original intention of adding it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's add that.

{props.translate('reportActionCompose.edited')}
</Text>
)}
</Text>
Expand Down
3 changes: 3 additions & 0 deletions src/styles/utilities/dInlineFlex/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default {
display: 'inline-flex',
};
1 change: 1 addition & 0 deletions src/styles/utilities/dInlineFlex/index.native.js
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

This is a bad example of platform file usage. Proper way would be only apply the inline flex on the web platforms and flex on native.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat Applying flex would break the layout:

flexbreak

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do also return empty for other styles such as overflowXHiden, pointerEventsNone, pointerEventsAuto, whiteSpace, wordBreak.
Any thoughts on this @marcochavezf ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting, I think this is fine since we're applying empty objects in other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat Given @marcochavezf thoughts, are we ok to leave this as it is?

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default {};
Copy link
Member

Choose a reason for hiding this comment

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

The problem with this code is inconsistency. It looks incorrect that inline-flex is null on native.

The proper way would be:

  1. Directly define this in the display.js as a new rule.
  2. Where we have to switch the styles based on the platform and create a new function that decides which styles go where.

So a new function|lib editedlabelStyles can be created.

Copy link
Contributor Author

@Ollyws Ollyws May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@parasharrajat Ok I see, could you point me at some code where we already do something like this so I have something to go on? I'm slightly confused about the implementation. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

You can do following

  1. move display:inline-flex to the display.js as a new rule.
  2. Create a new folder somewhere named editedlabelStyles. Created two child files index.native.js and index.js.
  3. export objects from these files. Add dInlineFlex to the index.js file.
  4. Now import and use this lib where needed.

4 changes: 4 additions & 0 deletions src/styles/utilities/display.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import dInlineFlex from './dInlineFlex';

/**
* Display utilities with Bootstrap inspired naming.
*
Expand All @@ -20,4 +22,6 @@ export default {
dInline: {
display: 'inline',
},

dInlineFlex,
};