pkamenarsky / replica

A remote virtual DOM library for Haskell
BSD 3-Clause "New" or "Revised" License
139 stars 14 forks source link

Implement SVG support #13

Closed seagreen closed 4 years ago

seagreen commented 4 years ago

From: https://github.com/pkamenarsky/replica/issues/11

seagreen commented 4 years ago

A few thoughts:

(1)

I added a Maybe Namespace field to VNode and VLeaf, which went well. I also added one to Diff, but it looks to me like you can't change the namespace of an element once it's created.

I'm going to try removing that field and instead issuing a Delete and Insert when an element's namespace changes.

(2)

As far as attributes go, I think in both places we call element.setAttribute, we can insert an element.namespaceURI check right beforehand and use that to decide whether or not to use setAttributeNS instead. I'll try that in a followup commit.

(3)

For testing this, would it be helpful for me to start a new concur-replica branch with an added SVG example?

Let me know what you think!

pkamenarsky commented 4 years ago

I'm going to try removing that field and instead issuing a Delete and Insert when an element's namespace changes.

👍

As far as attributes go, I think in both places we call element.setAttribute, we can insert an element.namespaceURI check right beforehand and use that to decide whether or not to use setAttributeNS instead.

👍

Looks pretty good to me. One nitpick: could you add a Just "something" namespace QuickCheck case as well? E.g. something like:

namespace :: Arbitrary (Maybe Text)
namespace = do
    t <- choose (0, 1) :: Gen Int
    case t of
      0 -> pure Nothing
      1 -> pure $ Just "http://www.w3.org/2000/svg"

and then in the Arbitrary VNode instance:

... VNode <$> arbitrary <*> arbitrary <*>  namespace <*> arbitrary

An SVG example in concur-replica would be great!

seagreen commented 4 years ago

Browsers strike again! The current code gives this error when you try to display an SVG with an attribute:

Uncaught DOMException: Failed to execute 'setAttributeNS' on 'Element': 'http://www.w3.org/2000/svg' is an invalid namespace for attributes.

Following the error rabit hole leads me to https://github.com/d3/d3/issues/1935 and https://dom.spec.whatwg.org/#validate-and-extract.

Do you know what we should do here? For one thing how bad would it be to stick with plain setAttribute, since it seems to work even on SVGs? Maybe that would be really bad, I don't know.

pkamenarsky commented 4 years ago

I'd say be pragmatic here and just go with setAttribute, since it seems to work on both Chrome and FF. It should be a relatively easy fix If it turns out to be a problem later.

seagreen commented 4 years ago

I'd say be pragmatic here and just go with setAttribute, since it seems to work on both Chrome and FF. It should be a relatively easy fix If it turns out to be a problem later.

Done. Also removed "WIP" from the PR title, and updated the (now working) concur-replica example PR. Ready for real review-- let me know if you spot anything you'd like improved, however small.

pkamenarsky commented 4 years ago

Thanks, this looks great. Left a comment, other than that LGTM 👍

seagreen commented 4 years ago

Pushed up a new commit, let me know what you think.

pkamenarsky commented 4 years ago

Perfect, thanks for the PR!

seagreen commented 4 years ago

You're welcome. Thank you for shepherding it. I'm hyped to get https://github.com/seagreen/concur-replica-example-2d off of my replica fork.