HeinrichApfelmus / threepenny-gui

GUI framework that uses the web browser as a display.
https://heinrichapfelmus.github.io/threepenny-gui/
Other
437 stars 77 forks source link

Mousedown event in SVG "crashes" Threepenny #238

Closed brprice closed 4 years ago

brprice commented 4 years ago

The symptom

Consider the code

import Graphics.UI.Threepenny as UI
import qualified Graphics.UI.Threepenny.SVG as SVG

main :: IO ()
main = startGUI defaultConfig setup

setup :: Window -> UI ()
setup w = do
  pure w # set title "Buggy event position in SVG"
  elOut <- textarea
  elOk <- UI.div #+ [string "non-svg mousedown is ok"]
  elBad <- SVG.text # set html "svg mousedown is bad" # set SVG.x "10" # set SVG.y "50"
  elSVG <- SVG.svg # set SVG.viewBox "0,0,100,100" # set SVG.width "300" # set SVG.height "100" #+ [pure elBad]
  getBody w #+ [grid [[pure elOk, pure elSVG], [string "Event information:", pure elOut]]]

  on mousedown elOk $ \p -> pure elOut # set text (show p)
  on mousedown elBad $ \p -> pure elOut # set text (show p)  -- crashes as fails to parse data as a pair of ints
  --on (domEvent "mousedown") elBad $ \p -> pure elOut # set text (show p) -- look at the EventData
  pure ()

Running this we find that clicking on the "non-svg mousedown is ok" text behaves well: it dumps the click location into the textbox. However clicking on the "svg mousedown is bad" behaves poorly: instead of doing the same as above, "crashes" threepenny. By this I mean that it reloads the browser window and we see in the console

Foreign.JavaScript: Browser window disconnected.
src/Graphics/UI/Threepenny/Internal.hs:190:24-55: Non-exhaustive patterns in Success y

The problem

Assuming that on mousedown is supposed to work within an SVG (if not, what is the alternative?), the problem is that (https://github.com/HeinrichApfelmus/threepenny-gui/blob/master/src/Graphics/UI/Threepenny/Events.hs#L64)

readCoordinates :: EventData -> (Int,Int)
readCoordinates json = (x,y)
    where [x,y] = unsafeFromJSON json

-- | Mouse down event.
-- The mouse coordinates are relative to the element.
mousedown :: Element -> Event (Int,Int)
mousedown = fmap readCoordinates . domEvent "mousedown"

assumes that the EventData are integers, which is false.

To see it is false, comment out the second to last line in the example code and uncomment the penultimate line to see the raw event data.

The workaround

One can work around this simply by writing a version of readCoordinates which returns (Float,Float).

Depending on how we want this event (and also mousedown and mouseup) to work, it would be good to either

However, I am unconvinced that this is the correct solution, since the raw javascript event handler seems to only have integers. Unfortunately I don't understand how threepenny-gui calculates its result (presumably) from these raw integers.

sjakobi commented 4 years ago

Many thanks for the report and the great reproducer @brprice! :)

(For my own reference) I built it within the threepenny-gui directory, with

$ cabal build -w ghc-8.8.3
$ cabal exec -w ghc-8.8.3 -- ghc issue238.hs

Interestingly I wasn't able to reproduce the issue with Chromium (v81.0.4044.122). With Firefox (v75.0) I get the buggy behaviour you reported!

Which browser did you test this with @brprice?

I've had a look at https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent. The various coordinates included there (I'm not quite sure which ones we actually use!) are documented to be doubles now, but they previously were longs! See e.g. https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/clientX.

To me, this suggests that we should change readCoordinates to produce Doubles. It would be nice to do this in a backwards-compatible way of course, but I don't quite see how…

@HeinrichApfelmus Thoughts?

brprice commented 4 years ago

Tested with Firefox 76.0 (sorry - I really should have included this info up front!).

To clarify what I meant by the raw javascript returning integers: running

document.getElementsByTagName("svg")[0].children[0].addEventListener("mousedown",(e) => {console.debug(e)})

in the debug console to attach an event to the SVG text, we then see something like the following in the console:

mousedown { target: text, buttons: 1, clientX: 471, clientY: 58, layerX: 471, layerY: 58 }

compared with the EventData from threepenny-gui which gives

Array [Number 146.2833251953125,Number 16.0]

So I'm not sure why the EventData is not actually integral in this case (I don't follow how it gets computed from presumably the same underlying data).

Thanks for digging through the mozilla docs -- I was unaware that they specified coordinates to be doubles now - this makes the idea of changing the type of readCoordinates more palatable! (Though there is the issue of backwards compatability...)

HeinrichApfelmus commented 4 years ago

Thanks for the report! Crashes happen at runtime when types change from long to double and there is no type checker to catch it. 😅

I think we should reflect the type change in the web standards and change the return type of readCoordinates to (Double,Double). Just changing the types is enough, unsafeFromJSON takes care of the rest.

Two questions for @brprice :

https://github.com/HeinrichApfelmus/threepenny-gui/blob/a5997e91060c09b881ede331e15c522bcc1943e4/js/lib.js#L40

sjakobi commented 4 years ago

Regarding standards: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent#Specifications seems to indicate that the latest official specification of MouseEvent (which still used long coordinates) is obsolete, and that various more recent "working drafts" use double.

I'm in favour of changing readCoordinates to return Doubles then. For convenience we could add e.g. readCoordinates' with the old type.

@brprice Would you be interested in making a PR?

brprice commented 4 years ago

@HeinrichApfelmus

@sjakobi : sure! I'll try to get something in tomorrow.

brprice commented 4 years ago

For the record, I have dug into lib.js and worked out why threepenny-gui was returning doubles to Haskell (which were then choked on) even though Firefox was giving integers as pageX etc.

This happens because we give the mousedown event data as relative to the element, and the element's offset is fractional even though the mouse position is integral. Specifically https://github.com/HeinrichApfelmus/threepenny-gui/blob/a5997e91060c09b881ede331e15c522bcc1943e4/js/lib.js#L39 this jQuery returns fractional data for the SVG element but not the plain html one.

This is documented at https://api.jquery.com/offset/ under "Additional Notes".

brprice commented 4 years ago

Closing because this was fixed in cf147cd and released with 0.9.0.0 (#242).

@sjakobi, @HeinrichApfelmus : thanks for getting this out so quickly!