Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pager buttons (PDFViewerApplication.page) not working correctly in IE11 #154

Closed
CMadden8 opened this issue Oct 14, 2019 · 19 comments
Closed
Assignees
Labels
bug Something isn't working IE11 Solved

Comments

@CMadden8
Copy link

CMadden8 commented Oct 14, 2019

@stephanrauh - I still haven't managed to dig into the other bug that was raised, but this one seems more important - and possibly easier to debug? The problem is with viewer.js this time, and only happens in IE11.

This is confirmed for all versions, including those prior to creating the es5 equivalents of the library files.

Clicking any of the pager buttons doesn't seem to trigger the scroll event for moving the pages within the document's container. The problem exists for up/down arrow, 'show first/last page' and clicking the page thumbnails themselves. You need to click the page thumbnails twice to fire the scroll event. Otherwise, the scroll never happens for the other buttons (though it does correctly focus on the relevant thumbnails).

I see this PDFViewerApplication.page is being triggered by the pager buttons, but it doesn't appear to work correctly for IE11. I haven't yet been able to find where the scroll occurs, but I intend to do more debugging tomorrow and will see if I can find the problem.

@stephanrauh
Copy link
Owner

When you say "pager button", are you referring to this one?

image

@stephanrauh
Copy link
Owner

Maybe this is related to #140?

@CMadden8
Copy link
Author

CMadden8 commented Oct 14, 2019

@stephanrauh - yes, that pager button - but also the below:

pager

You also need to double click the thumbnails to activate the page.

I don't think it's related to issue #140. This is happening in even the older versions, before the es5 files.

@CMadden8
Copy link
Author

@stephanrauh - quick update - yes, it does seem related to #140 after all. I'm getting the 'ERROR TypeError: Object doesn't support this action' when I click on a pager button in IE11.

I found this post on stackoverflow that seems relevant: https://stackoverflow.com/questions/31765353/ie11-javascript-error-script445-object-doesnt-support-this-action

I see dispatchEvent is being used in viewer.js. I'll see if I can find anything else. But it seems to be a common problem in IE11

@CMadden8
Copy link
Author

@stephanrauh - I found what appears to be the problem in viewer-es5.js.

The below function on line 10481 is causing the error:

value: function _setCurrentPageNumber(val) {}

and it's failing at:

this.eventBus.dispatch('pagechanging', {
source: this,
pageNumber: val,
pageLabel: this._pageLabels && this._pageLabels[val - 1]
});

I think I'll have to leave it for now, but looks like it's definitely related to this dispatch method and IE11's problems with it.

@CMadden8
Copy link
Author

And following on from the above, the dispatch function fails at line 4039, which is causing the problem of the above:

eventListeners.slice(0).forEach(function (listener) {
listener.apply(null, args);
});

Seem to be getting closer to an answer...

@CMadden8
Copy link
Author

CMadden8 commented Oct 15, 2019

It's Function.prototype.apply() for listener.apply(null, args) that's causing it.

I have all of the angular polyfills enabled, so I'm not sure what's required. But need to leave it for another day now.

@stephanrauh0000
Copy link
Contributor

@CMadden8 Awesome analysis! This evening, I've opened my company laptop to debug the issue. After a while, I've managed to reproduce the bug. It took a while, because it's sort of hidden behind several other exceptions (when I run the root project of https://github.com/stephanrauh/ngx-extended-pdf-viewer).

I've tried to add a breakpoint to eventListeners.slice(0).forEach(function (listener) {, but this particular line is called many times whenever I move the mouse. Setting the breakpoint on listener.apply(null, args); didn't make it any better.

Cutting a long story short, I'm impressed by your debugging skills, and I hope you'll find the solution. You know, I'm sort of at a loss... I don't have as much spare time as I'd like to devote to my pet project. I'm glad you're helping me!

Best regards,
Stephan

@CMadden8
Copy link
Author

Thanks @stephanrauh0000! It's not easy to find the time for these things. I had to go old school and use a few strategically placed console.logs to find it - breakpoints too much of a headache in IE11.

It's definitely the apply() function. But I'm not familiar with it, and it there's not much on it regarding why it would fail in IE11. The docs say it expects an array, so didn't get a chance yet to log the args being passed.

I have it on a branch in work, and I just dip in and out of it when there's time... It may take a while to figure this one out. I'll let you if I make any progress!

@CMadden8
Copy link
Author

@stephanrauh - I got around to looking at it briefly today and found the problem. Line 2277 of viewer-es5.js.

// if (pageNumberInput) {
// var pageScrollEvent = new CustomEvent('page-change');
// pageNumberInput.dispatchEvent(pageScrollEvent);
// }

I commented out this and everything worked properly in IE11. Up/down arrows. Click on the thumbnails.

However, it looks like this code shouldn't be commented out, so I need to investigate further. But it certainly removed the error and fixed the pager problem.

At least it's clear the apply() function is ok - the problem is that the function containing the broken logic above is being called in the loop where apply is being used.

@stephanrauh
Copy link
Owner

stephanrauh commented Oct 23, 2019

Awesome! Keep up the good work!

Now that you say it: as far as I remember, CustomEvents aren't supported by IE11. There's a workaround I've already used somewhere else. At the moment I'm a bit confused because the file I'm using to modify the original PDF viewer uses exactly the offending line, and I thought I've already fixed it:

"\n var pageScrollEvent = new CustomEvent('page-change');" +

Here's the error message include a bug fix proposal:
#123 (comment)

It seems I've forgotten to fix this bug - or maybe the bugfix has gone lost during the ages. :(

@CMadden8
Copy link
Author

@stephanrauh - interesting. I'll see what can be done to refactor it based off the above.

The strange thing is, everything works without the code for CustomEvent. At least in this case. It makes me wonder what it's actually doing...

@CMadden8
Copy link
Author

CMadden8 commented Oct 24, 2019

@stephanrauh - I have it working now, without commenting out the code. You can include the below code in the viewer-es5.js. I included it around line 23, as early as possible in script. This polyfill will allow all of the CustomEvents to work without any refactoring. I think this is the simplest solution?

 function CustomEvent ( event, params ) {
    params = params || { bubbles: false, cancelable: false, detail: undefined };
    var evt = document.createEvent( 'CustomEvent' );
    evt.initCustomEvent( event, params.bubbles, params.cancelable, params.detail );
    return evt;
  }

  CustomEvent.prototype = window.Event.prototype;

  window.CustomEvent = CustomEvent;

stephanrauh pushed a commit that referenced this issue Oct 24, 2019
stephanrauh pushed a commit that referenced this issue Oct 24, 2019
@stephanrauh
Copy link
Owner

@CMadden8 Thanks a lot to your diligent work. I've just added your polyfill to the source code. With a small exception: Following the advice of MDN, I omitted the CustomEvent.prototype bit. Does this make any difference?

@stephanrauh
Copy link
Owner

@CMadden8 I suppose the custom event is not important for the PDF viewer. It has been added for us. We can subscribe to the event and react when the user scrolls through the document. So you can safely comment the code if you don't need to react to scrolling.

@stephanrauh
Copy link
Owner

Having published version 1.7.0, I'm closing this ticket now. For some reason, https://pdfviewer.net still shows errors, but I guess that's because it's still loading the ES5 version of the service worker.

I'd love to hear from you. Does the new version solve the bug?

@CMadden8
Copy link
Author

No worries @stephanrauh - again, cheers for quickly resolving these issues!

I won't be back in the office until Tues next week, so I won't be able to test these latest changes until then. But it'll be the first thing I do Tues morn. I'll get back to you either way on whether it worked. Thanks again!

@CMadden8
Copy link
Author

@stephanrauh - confirmed this morning that this works in IE11, and I can see no side effects. Cheers again for addressing this!

@stephanrauh
Copy link
Owner

Glad to hear this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working IE11 Solved
Projects
None yet
Development

No branches or pull requests

3 participants