Fix for IE, to make it stable.

[acierto]

When the event is sent (ex. start dragging) slow browser (for example IE) is still didn't react on it and we send another event. So we need to wait a bit. No idea how to get the information that event is processed. So we wait for 0.5 s. For me it works all the time now.
I'm using Selenium Grid infrastructure with IE 11 slaves on Win 7 VMs.

[Kuniwak]

Could you unify indent to 2 width space, please?
Because it is hard to see what is changed.

(I know ?w=1 hack, but we cannot comment to changes on GitHub when use this hack)

[acierto]

Sure, have done it. Sorry IntelliJ IDEA has 4 spaces for tab by default.

[Kuniwak]

@acierto Thanks! I have a question.

Did you try shorter wait durations?
I think the shortest one is the best because it makes test fast.

[acierto]

You can make those values configurable in html-dnd, but not waiting at all makes this lib very flaky for the usage.

[Kuniwak]

Sorry for my too late reply... 🙇

What purpose of the changes at line 2 and 3 is?

[acierto]

it should be reverted, it's not supposed to be for a PR.
I had to release a separate lib to make it work, as my PR wasn't accepted ;)

[Kuniwak]

OK, we can take either of the 2 ways:

1. Revert some commits on this PR
2. Create new PR that include squashed commit

Which way do you like?

[acierto]

I reverted back package.json so it's ready for merge.

[Kuniwak]

Thanks! But still have a problem about coding style as the following:

src/html_dnd.ts[22, 1]: trailing whitespace
src/html_dnd.ts[25, 1]: trailing whitespace
src/html_dnd.ts[29, 1]: trailing whitespace
src/html_dnd.ts[34, 1]: trailing whitespace

Would you remove the trailing whitespaces?

[acierto]

Hi man, do you have any comments regarding PR to merge it?

The tests are not failing anymore after the last change, hopefully you will accept the PR and we can use your library directly instead of own fork!

If you have any comments, please let me know

[chin2km]

+1. It works flawlessly.

[Kuniwak]

We should make test pass to keep stable.

So, I'm thinking about how to make it be testable.
Please wait few days.

[acierto]

Let me clarify what you mean, as the first sentence doesn't make any sense for me.
Tests are already stable and they pass.

Do you want extra tests to reveal the current issue with IE e2e tests and then verify with this PR that the problem will be resolved?

Hint from my side, you need to have tests which will not render immediately, it should happen slowly, as it happens in real life on a big project. And as IE driver for protractor by nature is very slow it is reproducible on master code in 100% of the cases.

[Kuniwak]

Oops, I missed the CI status... I'm sorry.

Now, I'm reviewing the patch again.

[Kuniwak]

I don't want to use the sleep if it is possible. Because it make very high CPU usage.

I'm searching other ways that use "Execute Async Script API" such as WebDriver#executeAsyncScript.

But I don't know that whether major WebDriver libraries support this API.
If major WebDriver libraries do not support them, I' gonna accept the Pull request.

[Kuniwak]

Fairly, we can use "Execute Async Script" API on major libraries:

Library	Status for "Execute Async Script" API	Source
selenium-webdriver	Supported	https://seleniumhq.github.io/selenium/docs/api/javascript/module/selenium-webdriver/index_exports_WebDriver.html#executeAsyncScript
WebdriverIO	Supported	http://webdriver.io/api/protocol/executeAsync.html
Nightwatch.js	Supported	http://nightwatchjs.org/api/executeAsync.html
Protractor	Supported	http://www.protractortest.org/#/api?view=webdriver.WebDriver.prototype.executeAsyncScript

[acierto]

Fair enough, what do you think about it?

https://github.com/zengfenfei/delay

[Kuniwak]

Fair enough, what do you think about it?

I saw it, but this library can not fit in this case.

But I think that using "Execute Async Script" API is the better way:

var dragAndDrop = require('html-dnd').code;
var webdriver = require('selenium-webdriver');
var By = webdriver.By;

var driver = new webdriver.Builder()
  .forBrowser('firefox')
  .build();

driver.get('http://example.com');

var draggable = driver.findElement(By.id('draggable'));
var droppable = driver.findElement(By.id('droppable'));

// This is the only change: executeScript -> executeAsyncScript
driver.executeAsyncScript(dragAndDrop, draggable, droppable);

driver.quit();

How do you feel it?

[Kuniwak]

I'm gonna send new Pull request to your branch about it.

[acierto]

If it would be the single change how will it resolve the issue? As still 4 events won't wait for each other.
Basically with

// This is the only change: executeScript -> executeAsyncScript
driver.executeAsyncScript(dragAndDrop, draggable, droppable);

you guarantee that the end result will be synchronised, but the problem is also happening if another event is triggered with not waiting till the first one finished.

We can fix it or with callback hell as it is now, or with Promises, what will look better and as an end accord we can use executeAsyncScript to get rid of sleep.

[acierto]

I sent you an invitation in GoogleChat, maybe we can faster discuss it there.

[Kuniwak]

I sent you an invitation in GoogleChat, maybe we can faster discuss it there.

Hmm, I feel it is hard because I'm not good at speaking English.
I like text-based conversation because some translation tools can help me.