WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 8 months ago

#40242 closed defect (bug) (fixed)

Bulk edit tag autocomplete layout error

Reported by: davidbenton Owned by: afercia
Milestone: 4.7.4 Priority: normal
Severity: normal Version: 4.7
Component: Quick/Bulk Edit Keywords: has-screenshots has-patch commit fixed-major
Focuses: ui, javascript Cc:

Description

When bulk editing tags (or any taxonomy terms), the autocomplete suggestions <ul> is positioned correctly when it is first opened, but closing and reopening the bulk edit form (by using the "Cancel" button, for example) causes the term autocomplete list to be positioned incorrectly. See image:
https://www.dropbox.com/s/c5iragizvk3wak3/Screenshot%202017-03-23%2012.46.19.png?dl=0

I was able to consistently reproduce this in latest Chrome and Safari on OS X for v4.7.2 and v4.7.3.

Attachments (3)

40242.patch (416 bytes) - added by davidbenton 8 months ago.
40242.diff (440 bytes) - added by afercia 8 months ago.
40242.2.diff (1.2 KB) - added by afercia 8 months ago.

Download all attachments as: .zip

Change History (20)

#1 @afercia
8 months ago

  • Focuses javascript added; administration removed
  • Keywords has-screenshots added
  • Milestone changed from Awaiting Review to 4.7.4
  • Version changed from 4.7.2 to 4.7

@thanks @davidbenton, I was able to reproduce it in Firefox too, seems something related to jQuery ui autocomplete not being able to correctly calculate the left position when the Quick Edit form gets closed and then re-opened. Introduced in 4.6 see #33902. Would be nice to try to fix it in the next minor release, but I will leave the decision to the release lead of course.

Re-posting your screenshot to make it immediately visible:

https://cldup.com/Cm_T2lEgER.png

#2 @davidbenton
8 months ago

Changing tags-suggest.js near line 153 from:

			position: {
				my: 'left top+2'
			},

to...

			position: {
				my: 'left top+2',
				at: 'left bottom'
			},

...solves for me. I'll work on submitting a patch, but I'm new around here so it may take a bit for me to figure out the right way to do that, and I wanted to go ahead and post a potential solution.

@davidbenton
8 months ago

#3 @davidbenton
8 months ago

  • Keywords has-patch needs-testing added

Man, it's been a while since I used svn... hope that patch is right. :) I've tested the functionality in a couple browsers, and jQuery UI's docs on this seem straightforward, but would love feedback from someone who has more context for WP admin's inner workings.

#4 @afercia
8 months ago

@davidbenton yep same idea here, ensuring the defaults are always passed fixes it for me. Not sure why it doesn't happen for quick edit though (still have to dig into that part). I'd just make sure to pass also collision.

@afercia
8 months ago

#5 @afercia
8 months ago

Dumping the options object in wp-includes/js/jquery/ui/position.js right after options = $.extend( {}, options ); highlights that, the second time Bulk Edit is open, the default are not extended but overridden instead.

https://cldup.com/f9GT4nUSsK.png

#6 @afercia
8 months ago

To clarify, jQuery UI autocomplete uses the following defaults (which are different from the jQuery UI position defaults):

position: {
	my: "left top",
	at: "left bottom",
	collision: "none"
},

This ticket was mentioned in Slack in #core-editor by afercia. View the logs.


8 months ago

#8 follow-up: @afercia
8 months ago

  • Keywords commit added; needs-testing removed

Tested a bit more and I think the different behaviour between Bulk Edit (where the misplacing occurs) and Quick Edit (which works ok) depends on a mix of factors.
Both Quick Edit and Bulk Edit use a hidden form that then gets revealed. By the way, the Quick Edit one gets cloned each time with jQuery clone() so it's a new object each time while the Bulk Edit one gets just moved through the DOM. I guess the Quick Edit autocomplete widget is always a new fresh instance and thus its options are properly extended, while the Bulk Edit autocomplete widget gets its options overridden the second time it gets used.
Regardless, passing always a complete position object solves the positioning issue. Patch looks good to me. @davidbenton any thoughts? :)

#9 @afercia
8 months ago

  • Owner set to afercia
  • Status changed from new to assigned

#10 follow-up: @davidbenton
8 months ago

Yep, looks good to me @afercia, and works for me.

Thanks for digging deeper. Interesting about the differences between Quick Edit and Bulk Edit. Almost sounds like it may be a jQuery UI issue, or do you think Bulk Edit is using it in an unusual way? I will try to take a look at that myself, but probably won't be today.

I think the issue as described in the ticket is fixed by the patch, though, so as far as I'm concerned this can move forward as-is. Thanks!

#11 in reply to: ↑ 8 @azaozz
8 months ago

Replying to afercia:

40242.diff works properly and looks good here too.

#12 in reply to: ↑ 10 @afercia
8 months ago

Replying to davidbenton:

Almost sounds like it may be a jQuery UI issue, or do you think Bulk Edit is using it in an unusual way?

Maybe a mix of both :) Yes, autocomplete here is used in a particular way. While Quick Edit uses each time a new fresh autocomplete instance, Bulk Edit re-uses the same instance.

On the other hand what autocomplete does when the suggestions get displayed doesn't work for some edge cases. It seems to me it defeats the purpose of extending the defaults with the passed options, since the default position object there is incomplete (there's just the of property). This could be confusing for developers because when using .extend() we should safely assume there are always some proper defaults, even when using autocomplete in a particular way.

Maybe worth considering to check if an autocomplete widget instance already exists and avoid calling again wpTagsSuggest() in that case. /cc @azaozz

@afercia
8 months ago

#13 follow-up: @afercia
8 months ago

While 40242.diff works, 40242.2.diff also avoids to call again wpTagsSuggest() when an autocomplete widget instance already exists for the Bulk Edit textarea. This makes passing the at and collision properties not strictly necessary but I'd say to keep them, just in case...

#14 in reply to: ↑ 13 @davidbenton
8 months ago

I found a jQuery UI ticket addressing this, and it was closed as "not a bug." So, while I find that behavior less than intuitive, I guess it is as intended by jQuery UI.

Replying to afercia:

40242.2.diff [...] makes passing the at and collision properties not strictly necessary but I'd say to keep them, just in case...

I think that's reasonable. It makes the position more explicit and readable, which I'd consider valuable even if those are the defaults, especially given the potential confusion of different defaults for jQuery UI and jQuery UI autocomplete.

#15 @afercia
8 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 40357:

Quick/Bulk Edit: Fix the Tag suggestions position on the Bulk Edit textarea.

Always passes the complete position object to the jQuery autocomplete widget.
Also checks if an autocomplete instance already exists on the Bulk Edit textarea.

Props davidbenton.
Fixes #40242.

#16 @afercia
8 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for 4.7.4 consideration.

#17 @swissspidy
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 40365:

Quick/Bulk Edit: Fix the Tag suggestions position on the Bulk Edit textarea.

Always passes the complete position object to the jQuery autocomplete widget.
Also checks if an autocomplete instance already exists on the Bulk Edit textarea.

Props davidbenton.
Fixes #40242.

Merges [40357] to the 4.7 branch.

Note: See TracTickets for help on using tickets.