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

Fix nick change on Safari for Mac and iOS #1568

Merged
merged 1 commit into from Sep 27, 2017
Merged

Conversation

Gilles123
Copy link
Contributor

It is currently impossible to edit the nick text after pressing the edit button on Safari for iOS and Mac because user-select is set to None on Safari by default. This fixes that issue. Tested that it is not editable when it shouldn't be and that it doesn't break anything on Chrome or Firefox.

@@ -1496,6 +1496,8 @@ part/quit messages where we don't load previews (adds a blank line otherwise) */

[contenteditable]:focus {
outline: none;
-webkit-user-select: text;
user-select: text;
Copy link
Member

Choose a reason for hiding this comment

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

If this is the right fix, you'll want to use the full set of prefixes we use for example here:

-webkit-user-select: text;
-moz-user-select: text;
-ms-user-select: text;
user-select: text;

While you're at it, could you add cursor: text; please? Thanks!

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 didn't include those because as far as I can tell, only WebKit/Safari uses "none" as its default and the other browsers worked as expected. For certainty's sake, we can do that though.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's be consistent, and whenever we tackle #1551, we'll let the auto-prefixer get on with it :)

@astorije
Copy link
Member

Thanks for this, @Gilles123!

I know this is broken in v2.4.0 as well, but I'm marking this for v2.5.0 milestone, because fix seems easy, and the bug is bad enough :)

@MaxLeiter, could you give this a go please?

@astorije astorije added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Sep 25, 2017
@astorije astorije added this to the 2.5.0 milestone Sep 25, 2017
@xPaw
Copy link
Member

xPaw commented Sep 25, 2017

Shouldn't this be set on [contenteditable] without :focus?

@Gilles123
Copy link
Contributor Author

It works either way. I changed it to be without focus.

@xPaw
Copy link
Member

xPaw commented Sep 25, 2017

Approved, but can you squash your commits?

@MaxLeiter
Copy link
Member

Works for me 👍

Copy link
Member

@astorije astorije left a comment

Choose a reason for hiding this comment

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

Tested 👌 on Safari, great job @Gilles123!
One comment before we get this in :)

@@ -1498,6 +1498,14 @@ part/quit messages where we don't load previews (adds a blank line otherwise) */
outline: none;
}

[contenteditable] {
Copy link
Member

Choose a reason for hiding this comment

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

Actually, make this [contenteditable="true"] so it's only selectable + correct cursor in edit mode.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's necessary in lounge?

Copy link
Member

Choose a reason for hiding this comment

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

Why not? That's what we do everywhere else in the app except messages, i.e. UI not selectable. With this addition, it stays like that, except when the nick is being edited, when it can be selected and have the text cursor. Makes sense?

It is currently impossible to edit the nick text after pressing the edit button on Safari for iOS and Mac because user-select is set to None on Safari by default. This fixes that issue. Tested that it is not editable when it shouldn't be and that it doesn't break anything on Chrome or Firefox.
Copy link
Member

@astorije astorije left a comment

Choose a reason for hiding this comment

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

@Gilles123, I just made the suggested change + squashed commits as we would like to draft a pre-release right now. Happy to circle back with you to share what I did to get there if you want :)

Either way, great stuff and thanks for catching this!

@xPaw xPaw merged commit 0b16026 into thelounge:master Sep 27, 2017
@astorije
Copy link
Member

(Hey @Gilles123, do you happen to have a Twitter account to give you a shout-out after we release v2.5.0? 🙂)

@astorije
Copy link
Member

Hey @Gilles123, we have free sticker packs for our contributors now!
If you're interested, go fill the form at https://goo.gl/forms/f5usqAEp5DWoeXC92 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants