Modify ↓

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#13494 closed defect (fixed)

Broken on Internet Explorer version 8

Reported by: solstice333 Owned by: Jon Ashley
Priority: normal Component: KeepInterfaceSimplePlugin
Severity: normal Keywords:
Cc: Trac Release: 1.2

Description

Hiding fields with the visible property is broken on internet explorer. The fields are still exposed.

Attachments (2)

error.PNG​ (64.8 KB) - added by solstice333 5 years ago.
error_then.PNG​ (95.7 KB) - added by solstice333 5 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 6 years ago by Jon Ashley

Can you please tell me which version of Internet Explorer you see this on? Thanks.

comment:2 Changed 6 years ago by Jon Ashley

I have tried to reproduce this on Internet Explorer version 11 (version 11.0.9600.19129), but without success.

Can you please press F12 while viewing the ticket in IE to bring up the developer console. Click on the 'Debugger' tab, then click on the little folder icon near the top left corner of the main debugger window. This should list all the scripts that are currently loaded.

Can you please confirm that the scripts kis.js and bluebird.min.js are both present in the list. (The bluebird.min.js script only loads when using the Internet Explorer browser.)

comment:3 Changed 5 years ago by solstice333

Sorry, forgot to list the version in description: 8.0.7601.24000

Last edited 5 years ago by solstice333 (previous) (diff)

comment:4 Changed 5 years ago by solstice333

Hm, maybe this isn't worth fixing but rather looking for some cheesy workaround. Basically some consumer is using some old version of IE to submit the cc field and it's spamming too much. I confirmed both kis.js and bluebird.min.js are loaded.

comment:5 Changed 5 years ago by solstice333

Alright, SimpleTicketPlugin seems to be a decent workaround for now. I just want to make it so some normal user without the TICKET_EDIT_CC permission, cannot create a ticket that has a bunch of email addresses in the cc field. The user who's doing this is probably using some old version of IE and is able to get around kis features. That said, kis is the only plugin I can find that does any sort of field hiding bound to the ticket state. SimpleTicketPlugin is good enough for hiding fields at the ticket <none> state even on old IE.

comment:6 Changed 5 years ago by Jon Ashley

OK, I think the problem is likely to be the ancient version of Internet Explorer, and the fact that it isn't quite compatible with the Bluebird library. From the Bluebird website:

IE7 and IE8 do not support using keywords as property names, so if supporting these browsers is required you need to use the compatibility aliases:

Promise.try() -> Promise.attempt()
.catch() -> .caught()
.finally() -> .lastly()
.return() -> .thenReturn()
.throw() -> .thenThrow()

The kis plugin definitely uses the .catch method in two places, so won't work as-is on IE7 or IE8. I might be able to do something about this. I will have a look at it over the weekend.

As for 'cheesy workarounds'... if you are willing to give it a try, you could edit the file kis.js in the plugin source and remove the two .catch() clauses that are in there (delete everything from the full-stop before catch up to but not including the semi-colon on the next but one line). This will remove some of the plugin's ability to catch and report errors, but you probably don't need that anyway. Don't change catch to caught as suggested by the Bluebird website, as that will break the plugin for every browser that isn't Internet Explorer.

comment:7 Changed 5 years ago by Jon Ashley

Status: new → accepted
Summary: broken on internet explorer → Broken on Internet Explorer version 8

This plugin will not be supported for versions of Internet Explorer older than version 7, as Bluebird doesn't officially support these. There are anecdotal reports that Bluebird does in fact work with Internet Explorer 6 and Netscape 7.

comment:8 Changed 5 years ago by solstice333

Ah I see. Yeah, I'll try the workaround you suggest if for some reason the supposed-to-be-hidden-fields in the other ticket states (aside from newticket) become problematic, but so far it's just been that damn cc text field that's been bad.

Hm, seems like there's only two options as far as catching the rejected promise. What about trying to call .catch and if that fails call .caught? For instance, 

function mockReject() {
   return new Promise((resolve, reject) => { throw new Error("foo error") });
}

let p = mockReject();

try { p.catch(e => console.error(e.message)) }
catch (e) {
   if (e instanceof TypeError) { p.caught(e => console.error(e.message)) }
   else { throw e }
}

I figure it'll throw some kind of error that you can catch and duck type again.

Anyway, no rush. Thanks for looking into this. I appreciate it. Let me know if I can provide any more information.

comment:9 Changed 5 years ago by Jon Ashley

In 17298:

Experimental patch to make compatible with IE7/IE8
re #13494

comment:10 in reply to:  5 ; Changed 5 years ago by Jon Ashley

Replying to solstice333:

Alright, SimpleTicketPlugin seems to be a decent workaround for now. I just want to make it so some normal user without the TICKET_EDIT_CC permission, cannot create a ticket that has a bunch of email addresses in the cc field. The user who's doing this is probably using some old version of IE and is able to get around kis features. That said, kis is the only plugin I can find that does any sort of field hiding bound to the ticket state. SimpleTicketPlugin is good enough for hiding fields at the ticket <none> state even on old IE.

I have put a patch on the trunk (see above), which I think doesn't break for existing browsers. I can't test it on Internet Explorer 7 or 8, so please let me know if it works for you.

I have also put a fix into the way the Warden rules operate, because from your comment above, I think you might be better off using a Warden rule to enforce the ticket creation restriction. Users can get around any Assistant rule just by turning off Javascript, but getting aroung the following rule won't be so easy: 

[kis_warden]
can't set initial cc list = _status == '' && cc && !has_role('TICKET_EDIT_CC')

Without the fix, this rule doesn't work because _status doesn't evaluate to an empty string.

comment:11 in reply to:  10 ; Changed 5 years ago by solstice333

Replying to Jon Ashley:

Replying to solstice333:

Alright, SimpleTicketPlugin seems to be a decent workaround for now. I just want to make it so some normal user without the TICKET_EDIT_CC permission, cannot create a ticket that has a bunch of email addresses in the cc field. The user who's doing this is probably using some old version of IE and is able to get around kis features. That said, kis is the only plugin I can find that does any sort of field hiding bound to the ticket state. SimpleTicketPlugin is good enough for hiding fields at the ticket <none> state even on old IE.

I have put a patch on the trunk (see above), which I think doesn't break for existing browsers. I can't test it on Internet Explorer 7 or 8, so please let me know if it works for you.

I have also put a fix into the way the Warden rules operate, because from your comment above, I think you might be better off using a Warden rule to enforce the ticket creation restriction. Users can get around any Assistant rule just by turning off Javascript, but getting aroung the following rule won't be so easy: 

[kis_warden]
can't set initial cc list = _status == '' && cc && !has_role('TICKET_EDIT_CC')

Without the fix, this rule doesn't work because _status doesn't evaluate to an empty string.

Thanks, I'll get to this eventually. Need to finish a few things first.

comment:12 in reply to:  11 Changed 5 years ago by anonymous

Replies inline...

Replying to solstice333:

Replying to Jon Ashley:

Replying to solstice333:

Alright, SimpleTicketPlugin seems to be a decent workaround for now. I just want to make it so some normal user without the TICKET_EDIT_CC permission, cannot create a ticket that has a bunch of email addresses in the cc field. The user who's doing this is probably using some old version of IE and is able to get around kis features. That said, kis is the only plugin I can find that does any sort of field hiding bound to the ticket state. SimpleTicketPlugin is good enough for hiding fields at the ticket <none> state even on old IE.

I have put a patch on the trunk (see above), which I think doesn't break for existing browsers. I can't test it on Internet Explorer 7 or 8, so please let me know if it works for you.

Okay, I tried the one in trunk on IE8. Same behavior -- the cc field is not hidden on IE8 , but is hidden, as expected, on firefox. In other words, same behavior.

I have also put a fix into the way the Warden rules operate, because from your comment above, I think you might be better off using a Warden rule to enforce the ticket creation restriction. Users can get around any Assistant rule just by turning off Javascript, but getting aroung the following rule won't be so easy: 

[kis_warden]
can't set initial cc list = _status == '' && cc && !has_role('TICKET_EDIT_CC')

Without the fix, this rule doesn't work because _status doesn't evaluate to an empty string.

Thanks, the warden works great as you suggested. I'll use this alongside the SimpleTicketPlugin just in case.

Thanks, I'll get to this eventually. Need to finish a few things first.

comment:13 Changed 5 years ago by solstice333

Let me know if you want to go through another cycle of testing. I have the environment set up for it.

comment:14 Changed 5 years ago by Jon Ashley

Rats. I thought that would fix it. I don't suppose their are any error messages in the IE8 JavaScript console that might shed some light on this? I can't even remember if IE8 has a JavaScript console.

If not, I'll have to see if I can maybe set up an IE8 environment somehow for testing.'

Changed 5 years ago by solstice333

Attachment: error.PNG​ added

comment:15 Changed 5 years ago by solstice333

Looks like IE8 really doesn't like js keywords being used as properties.

BTW, ignore the second error. I think it's from a different plugin.

comment:16 Changed 5 years ago by solstice333

What about instead of doing window.Promise.prototype.catch, you do window.Promise.prototype["catch"]?

comment:17 Changed 5 years ago by Jon Ashley

That sounds hopeful. IE shouldn't be executing the failing line, so it must be getting picked up in an initial syntax check. I think you are right and accessing the 'catch' property that way should evade such a check. Can you give that a try and let me know if it works please?

Changed 5 years ago by solstice333

Attachment: error_then.PNG​ added

comment:18 Changed 5 years ago by solstice333

Hm, promise objects don't seem to have the then method defined

comment:19 Changed 5 years ago by Jon Ashley

That's very odd. I'm going to try and install IE8 myself so that I can look at this properly. Can you confirm that Bluebird is still getting loaded in IE? (The error above looks as though the return statement has caused the problem, but that's even harder to believe.)

comment:20 Changed 5 years ago by Jon Ashley

Managed to reproduce this eventually. It turned out to be the bind() method that is causing the problem, not then(); apparently IE8 doesn't implement it. There are other problems, such as the jQuery prop() method not working.

The good news is that a polyfill to implement bind() fixes the error. The bad news is that having done so, the field-hiding behaviour for the plugin still doesn't work on IE8. I am still hoping this can be sorted out, but the IE8 debugging environment is poor so it may take a while.

comment:21 Changed 5 years ago by Jon Ashley

The most recent commit on the plugin trunk works for me with IE8. The changes are:

  • remove trailing commas from list items (IE8 rejects them in compatibility mode);
  • use $.bind() instead of $.on() if the version of jQuery present is lacking .on();
  • use $.attr() instead of $.prop() on Internet Explorer;
  • provide a polyfill for Javascript's .bind() on Internet Explorer.

Can you please give it a try and confirm here that it works for your user? Thanks.

comment:22 in reply to:  21 Changed 5 years ago by solstice333

Replying to Jon Ashley:

The most recent commit on the plugin trunk works for me with IE8. The changes are:

  • remove trailing commas from list items (IE8 rejects them in compatibility mode);
  • use $.bind() instead of $.on() if the version of jQuery present is lacking .on();
  • use $.attr() instead of $.prop() on Internet Explorer;
  • provide a polyfill for Javascript's .bind() on Internet Explorer.

Can you please give it a try and confirm here that it works for your user? Thanks.

Thanks. Trying this out now. I'll let you know if it works for me too.

comment:23 Changed 5 years ago by solstice333

Ok, it seems like whatever is in trunk is working for me. Just did some light testing on firefox and IE8. Thanks! I believe you can close this ticket as fixed.

comment:24 Changed 5 years ago by Jon Ashley

In 17314:

Fix problem in how Warden rules look up the current value of a field when it _isn't_ changing. Related to the changes made for #13494 and found in testing.
re #13494

comment:25 Changed 5 years ago by Jon Ashley

Resolution: → fixed
Status: accepted → closed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jon Ashley.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment

E-mail address and name can be saved in the Preferences.

Attachments ↑ Description ↑
 
Note: See TracTickets for help on using tickets.