Skip to content
This repository has been archived by the owner on Feb 12, 2020. It is now read-only.

unable to move popup and toolbar in FF #712

Closed
balmas opened this issue Jul 30, 2019 · 21 comments
Closed

unable to move popup and toolbar in FF #712

balmas opened this issue Jul 30, 2019 · 21 comments
Labels
bug Something isn't working firefox verified passed qa

Comments

@balmas
Copy link
Member

balmas commented Jul 30, 2019

build 3.0.0.49 in FF
I can't move pop-up (after first move) and flex nav (from the start) in
https://latin.packhum.org/loc/22/1/17/8497-8502#17
https://la.wikipedia.org/wiki/Vicipaedia:Pagina_prima
http://www.thelatinlibrary.com/quintilian/quintilian.decl.mai9.shtml

Originally posted by @monzug in #634 (comment)

@balmas
Copy link
Member Author

balmas commented Jul 30, 2019

I think the popup problem is the same as #563 . The toolbar problem seems to be new.

@monzug
Copy link

monzug commented Jul 30, 2019

side effect of #472 fix?

@balmas
Copy link
Member Author

balmas commented Jul 31, 2019

Debugging notes:

I think the problem might be related to the interact.js library's use of window.requestAnimationFrame and window.cancelAnmiationFrame . When i tried calling unset on the interactInstance in the browser console, I got this error message which I think maybe is happening on dragging too but isn't bubbling up to the console when the interact methods are called through our code:

cancelAnimationFrame' called on an object that does not implement interface Window

I see a bunch of references to this error being thrown in Firefox extensions. Eg. see

Semantic-Org/Semantic-UI#3855
reagent-project/reagent#438

This is apparently a Firefox bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=1208775

https://bugzilla.mozilla.org/show_bug.cgi?id=1372649

@balmas
Copy link
Member Author

balmas commented Jul 31, 2019

confirmed that that is actually the error being thrown by the interact library when it executes the dragEndListener (the call is from the interact.stop() method which is called when the end listener function is done executing)

@balmas
Copy link
Member Author

balmas commented Jul 31, 2019

Unfortunately I can't figure out how to get the workaround described in https://bugzilla.mozilla.org/show_bug.cgi?id=1372649 to work .

It suggests rebinding window.cancelAnimationFrame

window.cancelAnimationFrame = window.cancelAnimationFrame.bind(window);

If I do this in the content.js, the background.js or the uiController, it has seemingly no impact on the interact.js code as initialized by the toolbar vue module.

I tried rebinding it directly in the Vue mounted handler, but when called from within the Vue code (or Vuex code -- I also tried as a state watcher) the window methods are read-only and I get "TypeError: ""cancelAnimationFrame" is read-only""

@balmas
Copy link
Member Author

balmas commented Aug 1, 2019

After further debugging, it looks like our use of Webpack is complicating this. The interact code which assigns a variable to window.cancelAnimationFrame (https://github.com/taye/interact.js/blob/44eb6166aedb4d051f0d552dbd303c41c4d6522d/packages/utils/raf.ts#L7) is executed in a Webpack prelude call before we ever actually activate interact on the toolbar element.

I think in order for the workaround for the FF bug to have any hope of working for us, we would have to somehow execute it in the Webpack code. I have no idea how to do that.

This will have to wait for @kirlat to get back from vacation I think. If we cannot figure out how to hack interact to work in the Firefox extension, I think our only options are to find a replacement for it or to write our own animation code.

@kirlat
Copy link
Member

kirlat commented Aug 12, 2019

Thanks for the thorough investigation! I can confirm that the fix described in https://bugzilla.mozilla.org/show_bug.cgi?id=1372649 works. I've included it by patching the interact.js code.

Will now look for a more convenient way to apply it.

@kirlat
Copy link
Member

kirlat commented Aug 13, 2019

I think I found a very hacky yet a simple solution to achieve what we need. It is to use imports-loader webpack loader (https://github.com/webpack-contrib/imports-loader) and assign a self-executing function to a variable.
The webpack rule for this looks like below:

{
          test: /.*node_modules(?:\/|\\)interactjs(?:\/|\\)dist(?:\/|\\)interact.js/,
          use: "imports-loader?alpheiosFFCSFix=>(function () { window.requestAnimationFrame=requestAnimationFrame.bind(window); window.cancelAnimationFrame=cancelAnimationFrame.bind(window) })()"
        }

As a result of this, webpack will create the following piece of code and will put it right before the code of interact.js package within the components bundle:

var alpheiosFFCSFix = (function () { window.requestAnimationFrame=requestAnimationFrame.bind(window); window.cancelAnimationFrame=cancelAnimationFrame.bind(window) })();

The function will be executed once the script is loaded this will be guaranteed to happen before any of interact.js functions be called.

I'm have yet to finish the full testing cycle, but the results are looking promising so far.

@balmas
Copy link
Member Author

balmas commented Aug 15, 2019

qa-3.0.0.55

@monzug
Copy link

monzug commented Aug 19, 2019

I can move the toolbar around the screen and if it goes outside the visible area, it bounces back.
However If I drag and drop the pop-up out of visible screen, it does not come back. Tested in https://la.wikipedia.org/wiki/Vicipaedia:Pagina_prima, https://latin.packhum.org, https://scaife.perseus.org, http://www.thelatinlibrary.com, https://texts-test.alpheios.net
can't get the pop-up back, not even by reloading the page

@monzug monzug reopened this Aug 19, 2019
@monzug monzug assigned balmas and unassigned monzug Aug 19, 2019
@balmas
Copy link
Member Author

balmas commented Aug 19, 2019

back to @kirlat for that.

@balmas balmas assigned kirlat and unassigned balmas Aug 19, 2019
@balmas balmas added the safari label Aug 19, 2019
@kirlat
Copy link
Member

kirlat commented Aug 21, 2019

Yes, I think that's definitely the case. I will update the popup's code.

kirlat pushed a commit that referenced this issue Aug 21, 2019
@balmas balmas added the waiting_build Waiting for build label Aug 22, 2019
@balmas
Copy link
Member Author

balmas commented Aug 23, 2019

this fix is in qa-3.0.0.58/components 1.2.28 (and in Safari build 3.0.0.58 build in Dropbox)

But on a quick test, it seems that it isn't fully fixed. I can still drag the popup down below the screen, but it does come back (usually) on the next lookup. @kirlat please take another look at this when you get a chance. Thanks!

@balmas
Copy link
Member Author

balmas commented Aug 23, 2019

I'm actually moving the issues with dragging the popup to a new issue, #770, because it's unrelated to the original issue reported in this bug, which was the brokenness of the interact.js library with Firefox. I'd like us to verify this one if dragging is generally working in Firefox now. Thanks!

@balmas balmas closed this as completed Aug 23, 2019
@balmas balmas removed the safari label Aug 23, 2019
@balmas balmas assigned monzug and unassigned kirlat Aug 23, 2019
@monzug
Copy link

monzug commented Aug 26, 2019

as said above, the toolbar bounces back in FF, but not the pop-up which will be covered in #770. tested in the following sites: latin library, perseus, text env,

I don't see build qa-3.0.0.58 in github. I did test on qa-3.0.0.57 and component 1.2.28
any idea?

@monzug monzug reopened this Aug 26, 2019
@monzug monzug assigned balmas and unassigned monzug Aug 26, 2019
@monzug monzug added question Further information is requested and removed waiting_verification labels Aug 26, 2019
@balmas
Copy link
Member Author

balmas commented Aug 26, 2019

oh, I'm sorry I forgot to push the 3.0.0.58 build to github. It's there now.

@balmas balmas closed this as completed Aug 26, 2019
@balmas balmas assigned monzug and unassigned balmas Aug 26, 2019
@balmas balmas added waiting_verification and removed question Further information is requested labels Aug 26, 2019
@monzug
Copy link

monzug commented Aug 26, 2019 via email

@monzug
Copy link

monzug commented Aug 27, 2019

retested in qa-3.0.0.59

@monzug monzug removed their assignment Aug 27, 2019
@monzug monzug added verified passed qa and removed waiting_verification labels Aug 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working firefox verified passed qa
Projects
None yet
Development

No branches or pull requests

3 participants