`onClick` is firing twice when elements are unwrapped

[hecrj]

I have found an unexpected behaviour with the onClick attribute.

Say we define elements like this:

elements =
    div []
        [ div [ onClick A ] [ text "Element A" ]
        ]

And our view returns div [] [ elements ]. Now, if clicking Element A causes the view to return elements (i.e. unwrapped) the message A is fired again.

SSCCE: https://gist.github.com/hecrj/20da8c7bad9963247227a18c6287cd8f

Here is a GIF that shows the issue:

The Unwrap message is fired twice when the text is clicked only once.

Some more details:

• I can confirm that this doesn't occur in Elm 0.18: https://ellie-app.com/pwmXRcQrxa1/0
• It happens with and without the --debug flag.
• In order to reproduce the issue, compile the SSCCE with: elm make src/OnClickBug.elm --output=index.html and open the produced index.html with Chrome.
• I first discovered this issue when implementing a collapsible tab menu.

[evancz]

This is related to switching away from requestAnimationFrame for user inputs, so it is the same root issue as observed in https://github.com/elm/virtual-dom/issues/126

The DOM change is made while the event is still bubbling, so it triggers a second time.

[evancz]

Current Fix

I changed the type of Handler in elm/virtual-dom to:

type Handler msg
  = Normal (Json.Decoder msg)
  | MayStopPropagation (Json.Decoder (msg, Bool))
  | MayPreventDefault (Json.Decoder (msg, Bool))
  | Custom (Json.Decoder { message : msg, stopPropagation : Bool, preventDefault : Bool })

Notice that the ability to specify Sync or Async is no longer available. Instead, everything renders with requestAnimationFrame by default (async) and if you stop propagation it will be rendered immediately (sync)

The root problem in this issue is that the DOM changes while the event is still bubbling, leading to an event from the past interacting with the future UI. Now this is impossible because the render only happens immediately if the event is no longer propagating.

So onInput will stop propagation, while the others will go back to being async.

Future Possibility

If we would like to recover full control over sync or async renders, it is possible, but requires some significant changes to the virtual-dom implementation.

When an event is noticed, we immediately turn it to a Msg, run update, send out port messages. We then look at whether the user wants a sync or async render:

• If async, we are done.
• If sync, we check on stop propagation:
  • if stopped, we render immediately
  • if not, we need another listener on the root node that runs render when it gets the event

There are various optimizations that can be done to use fewer event listeners in this scheme.

It is not clear if this will actually be needed, but it appears to be possible.

[evancz]

If you have questions or comments about this, please share them on https://discourse.elm-lang.org/ or http://elmlang.herokuapp.com/

Folks there can help you figure out if it should be upgraded to the issue tracker.