Fix missing attributes when an option is chosen by default on variations. #12585

Merged
merged 14 commits into from Dec 15, 2016

Projects

None yet

5 participants

@mikejolley
Member
mikejolley commented Dec 13, 2016 edited

This is a workaround for #12582. I’ve targeted 2.6 branch as this is a fix, but this can be applied to master too.

b279727 fixed an issue where Firefox would lose the current selection on focusin, but removing that code caused the problem in #12582.

The reason it breaks is because the current script removes the options from the DOM then appends them back.

Clearly this is not something we can rely on being cross browser compatible any longer so I propose the following PR.

My PR uses the HTML5 HIDDEN attribute, with a fallback for the DISABLED attribute.

Browsers which support hidden (chrome, firefox, IE11+ apparently) hide the option as intended.

Browsers which do not support hidden on options (Safari) revert to disabled attributes.

No DOM changes are made. I feel this will be more reliable in the long run.

This now works with .HTML() and across all browsers!

@mikejolley mikejolley Fix missing attributes when an option is chosen by default on variati…
…ons.
36ecce6
@mikejolley
Member

@franticpsyx you may be interested in this one too.

@mikejolley mikejolley added this to the 2.6.x milestone Dec 13, 2016
@claudiosanches

👍

@franticpsyx
Member
franticpsyx commented Dec 13, 2016 edited

Some feedback:

(+)

  1. Resolves the reported issue.
  2. Resolves the problem with hidden placeholders reported in #12487 -- Before: https://cl.ly/0o3Z2d0h0u0j After: https://cl.ly/1P3r2O250d0O

(-)

My PR uses the HTML5 HIDDEN attribute, with a fallback for the DISABLED attribute.

  1. How widely supported is it? The disabled attr is used for variations for which variation_is_active is false. So the fallback behavior will lead to confusion. There has to be a reliable way to simply get rid of invalid options.
  2. It's still making all changes directly in the DOM. There has to be a better way than adding/removing classes and attributes one after the other right in there.
  3. It's still refreshing the DOM on touchstart/focus, which I find confusing, bug-prone and not that helpful when trying to hook into what's happening from another script. Basically the state of the select fields is out-of-sync with what should be there after a select change event. The state of the options should always be accurate depending on what's chosen, without having to wait for focus.

I've quickly hacked up an example based on #12566 (link below) which

  • resolves the reported issue,
  • deals with the problem with hidden placeholders,
  • tackles pain points (1), (2) and (3), (hopefully?)

It relies on doing a single .html() call to update the attribute options state after preparing their DOM state offline.

However, I don't know if any browsers could have issues with refreshing select options with this method. Essentially, nothing gets removed so I guess that val() state should be maintained. I'd be very surprised if updating portions of the DOM like this caused issues in modern browsers.

I think it's worth looking at this to see if it's cross-browser compatible, since it should be a tad faster and avoids the issues of the "fallback" behavior when HIDDEN is unsupported.

variation-script-fixes...franticpsyx:variation-script-fixes-take-2

PS: Let's not rush yet another variations script patch without looking at all pros/cons and other approaches. Browsers are apparently really fragile with select so it makes sense to look for the safest solution by doing some tests.

@franticpsyx
Member
franticpsyx commented Dec 13, 2016 edited

How widely supported is it?

Safari -- I see disabled options, not hidden (gahh). Safari is a widely used browser (cough).
iOS 10 -- Same as above.
Chrome -- Sweet, seems to handle this patch correctly.
Firefox -- Works.

Based on these results alone I'd say 👎

@mikejolley
Member

@franticpsyx

It's still making all changes directly in the DOM. There has to be a better way than adding/removing classes and attributes one after the other right in there.

The classes are only there for backwards compatibility. If you look deeper it uses DATA attributes, not classes.

It's still refreshing the DOM on touchstart/focus, which I find confusing, bug-prone and not that helpful when trying to hook into what's happening from another script

I think this is only so options are fully expanded for the current attribute so it doesn't just show the attribute for the matching variation.

I think it's worth looking at this to see if it's cross-browser compatible, since it should be a tad faster and avoids the issues of the "fallback" behavior when HIDDEN is unsupported.

You're removing elements from the DOM. You already said this was bad :p And it is - it's not reliable as we've seen with Safari etc.

The only way to hide options are:

  • hidden attribute where supported
  • wrapping options span which is the stupidest hack I've come across
  • display none which has the same support as hidden it seems
  • removing elements from the DOM

Stepping back, why is greying out options so bad vs removing them? I know we've done both in the past, but what is the actual concern there?

Right now I'm more concerned with options just not working and all the edge case browser issues we keep coming across with DOM manipulation.

cc @kellychoffman @jameskoster

@mikejolley
Member

@franticpsyx Also scared about a patch so big for a point release FYI. Your changes are extensive and may break compatibility?

@franticpsyx
Member
franticpsyx commented Dec 13, 2016 edited

You're removing elements from the DOM. You already said this was bad

How is a clone removing options from the DOM -- variation-script-fixes...franticpsyx:variation-script-fixes-take-2#diff-b4056a3102144c129f0d05be2cf8cf32R339

Essentially the only change is happening down here: variation-script-fixes...franticpsyx:variation-script-fixes-take-2#diff-b4056a3102144c129f0d05be2cf8cf32R428

There's a difference between DOM replacement like this and actually removing / reattaching option elements. select elements in some browsers will lose state once all options are removed. This PR doesn't remove options anymore but it does so with a tradeoff -- the fallback.

I think this is only so options are fully expanded for the current attribute so it doesn't just show the attribute for the matching variation.

Updating DOM state on focus has been a frequent cause of issues. @lucasstark has been working with the variations script for ages and I know this has caused him pains. Just saying I think there might be a better way, but it's not really relevant to the issue so we can just leave this part out.

Stepping back, why is greying out options so bad vs removing them? I know we've done both in the past, but what is the actual concern there?

On point. Greying out has been used to represent variation_is_active state. The fallback behavior mixes two different things. So people who use variation_is_active to -- say -- grey out Out-of-stock combinations (or other uses that I don't know) will not like the change. It is a UI change that affects modern browsers, so people will naturally complain. Doesn't really matter what I think about Safari ;)

Also scared about a patch so big for a point release FYI. Your changes are extensive and may break compatibility?

Good point. I'm not saying we should merge my hack (it's a hack until scrutinized + proven right), but I'd be cautious about introducing a UI change in a release that could be one of the last for 2.6. People might be stuck with a change they won't like for a while.

But it makes sense to get more people to have a look and comment because changes in the variations script (no matter how many lines of code are affected) have traditionally caused pain.

Right now I'm more concerned with options just not working and all the edge case browser issues we keep coming across with DOM manipulation.

Also a good point, a fix is needed -- but complaints about the fallback behavior should be expected as well.

@franticpsyx
Member
franticpsyx commented Dec 13, 2016 edited

Stepping back, why is greying out options so bad vs removing them?

Another note -- Cross-browser consistency is important when we are talking about latest versions of modern browsers. Two people seeing something entirely different in a UI is not ideal.

@mikejolley
Member

@franticpsyx then what we need is a hybrid of our solutions without all the new 'locking' stuff you've introduced.

@mikejolley
Member

Also the filters and data stuff I added is way cleaner than all the extra CSS selectors we have in current ;)

@franticpsyx
Member
franticpsyx commented Dec 13, 2016 edited

without all the new 'locking' stuff you've introduced.

Oh that one is not in the https://github.com/franticpsyx/woocommerce/tree/variation-script-fixes-take-2 branch at all.

If .html() replacement is safe, after all (IS IT?), we can even keep the current exclude/focus behavior and just keep the .html() replacement and the other code in update_variation_values ( https://github.com/franticpsyx/woocommerce/blob/variation-script-fixes-take-2/assets/js/frontend/add-to-cart-variation.js#L355-L432 ).

The problem is, even this part needs to be carefully reviewed. If you just compare that part with the changes of this PR, you'll see that I'm not changing too much. But that's not the point. The question is what's safer?

More eyes = better.

Just to clarify, I'm not too keen on accepting even some of my branch changes without scrutinizing those. As I said, I just hacked that in 10 minutes.

Also the filters and data stuff I added is way cleaner than all the extra CSS selectors we have in current

It is.

assets/css/woocommerce.scss
@@ -48,6 +48,10 @@ p.demo_store {
clear: both;
}
+option[hidden] {
+ display: none;
@jameskoster
jameskoster Dec 14, 2016 Member

Is this needed because we're adding removing DOM elements or something? I thought hidden elements were hidden automatically?

If it's required then I'm a little concerned. Lots of themes of course do not load our CSS.

If this style isn't present in the theme, options are going to be visible that shouldn't be (?)

If that's the case this probably shouldn't go in a fix release imo, unless we make the styles inline or something.

@mikejolley
mikejolley Dec 14, 2016 Member

I don't think it's needed if the browser supports hidden

@franticpsyx
franticpsyx Dec 14, 2016 Member

I was hoping display:none would work as a fallback but it doesn't seem to be supported by browsers that ignore the hidden attr.

@jameskoster
jameskoster Dec 14, 2016 edited Member

I was hoping display:none would work as a fallback but it doesn't seem to be supported by browsers that ignore the hidden attr.

I'll test that.

But I'm still concerned with this, even if it's just a fallback.

Edit: Can confirm, the CSS doesn't work in Safari. Looks like the span hack is the best bet there :(

@mikejolley
mikejolley Dec 14, 2016 Member

@jameskoster that span hack may break forms using select2 etc in themes. I don't want to use that one.

@franticpsyx
Member

Some more feedback on this --

greying out options so bad vs removing them

The fallback behavior mixes two different things. So people who use variation_is_active to -- say -- grey out Out-of-stock combinations (or other uses that I don't know)

Adding an example I'm very familiar with: Composite Products relies on the grey-out behavior to display attribute options that are "incompatible" based on a set of rules.

So in browsers that don't support the hidden attr, it will be impossible for users to tell whether an option is "inactive/incompatible" or just not available at all (hidden).

Just an example but I'm sure there are more similar use cases of "greying-out" based on .variation_is_active out there.

@franticpsyx
Member

Another idea to look into --

How do jQuery selectors such as option:selected or option:disabled behave with this PR, especially in browsers that don't support the hidden attribute?

@lucasstark
Member

Just FYI this changes the behavior of greying out out of stock items. In 2.6.9 with the filter

add_filter( 'woocommerce_variation_is_active', 'grey_out_variations_when_out_of_stock', 10, 2 );

function grey_out_variations_when_out_of_stock( $grey_out, $variation ) {

	if ( ! $variation->is_in_stock() )
		return false;

	return true;
}

Out of stock items will be greyed out. However in this branch they are not greyed out, they can be selected and user only knows it's out of stock once they select it.

mikejolley added some commits Dec 14, 2016
@mikejolley mikejolley Dump hidden css ee09293
@mikejolley mikejolley Merge branch 'pr/12600' into variation-script-fixes c79e96e
@mikejolley mikejolley Refactor JS 9ac694b
@mikejolley mikejolley Fix values
0a88ead
mikejolley added some commits Dec 14, 2016
@mikejolley mikejolley deselect disabled
f865a76
@mikejolley mikejolley prefix selectors
e3fb856
@mikejolley mikejolley Check after reload
61c4a1a
@mikejolley
Member

@claudiosanches can you give this another look over? I've moved things around so it's easier to read through and based on @franticpsyx PR. Fixes the firefox issue etc and does not use hidden...

mikejolley added some commits Dec 14, 2016
@mikejolley mikejolley 2 dollars
201d24e
@mikejolley mikejolley Fix ID
99f6099
@mikejolley mikejolley Change event after invalid deselected
3c83b38
@claudiosanches
Member

@mikejolley testing on Chrome and Firefox on Ubuntu looks good.

@franticpsyx

Looks good. Safari, Chrome, Firefox and iOS like it, at least the latest versions. Can't see anything wrong with the clean-up. My existing integrations with the script work fine.

+ this.$resetVariations.unbind( 'click' );
+ this.$attributeFields.unbind( 'change ' );
+
+ // Methods.
@franticpsyx
franticpsyx Dec 14, 2016 Member

Nitpicking but perhaps we can align these.
Also is binding really needed here?

+ * When the cart button is pressed.
+ */
+ onAddToCart: function( event ) {
+ if ( $( this ).is('.disabled') ) {
@franticpsyx
franticpsyx Dec 14, 2016 Member

Nitpicking again. Search and replace (' and ') with ( ' and ' ) for consistency?

-
- // Upon gaining focus
- .on( 'focusin touchstart', '.variations select', function() {
- if ( 'ontouchstart' in window || navigator.maxTouchPoints ) {
@franticpsyx
franticpsyx Dec 14, 2016 Member

Would anything break by not triggering woocommerce_variation_select_focusin? What are possible uses for this trigger (can't think of any after these changes)?

- current_attr_select.find( 'option' + option_gt_filter + ':not(.enabled)' ).attr( 'disabled', 'disabled' );
+ // Finally, copy to DOM and set value.
+ current_attr_select.html( new_attr_select.html() );
+ current_attr_select.find( 'option' + option_gt_filter + ':not(.enabled)' ).prop( 'disabled', true );
@franticpsyx
franticpsyx Dec 14, 2016 edited Member

Regarding .attr and .prop, I am a bit confused with which jQuery version deprecated .attr. My console is clean with this:

// Detach unattached.
new_attr_select.find( 'option' + option_gt_filter + ':not(.attached)' ).remove();

// Grey out disabled.
new_attr_select.find( 'option' + option_gt_filter + ':not(.enabled)' ).attr( 'disabled', 'disabled' );

// Finally, copy to DOM.
current_attr_select.html( new_attr_select.html() );

It would be nice if we could set the disabled attribute before updating the DOM.

(For reference, @mikejolley and I briefly discussed that .prop does not work with "offline" jQuery objects, but .attr does.)

@franticpsyx
Member

CC @lucasstark for an extra pair of eyes.

@lucasstark
Member

The data element for attribute_options on the select list is inconsistent with 2.6.9. In 2.6.9 the data_attributes is an array of the option elements, in the fix it's the html for the options.

Not sure if you want to address this, but it might potentially be used by extension developers other than myself for Swatches. It's easy for me to update Swatches accordingly but just throwing this out there.

@mikejolley
Member

@lucasstark swatches worked in my testing. Is there breakage?

@lucasstark
Member
lucasstark commented Dec 14, 2016 edited

@mikejolley Yeah using the variation-script-fixes branch swatches didn't work for me if I leave this in my swatches script.

 var current_options = '';
                if (!$current_attr_select.data('attribute_options')) {
                    current_options = $current_attr_select.find('option:gt(0)').get();
                } else {
                    current_options = $current_attr_select.data('attribute_options');
                }

Instead I just changed it to always grab the options of the select rather than the data attribute.

Error: http://d.pr/i/AcP
Script Info: http://d.pr/i/T9HT

Script Info in the 2.6.9 Branch: http://d.pr/i/xr6h

So again, not a big deal for me, but just wanted to bring it up in case you do want to store the options array like before instead of the HTML.

@franticpsyx
Member
franticpsyx commented Dec 15, 2016 edited

So again, not a big deal for me, but just wanted to bring it up in case you do want to store the options array like before instead of the HTML.

Nice catch Lucas!

Just a note to @mikejolley --

Before submitting the 2.7 PR I struggled for a few minutes trying to do what Lucas describes here -- storing objects rather than text in the data attribute. I had some issues with my debug output though that I couldn't explain, even after ensuring that the objects were deep copies of the originals. So although the code seemed to work, my debug output was messed up.

I ended up storing the reference data as a string for simplicity / performance / debugging purposes. Didn't think anyone would actually use it like Lucas.

Edit -- on a side note, if the DOM was always updated on every select change in the first place, I guess there wouldn't be a reason to need this. Swatches state could be updated based on select options state.

mikejolley added some commits Dec 15, 2016
@mikejolley mikejolley BW compat data 6239583
@mikejolley mikejolley Use prototype pattern so form can be reused on the page.
400a537
@mikejolley
Member

Ok, added compatibility with that data attribute.

@mikejolley mikejolley added a commit that referenced this pull request Dec 15, 2016
@mikejolley mikejolley Variations script refactor from #12585
Applying this to master to match 2.6 PR
ad4704f
@jameskoster
Member

Tested IE8 which is the oldest I have access to and all seems fine.

@mikejolley
Member

👍

@mikejolley mikejolley merged commit f90d75e into release/2.6 Dec 15, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@mikejolley mikejolley deleted the variation-script-fixes branch Dec 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment