Make WordPress Core

Opened 11 years ago

Closed 10 years ago

#26061 closed defect (bug) (fixed)

Customizer settings with non-scalar values incorrectly trigger as changed

Reported by: westonruter's profile westonruter Owned by: ocean90's profile ocean90
Milestone: 4.1 Priority: normal
Severity: normal Version: 3.4
Component: Customize Keywords: has-patch
Focuses: Cc:

Description

When a value is updated in the customizer models, it checks to see if the new value equals the old value, and if it does, then it short-circuits so as to not trigger the change callbacks if the new value is not different. This is working fine for scalar values (e.g. strings, booleans, numbers). However, for objects the test fails because two JavaScript object instances with the same contents are still different instances, and so they are not identical.

The current equality test in customize.Value is:

if ( null === to || this._value === to )

But if if the old value was an object {"title":"Foo"} and the new value is {"title":"Foo"}, then this conditional will fail because in JavaScript: {"title":"Foo"} !== {"title":"Foo"}. The result is that the value change callbacks will fire even the value didn't change, including when the customizer preview first loads as the settings get synced over.

Fortunately, Underscore.js provides an isEqual function to compare two values to see if they have the same contents, and so we can use this instead:

if ( null === to || _.isEqual( from, to ) ) {

Introduced in #19910 via r19995

This bug was discovered in the course of developing the Widget Customizer plugin, as each widget instance is stored in a customizer setting, and these instances are JavaScript objects.

Attachments (4)

26061.patch (1.4 KB) - added by westonruter 11 years ago.
26061.2.patch (2.5 KB) - added by ocean90 10 years ago.
26061.jshint.patch (760 bytes) - added by atimmer 10 years ago.
26061.3.diff (1.4 KB) - added by celloexpressions 10 years ago.

Download all attachments as: .zip

Change History (26)

@westonruter
11 years ago

#1 @westonruter
11 years ago

  • Cc weston@… added
  • Summary changed from Customizer setting with array values incorrectly trigger as changed to Customizer setting with non-scalar values incorrectly trigger as changed

#2 @westonruter
11 years ago

  • Summary changed from Customizer setting with non-scalar values incorrectly trigger as changed to Customizer settings with non-scalar values incorrectly trigger as changed

#3 @SergeyBiryukov
11 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.8

#4 @ocean90
10 years ago

@westonruter Could you please provide a simple demo plugin which currently fails?

I'm not sure if we should enqueue Underscore.js for just one method.

#5 @westonruter
10 years ago

@ocean90: here is a quick and dirty demo plugin: https://gist.github.com/westonruter/7729203

You'll notice that if you hit the Update button in the control, that that the preview reloads and the customizer's state changes to unsaved, even though none of the setting's properties were changed.

As for not enqueueing all of Underscore.js for this one function <http://underscorejs.org/docs/underscore.html#section-84>, we could copy it out and include it in customize-base.js, maybe as api.Value.isEqual( to )

@ocean90
10 years ago

#6 @ocean90
10 years ago

Thanks westonruter.

26061.2.patch includes the copy of eq from Underscore.js. Seems to work.

#7 @westonruter
10 years ago

@ocean90: Looks perfect!

#8 @ocean90
10 years ago

  • Owner set to ocean90
  • Resolution set to fixed
  • Status changed from new to closed

In 26548:

Customizer: Don't trigger a change event if two unchanged object values are equal.

In JavaScript: {"title":"Foo"} !== {"title":"Foo"}
Underscore.js provides an isEqual function to compare two values to see if they have the same contents, which can be use instead.
Because only one function of Underscore.js is needed we just add a copy of _.isEqual to customize-base.js.

props westonruter.
fixes #26061.

#9 @JustinSainton
10 years ago

I'm more than happy to be the only one who thinks this...but...

Doesn't it seem kind of lame to copy massive chunks of Underscores.js? Would it be so bad to just have it globally enqueued, like jQuery (except jQuery isn't enqueued on the front-end, but you get my point)?

Like I said, happy to be alone on this, but I think having Underscores.js available everywhere would actually be a pretty huge win.

#10 @JustinSainton
10 years ago

Last edited 10 years ago by JustinSainton (previous) (diff)

#11 @atimmer
10 years ago

[26548] introduced a jshint error, 26061.jshint.patch fixes it.

#12 @SergeyBiryukov
10 years ago

In 26632:

Fix JSHint error. props atimmer. see #26061.

#13 @nacin
10 years ago

In 26702:

Customizer: Revert [26548], removing _.isEqual() for proper object comparison.

This broke the customizer in IE 11, with possibly other side effects. Revisit in 3.9.

Also reverts [26632].

see #26061 (reopens), fixes #26438.

#14 @nacin
10 years ago

  • Keywords needs-patch added; has-patch commit removed
  • Milestone changed from 3.8 to Future Release
  • Resolution fixed deleted
  • Status changed from closed to reopened

#15 @westonruter
10 years ago

@nacin Do we have details on what broke specifically in IE 11? If isEqual broke the Customizer, then perhaps the other instances of _.isEqual() are breaking IE 11 too. For instance:

src/wp-includes/js/media-models.js
65:		if ( _.isEqual( a, b ) )
502:				if ( ! _.isEqual( attachment.attributes, newAttributes ) )
783:					return _.isEqual( query.args, args );

src/wp-includes/js/shortcode.js
219:		} else if ( _.isEqual( _.keys( attrs ), [ 'named', 'numeric' ] ) ) {

#16 @nacin
10 years ago

ocean90 tested media and it appeared to work OK. Possibly because of the data types we were using with it. There's more of an explanation for what broke in #26438.

#17 @SergeyBiryukov
10 years ago

  • Milestone changed from Future Release to 3.9

According to the comments on #26438, updating Underscore.js to 1.5.2 would fix this.

#18 follow-up: @ocean90
10 years ago

According to the comments on #26438, updating Underscore.js to 1.5.2 would fix this.

Not sure, since the included part was from 1.5.2.

#19 in reply to: ↑ 18 @SergeyBiryukov
10 years ago

Replying to ocean90:

Not sure, since the included part was from 1.5.2.

I see, I misread the comment in ticket:26438:26438.patch. Thanks for the clarification.

#20 @markjaquith
10 years ago

  • Milestone changed from 3.9 to Awaiting Review

Need more information about the IE 11 issue.

#21 @celloexpressions
10 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 4.1

I believe we're nearly always enqueuing Underscore.js in the Customizer now for Widgets (along with Backbone.js), so we might as well use that version of isEqual here (#29572 also requires Underscore.js, via wp-util.js, which is being enqueued already anyway).

26061.3.diff ended up just being a refresh of westonruter's original patch, and doesn't break IE11 for whatever reason. If we get this in soon someone'll definitely run into any bugs, but I'm guessing this should work fine now.

#22 @ocean90
10 years ago

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

In 29907:

Customizer: Don't trigger a change event if two unchanged object values are equal, second pass.

Make Underscore.js a dependency for customize-base and use _.isEqual() to compare the values.
(Underscore.js was already enqueued via wp-util.js for Widgets.)

props westonruter.
fixes #26061.

Note: See TracTickets for help on using tickets.