To reproduce, say you have a form like this one:
// switching to 'bar' item should display 'dependant' text field
$form['principal'] = array(
'#type' => 'radios',
'#title' => 'Principal',
'#options' => array('foo', 'bar'),
'#default_value' => 0,
);
// should get displayed based on selection on 'principal'
$form['dependant'] = array(
'#type' => 'textfield',
'#title' => 'Dependant',
'#states' => array(
'visible' => array(
// (1) works with this, because a explicit string "1" is passed: ':input[name="principal"]' => array('value' => "1"),
':input[name="principal"]' => array('value' => 1), // doesn't work in IE8
),
),
);
Switching 'principal' radio buttons it is expected that 'dependant' text field is displayed and hidden.
Instead, in IE8 (without uncommenting section (1)), 'dependant' visible state is never changed on 'principal' changes, the problem is because of the usage of 'reference.constructor.name' js which is not supported on IE8 and the posterior call to
function compare (a, b) {
return (a === b) ? (typeof a === 'undefined' ? a : true) : (typeof a === 'undefined' || typeof b === 'undefined');
}
Where this method compares considering types (and the actual values compared are a Number vs a String, which gets false).
I'm attaching a module to reproduce this issue easily and a patch tested on Firefox, Chome and IE8.
Notes:
Bug only found in IE8, problem could exist in newer IE versions.
Comment | File | Size | Author |
---|---|---|---|
#68 | 1962800-68.patch | 1.86 KB | JeroenT |
Comments
Comment #1
hablutzel1 CreditAttribution: hablutzel1 commentedHere you can find that 'name' is not supported by IE
https://developer.mozilla.org/de/docs/JavaScript/Reference/Global_Object...
Comment #2
MatthijsI've created a similar patch based on jQuery.type(). This looks a bit cleaner, no?
Comment #3
filsterjisah CreditAttribution: filsterjisah commentedWorks great. Thx Matthijs!
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commented#879580-16: States fail when using integer values for select/radio dependencies and subsequent comments suggest this is an issue in newer version of Internet Explorer, in which case it would need to be fixed in Drupal 8 first. There's also a patch in that issue (before it was re-closed) that could be used for comparison.
Comment #5
DnaX CreditAttribution: DnaX commentedI can confirm this bug either with IE10. A workaround can be adding a empty string to the number to force a string cast.
Comment #6
dcam CreditAttribution: dcam commentedReverting version change. This issue must be fixed in Drupal 8 first.
Comment #7
nod_Comment #8
Jalandhar CreditAttribution: Jalandhar commentedPatch needs to be updated.
Comment #9
mgiffordreroll for D8.
Comment #10
jhedstromPatch still applies. Adding the manual testing tag.
Comment #11
AnybodyPatch works good for me. How can we proceed here? The issue leads to follow-up problems for example in conditional_fields.module in IE, see #1373656: Dependent fields don't open in IE
Comment #12
mgifford@Anybody You could mark it RTBC with manual testing.
Comment #13
AnybodyIf we can get some more feedback I'd like to set this RTBC of course. My feedback is not enough I think. But it works great for me.
Comment #14
MathieuMa CreditAttribution: MathieuMa commentedWe have been testing this patch (because of the reported issue) without any side effect as of now.
Comment #15
AnybodyComment #18
spencer95@gmail.com CreditAttribution: spencer95@gmail.com at S8080 commentedWe're using this patch on D7 and D8. It's working for us in IE 8/9/10.
Comment #19
AnybodyPerfect. Let's get this into a new release as soon as possible :)
Comment #20
alexpottSeems to introduce some javascript formatting errors.
Comment #21
geertvd CreditAttribution: geertvd at XIO commentedBumped into this issue in a d7 project today. Would be nice to have this fixed in d8 so it can be backported to d7.
Comment #22
droplet CreditAttribution: droplet commentedI wonder if we should use jQuery.type for all browsers.
Comment #23
peterpoe CreditAttribution: peterpoe as a volunteer commentedConfirming that bug applies up to IE11, and that the patch works after some manual testing.
This breaks many Conditional Fields configurations in IE, please commit.
@droplet May be a good point, but let's get this fixed first
Comment #24
catchThis really needs a comment.
Comment #25
peterpoe CreditAttribution: peterpoe as a volunteer commentedComment added.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedHere's the D7 backport.
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #30
Anonymous (not verified) CreditAttribution: Anonymous commentedRan the tests locally and manually tested the one that failed in the above and it passed, re-queuing.
Comment #31
geertvd CreditAttribution: geertvd at Geert van Dort commentedPatch looks good, I just recreated it to fix some indentation issues
Comment #32
ufku CreditAttribution: ufku commentedJquery.type() always returns "object" for custom types.
Regex method seems more reliable.
var name = reference.constructor.name || (reference.constructor.toString().match(/function\s+([^\(\s]+)\(/)||['', ''])[1];
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedThat doesn't matter, does it? Isn't the block where type is used just trying to find a valid fallback if the name doesn't exist regardless? Introducing regex here seems messy and the last patch seems to work fine.
Can you determine a use case where something is broken/bad? I'd like to have this RTBC and fixed so I don't have to patch my core every time.
Comment #34
smaz+1 for patch in #31 working for me in D7.
Cheers!
Comment #39
filsterjisah CreditAttribution: filsterjisah for District09 commentedI've added a D8 version of it.
Comment #40
Jeroen_005 CreditAttribution: Jeroen_005 at Pàu for District09 commentedComment #41
maacl CreditAttribution: maacl commentedHad this Problem on IE11, #39 is working for me. Thanks @filsterjisah
Comment #42
AnybodyConfirming #39 for D8. Did someone already check if the backport to D7 can work the same way?
Comment #43
star-szrA couple thoughts:
Comment #45
droplet CreditAttribution: droplet commentedIt depends on how we passing the function. Use jQuery workaround would be safer.
https://kangax.github.io/compat-table/es6/#test-function_name_property
Let's add some comments (and use jQuery workaround only IMO)
Comment #46
wesleydv CreditAttribution: wesleydv at District09 commentedAdding a new patch against 8.5.x (both .es6.js and the transpiled .js)
Only added a comment, code is the same as #39
Comment #47
wesleydv CreditAttribution: wesleydv at District09 commentedComment #51
MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer and at FFW commentedSomehow
git apply
fails to apply the patch #46 for me, butpatch -p1
works. Adding reroll here against core 8.6.1.Comment #53
aleevasComment #54
mpp CreditAttribution: mpp at AmeXio for District09 commentedComment #55
GrandmaGlassesRopeManUse template string here.
Don't use in, you could accidently match against a prototype. Use hasOwnProperty instead.
Comment #56
mpp CreditAttribution: mpp at AmeXio for District09 commentedThank you @alwaysworking for your feedback.
Personally I find this to be more readable:
name = name.charAt(0).toUpperCase() + name.slice(1);
Than this:
name = `${name.charAt(0).toUpperCase()}${name.slice(1)}`;
Note that this:
would be turned into:
I fixed your second remark in 1962800_56.patch, in 1962800_56_bis.patch a template string is used.
Comment #59
Alexj12 CreditAttribution: Alexj12 as a volunteer and at Zoocha commentedI'm updating the patches here to use
states.Dependent.comparisons.hasOwnProperty(name)
over
states.Dependent.comparisons.hasOwnProperty('name')
as the patches from #56 did not pass this check.
Comment #60
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedI have rerolled patch #59 as it failed to apply, please review.
Comment #61
nod_Not sure why we have 2 patches here. If the one with the concatenation is better just keep that one (i agree it's more readable and we're not concatenating that with a static string, so all good for me).
As long as eslint doesn't complain it's valid :), can we make it clear which patch should be reviewed? Thanks.
Comment #62
Alexj12 CreditAttribution: Alexj12 as a volunteer and at Zoocha commentedI agree too. I've been using the concatenating patch for a while with no issues and also believe it is more readable in this case. I'll hide the other patches and the argument can be made for the the template string if required.
Comment #63
Alexj12 CreditAttribution: Alexj12 as a volunteer and at Zoocha commentedComment #67
JeroenTLet's see if the patch still applies for Drupal 9.4.
Comment #68
JeroenTPatch no longer applied, so I created a reroll.
Also note that this patch is only necessary for Drupal 9. Since Drupal 10 drops support for Internet Explorer.
Comment #71
nod_IE is not supported in 10+ so this is not a concern anymore :)