WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#17004 closed defect (bug) (fixed)

Apostrophe in first/last/nickname causes JS error on user profile page

Reported by: johnbillion Owned by:
Milestone: 3.1.4 Priority: normal
Severity: minor Version: 3.0
Component: Administration Keywords: has-patch needs-testing fixed-major
Focuses: Cc:

Description

If you have an apostrophe in your first name, last name or nickname then tabbing between (or focusing/blurring) those fields on your user profile page throws a JavaScript error. The result is that the dropdown menu for selecting your display name doesn't get populated correctly.

JS error seen in Firefox 3.6:

uncaught exception: Syntax error, unrecognised expression: value=O'Hare]

Looks like the expression needs to be escaped and/or enclosed in quotes.

Attachments (4)

17004.diff (720 bytes) - added by andrewryno 3 years ago.
Convert single quotes to HTML entity
17004.2.diff (6.8 KB) - added by jacobwg 3 years ago.
Patch to fix quoting of jQuery attribute selectors for the Display Name drop down in user profile
17004.3.diff (2.8 KB) - added by andrewryno 3 years ago.
17004-4.patch (3.1 KB) - added by azaozz 3 years ago.

Download all attachments as: .zip

Change History (24)

andrewryno3 years ago

Convert single quotes to HTML entity

comment:1 andrewryno3 years ago

Didn't seem like it would be possible to just change the single quotes in the jQuery selector/function to double quotes because either way it would screw up with jQuery or the HTML. So just converted the quote to an HTML entity. Not sure if it needs to be done for the first name as well.

jacobwg3 years ago

Patch to fix quoting of jQuery attribute selectors for the Display Name drop down in user profile

comment:2 jacobwg3 years ago

  • Cc jacobwg added
  • Keywords has-patch added

According to http://api.jquery.com/category/selectors/, there are certain characters that need to be escaped in the selector:

!"#$%&'()*+,./:;<=>?@[\]^`{|}~

And according to http://api.jquery.com/attribute-equals-selector/, attribute equals selectors must always be quoted:

// wrong:
[attr=value]
// right:
[attr="value"]

Also, .children() does some special stuff that breaks when you have escaped characters in your selector, so all .children() calls were replaced with .find() calls.

My patch should address these issues.

comment:3 nacin3 years ago

Probably happened in [13539]. What happens if " is used in a name?

comment:4 andrewryno3 years ago

The patch by jacobwg should take care of any special characters inserted into a name. Mine would only take care of single quotes.

comment:5 nacin3 years ago

  • Milestone changed from Awaiting Review to 3.2
  • Version changed from 3.1 to 3.0

comment:6 azaozz3 years ago

17004.2.diff looks good, I was with the impression that similar functionality already was in jQuery. Maybe consider opening an enhancement ticket on jQuery's trac, it would be useful to have a native way of escaping selectors as that's a limitation in jQuery (in HTML option value can contain CDATA).

However I think #10030 and [13539] that brought this issue in the first place can be optimized a bit. Perhaps that block of code can be simplified to compare the user entered values instead of repeatedly checking for duplicates in the already created <option value="...">.

Version 0, edited 3 years ago by azaozz (next)

comment:7 follow-up: andrewryno3 years ago

I'm working on trying to get it to only compare user inputs. The problem is that if the user leaves one of the fields blank, it's hard to compare the inputs because you would need a lot more comparisons to account for blank fields. Also if someone puts in a nickname that is the same as either the first and last or last and first combination. I honestly think that the way it's done now will be the best it could be done, unfortunately. Someone feel free to correct me, however.

comment:8 in reply to: ↑ 7 johnbillion3 years ago

Replying to andrewryno:

I'm working on trying to get it to only compare user inputs. The problem is that if the user leaves one of the fields blank, it's hard to compare the inputs because you would need a lot more comparisons to account for blank fields. Also if someone puts in a nickname that is the same as either the first and last or last and first combination. I honestly think that the way it's done now will be the best it could be done, unfortunately. Someone feel free to correct me, however.

Personally I wouldn't spend time on this. It works as it is and you'd just be refactoring code on a feature that's rarely used more than once or twice. We should just fix the JS error and we'll be done :)

andrewryno3 years ago

comment:9 follow-up: andrewryno3 years ago

  • Keywords needs-testing added

I spoke too soon on my previous comment. Worked it a little more and got the above patch. Works well from my testing and doesn't require any regex or much traversing of the DOM. Just builds an array and deletes duplicate values. Definitely needs some testing just to ensure that it works in all cases.

azaozz3 years ago

comment:10 in reply to: ↑ 9 ; follow-up: azaozz3 years ago

Replying to andrewryno:

Yes, that's what I had in mind. However think instead of array we should use a simple object (associative array) there. It preserves the IDs and the currently selected option in the drop-down like in 17004-4.patch.

@johnbillion don't think this is a "real" refactoring. We already have the logic for this in the PHP version, just needed to use the same logic in the JS.

comment:11 in reply to: ↑ 10 andrewryno3 years ago

Replying to azaozz:

+1 on your patch. I didn't even think about using an object there. I knew I couldn't do a true associative array and I don't do too much features of JS to think about using an object.

comment:12 follow-up: nacin3 years ago

Would not mind seeing this fix in a point release given it's a recent regression.

comment:13 azaozz3 years ago

(In [17587]) Fix display of apostrophes in the user's first and last names on the User Profile page, partial props andrewryno, see #17004

comment:14 in reply to: ↑ 12 azaozz3 years ago

Replying to nacin:

Agreed. Just would need a bit more testing especially in non-Roman and RTL languages.

Removed the option values from the patch: this only concerns the browser and as we are capturing user input, there's no need to copy it there as well with JS. Also switched the places of username and nickname so we show nickname by default when no selection.

comment:15 nacin3 years ago

  • Milestone changed from 3.2 to 3.1.2

For review.

comment:16 nacin3 years ago

  • Keywords fixed-major added

The fixed-major keyword will hide this ticket from reports 5 and 6, as it is fixed for the major release.

comment:17 jacobwg3 years ago

Looks good... removes a lot of the regex stuff.

Sorry I haven't commented earlier... I though I would get an email notification for comments, but I guess you have to tell Trac what your email address is first, don't you? :)

Anyway, glad to see it fixed.

comment:18 nacin3 years ago

  • Milestone changed from 3.1.2 to 3.1.3

comment:19 SergeyBiryukov3 years ago

  • Milestone changed from 3.1.3 to 3.1.4
  • Resolution set to fixed
  • Status changed from new to closed

comment:20 downloadbook3 years ago

Nothing.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)
Note: See TracTickets for help on using tickets.