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

Adding lane crashes IE/Edge #746

Closed
philippfromme opened this issue Jan 3, 2018 · 20 comments
Closed

Adding lane crashes IE/Edge #746

philippfromme opened this issue Jan 3, 2018 · 20 comments
Assignees
Labels
browser:Edge browser:IE bug Something isn't working channel:support good first issue Good for newcomers help wanted Extra attention is needed pr welcome We rely on a community contribution to improve this.
Milestone

Comments

@philippfromme
Copy link
Contributor

philippfromme commented Jan 3, 2018

Adding a lane can result in either an empty canvas or IE/Edge crashing.

Forum: https://forum.bpmn.io/t/microsoft-edge-ie11-crahes-when-adding-a-lane/1968


Related to SUPPORT-5815

@philippfromme philippfromme added browser:Edge browser:IE bug Something isn't working labels Jan 3, 2018
@nikku nikku added help wanted Extra attention is needed backlog Queued in backlog pr welcome We rely on a community contribution to improve this. labels Apr 19, 2018
@nikku
Copy link
Member

nikku commented Sep 24, 2018

Probably related to this forum report.

@nikku nikku added the good first issue Good for newcomers label Sep 24, 2018
@gustavjf
Copy link
Contributor

gustavjf commented Jan 23, 2019

I just had a look at this and wasn't able to reproduce on IE11 or Edge 17. I can reproduce on IE11 but not on Edge 17. See attached gifs below.

As a sidenote, I noticed opening demo.bpmn.io on IE11 leads to a blank canvas. The console shows a syntax error related to unsupported class syntax usage in bpmn-js-signavio-compat.

bpmn modeler _ demo bpmn io - internet explorer 1_23_2019 2_14_02 pm

bpmn modeler _ demo bpmn io - internet explorer 1_23_2019 2_16_15 pm

On Edge using the provided diagram example in the forum post, I can add lanes above and below without experiencing a crash and without the canvas being empty.

Lane above:
issue-746-edge-repro_lane-above

Lane below:
issue-746-edge-repro_lane-below

Not shown in the gifs, but I tried convulsively adding lanes above and below - no crash or empty canvas.

I can look into the IE11 case.

@nikku
Copy link
Member

nikku commented Jan 23, 2019

As a sidenote, I noticed opening demo.bpmn.io on IE11 leads to a blank canvas. The console shows a syntax error related to unsupported class syntax usage in bpmn-js-signavio-compat.

This is about to be fixed in the update demo.

I just had a look at this and wasn't able to reproduce on IE11 or Edge 17. I can reproduce on IE11 but not on Edge 17.

I cannot reproduce any of the issues on IE11. I tried:

  • Example diagram, opening, adding lane above / below
  • Creating empty participant with (or without) lanes; updating lane / participant labels

Which one exactly can you still reproduce?

@gustavjf
Copy link
Contributor

gustavjf commented Jan 24, 2019

I can reproduce both locally using the modeler example in bpmn-js-examples.

I tried to do so on demo.bpmn.io after the syntax error fix you mentioned went live, but it doesn't happen there.

One thing, sometimes I couldn't reproduce either bug depending on the amount of open tabs I had with the modeler example running. I don't know why. If you aren't able to reproduce, make sure you have a single tab open in IE11 with the modeler example running and then follow the steps below. Reproduction is consistent for me this way. My mistake, turns out reproduction is sometimes inconsistent when following the steps below. Thankfully, I can reproduce both cases about 90% of the time but other times there is no error thrown when adding a lane or changing the participant name.

For the first case (Example diagram, opening, adding lane above / below):

  1. Run modeler example in bpmn-js-examples.
  2. Drag Titel.bpmn diagram file to viewport.
  3. Once diagram has rendered, click on a lane.
  4. Add lane below/above.
  5. Observe participant element and its children disappear from canvas.

The stack trace is as reported in the forum post you linked above.

Note I was using 'eval-source-map' for webpack devtool so line numbers are different but it's the same stack:

unhandled error in event listener
Error: unspecified error.
  at prependTo (eval code:220:3)
  at Anonymous function (eval code:152:7)
  at forEach (eval code:202:7)
  at Anonymous function (eval code:149:5)
  at forEach (eval code:202:7)
  at GraphicsFactory.prototype.updateContainments (eval code:139:3)
  at Anonymous function (eval code:51:5)
  at invokeFunction (eval code:506:3)
  at EventBus.prototype._invokeListener (eval code:359:5)
  at EventBus.prototype._invokeListeners (eval code:345:5)
SCRIPT16389: Unspecified error.
EventBus.js (379,1)

For the second case (editing the participant name in a newly created diagram), I simplified the steps from the forum post a bit:

  1. Run modeler example in bpmn-js-examples.
  2. Create a new diagram.
  3. Create a participant.
  4. Create a task inside the participant.
  5. Edit the task name with a single character. (I used s). Press enter.
  6. Double click the participant name area on the left-hand side.
  7. Enter any character then press enter. (I used s).
  8. Observe the participant element disappears along with its children, but the editable participant name area is still there.

Here, the stack trace is the same as that of the first case.

On further interaction, more errors are logged to the console (Unable to get property 'childNodes' of undefined or null reference).

Do any of these steps work for you?

@gustavjf
Copy link
Contributor

I forgot to mention, I didn't see the chinese characters while trying to reproduce these bugs locally but I do see them briefly when refreshing IE11 on https://demo.bpmn.io/new.

@gustavjf
Copy link
Contributor

I spent some more time on this but wasn't able to figure out the cause of the error. Here's what I managed to find:

The stack trace points to a call to Node.insertBefore() here.

The bug is intermittent. Sometimes the repro steps work, sometimes they don't. Maybe an async issue?

The unspecified error's code is 16389. While searching for that, I noticed it seems to be a generic error that IE throws whenever it encounters some JS code it doesn't like.

That same error is thrown when I attempt to call getBoundingClientRect() on an element which is not yet present in the DOM:

try {
  var element = document.createElement('div');
  element.getBoundingClientRect();
} catch (e) {
  console.log(e.number & 0xFFFF) // 16389
}
// https:///developer.mozilla.org/en-US/docs/Web/JavaScript/Microsoft_Extensions/Error.number#Example

So then I thought perhaps it's because one of the elements passed as arguments to insertBefore() was not yet present in the DOM?

// Before error is thrown, paused execution before call to parentNode.insertBefore(...)
document.body.contains(newNode) // true, which is odd considering insertBefore hasn't yet been called
document.body.contains(parentNode) // true, makes sense
document.body.contains(parentNode.firstChild) // true, makes sense

That doesn't seem to be the problem.

// After error is thrown, paused execution after call to parentNode.insertBefore(...)
document.body.contains(newNode) // false
document.body.contains(parentNode) // true
document.body.contains(parentNode.firstChild) // false

My searches for insertBefore and unspecified error 16389 didn't give me much to go on.

Maybe what I've found is of some use and I've overlooked something.

FWIW, Here's a gif of the error following the repro steps of the second case:

issue-746-ie11-repro_label-change

nikku added a commit to bpmn-io/diagram-js that referenced this issue Jan 28, 2019
We must ensure that we default to <null> as the reference node
when using Node#insertBefore in our code base.

Cf. https://developer.mozilla.org/en-US/docs/Web/API/Node/insertBefore

This may be related to bpmn-io/bpmn-js#746
@nikku
Copy link
Member

nikku commented Jan 28, 2019

The stack trace points to a call to Node.insertBefore() here.

This may be related to the fact that we don't explicitly pass null for non-existing reference nodes, as we should.

@gustavjf
Copy link
Contributor

Right, that was another suspicion of mine but IE11 just inserts the newNode to the end of the child list if the second parameter isn't passed, just as if null were passed. There is no error thrown, compared to Chrome or FF.

Here's a jsbin snippet demonstrating that.

@nikku
Copy link
Member

nikku commented Jan 28, 2019

Forget my previous comment; I reverted it as Node#firstChild is null, if not given, cross browser.

😢

nikku added a commit to bpmn-io/diagram-js that referenced this issue Jan 28, 2019
The check is unnecessary, as `Node#firstChild` defaults
to null as per spec.

Related to bpmn-io/bpmn-js#746
@nikku
Copy link
Member

nikku commented Jan 28, 2019

Good news is that I can reliably reproduce the issue you describe in your previous comment on my IE 11 (inside a VM)!

@gustavjf
Copy link
Contributor

Wonderful! Glad to hear it.

@nikku nikku changed the title IE/Edge: Adding lane crashes IE/Edge Adding lane crashes IE/Edge Jan 28, 2019
@nikku
Copy link
Member

nikku commented Feb 9, 2019

We applied a MS IE / Edge fix just recently that may circumvent this issue.

@gustavjf
Copy link
Contributor

gustavjf commented Feb 9, 2019

Thanks for the tip - I just reinstalled the modeler example's dependencies with bpmn-js at 3.2.1 but unfortunately I can still reproduce the issue in IE11 when changing the participant label.

@nikku
Copy link
Member

nikku commented Feb 9, 2019

Did you update diagram-js, too?

@gustavjf
Copy link
Contributor

gustavjf commented Feb 9, 2019

I did, yes. It's at 3.1.3.

@philippfromme
Copy link
Contributor Author

philippfromme commented May 21, 2019

It's very likely that this is an issue with the second parameter of #insertBefore being undefined.

As MDN states:

referenceNode is not an optional parameter -- you must explicitly pass a Node or null. Failing to provide it or passing invalid values may behave differently in different browser versions.

Source: https://developer.mozilla.org/en-US/docs/Web/API/Node/insertBefore

@nikku
Copy link
Member

nikku commented May 21, 2019

Which second parameter do you refer to? Which usage of #insertBefore?

@philippfromme
Copy link
Contributor Author

var insertedNode = parentNode.insertBefore(newNode, referenceNode);

I'm referring to referenceNode. insertBefore is used in https://github.com/bpmn-io/diagram-js/blob/master/lib/core/GraphicsFactory.js#L237.

@nikku nikku added the PM label Jun 11, 2019
@nikku nikku removed the PM label Jun 11, 2019
@nikku nikku added this to the M30 milestone Jun 11, 2019
@nikku nikku added ready Ready to be worked on and removed backlog Queued in backlog labels Jul 8, 2019 — with bpmn-io-tasks
@nikku nikku added in progress Currently worked on and removed ready Ready to be worked on labels Jul 8, 2019 — with bpmn-io-tasks
@philippfromme philippfromme added needs review Review pending and removed in progress Currently worked on labels Jul 8, 2019 — with bpmn-io-tasks
nikku pushed a commit to bpmn-io/diagram-js that referenced this issue Jul 8, 2019
nikku pushed a commit to bpmn-io/diagram-js that referenced this issue Jul 8, 2019
nikku added a commit that referenced this issue Jul 8, 2019
@nikku
Copy link
Member

nikku commented Jul 8, 2019

Closed via 202eff5

@nikku nikku closed this as completed Jul 8, 2019
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jul 8, 2019
@nikku
Copy link
Member

nikku commented Jul 8, 2019

Released as v4.0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser:Edge browser:IE bug Something isn't working channel:support good first issue Good for newcomers help wanted Extra attention is needed pr welcome We rely on a community contribution to improve this.
Development

No branches or pull requests

4 participants