search

guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.51k stars 245 forks source link

UndoManager Improvements (Firefox Freezing Fix) #353

Closed aaalsaleh closed 9 years ago

aaalsaleh commented 9 years ago

Second attempt of #346.

This PR only address the failing test by avoiding passing options to scribe in the test file undo-manager.spec.js. Passing options in the tests is known to cause freezing, then a timeout, which end up failing the test. Now, the interval is not passed as an option. Instead, it is changed before starting the tests. It was successfully tested on Firefox 31 on Mac OS X (10.10).

Hopefully it will pass all other browsers/platforms this time around.

Edit: I've just tested it on Firefox 32, and I think there is still freezing issue. I'll try to identify the source of the issue. Actually not only aaalsaleh/scribe:master freezes, but also guardian/scribe:master freezes on Firefox 32 on Mac OS X (10.10)!

aaalsaleh commented 9 years ago

@rrees Firefox 31 problem is fixed, and it was due to the helpers as mentioned. With the last commit it can be considered fixed. However, Firefox 32 on Mac OS X 10.10 freezes with both guardian/scribe and aaalsaleh/scribe. I made fresh clones of both of them and verified that just now.

I tried the new version in #347 just to see, and both guardian/scribe and aaalsaleh/scribe went through successfully. To double check, I down graded back, and the same issue came back.

If this is a problem with the master branch, then I think it is a problem in the testing suite with this particular browser version. Can someone else confirm this manually for guardian/scribe and aaalsaleh/scribe with Firefox 32 (possibly on Mac OS X)?

rrees commented 9 years ago

@aaalsaleh I'll try and merge with master but I think we can live with disabling FF32 if that is the only problematic browser for now

aaalsaleh commented 9 years ago

@rress, technically it is most probably a problem with the tests suite, not the browser. The test freeze right from the beginning, and you can tell by the browser not even loading the local server URL to start the tests. I don't know what is the problem, but the tests run fine with #347. I've also made quick manual tests yesterday with both branches (before merge), and I didn't see any problems with FF32.

aaalsaleh commented 9 years ago

@rrees you merged both PR, but for some reason you kept the undo options (or was kept due to the separate two PRs?). Please remove the options, it is the cause of the freezing/failing that I fixed with this PR. Edit: I think the order of the merge/revert is to blame. #346 should have been reverted first, then this #353 should've been merged after. I believe the opposite was done? In any case please remove the options on the line I commented on, and the tests will pass.

rrees commented 9 years ago

Yes, the order of the merges was to blame, I created a branch to make it a bit easier for me to try and keep the change in sync with master.