text: Skip setting HTML text when it hasn't changed

[kjarosh]

• Fixes (NSFW) Animation #6 - Shade - Can only type three numerical numbers instead of four #19011

Flash Player checks whether we're setting htmlText to a different value before doing it. It not only prevents text relayout, but also has observable effects, because not every set of spans can be represented as HTML.

This is especially affecting input text fields with bound variables, as text propagation will set the value back, and we can produce spans not representable as HTML during input.

Additionally, this PR fixes <p>'s align case sensitivity.

[adrian17]

This is especially affecting input text fields with bound variables, as text propagation will set the value back,

What do you mean? Aren't they supposed to be always synced anyway?

[kjarosh]

This is especially affecting input text fields with bound variables, as text propagation will set the value back,

What do you mean? Aren't they supposed to be always synced anyway?

They are, but due to the fact that HTML cannot represent all possible contents, it gets tricky. I'm talking specifically about the following scenario:

1. The text field has value 1\n (represented as <p align="center">1</p>)
2. The user deletes \n, leaving 1 (represented as <p align="center">1</p>)
3. Now the binding kicks in, it sets the value of the variable
4. The binding synchronizes the value back, setting the text field to <p align="center">1</p>
5. The text field now has value 1\n, which is wrong (it should be 1)

Now the fact that the binding synchronizes the value back doesn't matter, because when you do the same without binding, it behaves the same way (i.e. the value is 1, not 1\n). This case is covered by the test.

Unfortunately, the tests now started to fail, which means I have found yet another bug related to when the relayout takes place (yay! I guess?).

[adrian17]

To be sure, is the full roundtrip ("binding synchronizes the value back") needed, or is it more like "it's simpler this way and doesn't impact any behavior by itself"?

[kjarosh]

To be sure, is the full roundtrip ("binding synchronizes the value back") needed, or is it more like "it's simpler this way and doesn't impact any behavior by itself"?

So first of all, I did it this way not because it's simpler, but because it's correct. The binding has nothing to do with this behavior, it just happens to occur with a binding.

I don't think the roundtrip is needed (but I'm not sure), I think after this PR it will just be a useless string comparison + property lookup. I guess that could be optimized, but I'm not sure if that's worth it.

[adrian17]

So first of all, I did it this way not because it's simpler, but because it's correct.

I wasn't questioning the PR at all, sorry if it seemed that way. Was just making sure about the roundtrip.

[adrian17]

Ah, sorry, reversing the approve, since you mentioned that new bug 😅

[kjarosh]

I wasn't questioning the PR at all, sorry if it seemed that way. Was just making sure about the roundtrip.

No worries! I wasn't thinking that, just wanted to emphasize (for everyone) that the binding thing is not that important here 😄

[kjarosh]

@adrian17 Should be good now, added some more test cases here too