conditional addClass can wrongly destroy other addClasses

Quelklef commented 2 years ago

Our model has two states (≅ bool). In the initial state, we give a <p> element a class to make the text underlined. When we switch to the second state, we additionally give it a class to make the text bold. When we switch back to the initial state, we remove the class to make the text bold. The class to make the text underlined wrongly also disappears.

https://user-images.githubusercontent.com/14851215/150645856-bffe5704-dd62-4645-a4f6-dbf08f308211.mp4

MWE:

<!doctype html>
<html>
<head>
  <style>
    .one { text-decoration: underline; }
    .two { font-weight: bold; }
  </style>
  <script defer src='index.js'></script>
</head>
<body>
</body>
</html>

module Test.Main (main) where

import Prelude
import Attribute as A
import Data.Monoid (guard)
import Html (Html)
import Html as H
import Platform (Program)
import Platform as Platform

type Model = Boolean
type Msg = Model

main :: Program Unit Model Msg
main =
  Platform.app
    { init: \_ -> pure false
    , update: \_ msg -> pure msg
    , subscriptions: \_ -> mempty
    , view
    }

view :: Model -> { head :: Array (Html Msg), body :: Array (Html Msg) }
view model =
  { head: [ ]
  , body:
      [ H.p
        [ guard model $ A.addClass "two"
        , A.addClass "one"
        ]
        [ H.text "u will not escape from me this time, batman!!" ]
      , H.p
        [ ]
        [ H.button [ A.onClick (not model) ] [ H.text "flip" ]
        , H.text " "
        , H.text $ show model
        ]
      ]
  }

Note that if the order of the two addClasses are reversed, the bug ceases to manifest.

Quelklef commented 2 years ago

Workaround: put the conditional addClass after the unconditional one

ursi commented 2 years ago

I think I have spotted the cause, and it's with the diffing algorithm. This is what I think is happening.

      old      |      new      |
---------------+---------------|
addClass "two" | addClass "one"| compare: remove class "two", add class "one"
addClass "one" |               | compare: remove class "one"

Quelklef commented 2 years ago

Oh, that's subtle. This seems to suggest that the issue could affect other kinds of attributes, as well, such as conditionally attaching an event listener.

Unclear to me the proper solution. What jumps to mind is to reify attribute collections as Sets instead of Lists; set subtraction will then give you which attributes need to be added/removed.

But attributes don't commute, so jumping to a set is not correct.

Perhaps just do the same but with list differences? I don't know if there's an algorithm for that which is better than 2nm steps, but maybe that's acceptable since usually there aren't that many attributes on a node.

ursi / purescript-elmish

conditional addClass can wrongly destroy other addClasses #13