Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 5 commits into from

Conversation

kjarosh
Copy link
Member

@kjarosh kjarosh commented Dec 18, 2024

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.

@kjarosh kjarosh added text Issues relating to text rendering/input A-core Area: Core player, where no other category fits T-fix Type: Bug fix (in something that's supposed to work already) labels Dec 18, 2024
@adrian17
Copy link
Collaborator

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
Copy link
Member Author

kjarosh commented Dec 18, 2024

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
Copy link
Collaborator

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
Copy link
Member Author

kjarosh commented Dec 18, 2024 •
edited
Loading

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
Copy link
Collaborator

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
adrian17 requested changes Dec 18, 2024
View reviewed changes
Copy link
Collaborator

@adrian17 adrian17 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kjarosh
Copy link
Member Author

kjarosh commented Dec 18, 2024

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
text: Make HTML p's align attribute case insensitive
9c0afd9 
This fixes issues with setting align to a value that isn't lowercase.
@kjarosh
tests: Update avm2/edittext_html test
5d545cb 
Added cases related to case sensitivity of p's align attribute.
@kjarosh
text: Skip setting HTML text when it hasn't changed
e375fec 
This behavior not only prevents text relayout,
but it also has observable effects, because not
every set of spans is representable as HTML.
@kjarosh
text: Skip setting text when it hasn't changed
9ca2b00 
This behavior not only prevents text relayout,
but it also has observable effects, because text
format is not being reset to the default format.
@kjarosh
tests: Add avm2/edittext_set_html_same test
71405d4 
This test verifies the behavior of htmlText when
setting to the same value.
@kjarosh kjarosh requested a review from adrian17 December 19, 2024 19:54
@kjarosh
Copy link
Member Author

kjarosh commented Dec 19, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrian17 adrian17 Awaiting requested review from adrian17

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
A-core Area: Core player, where no other category fits T-fix Type: Bug fix (in something that's supposed to work already) text Issues relating to text rendering/input
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(NSFW) Animation #6 - Shade - Can only type three numerical numbers instead of four
2 participants
@kjarosh @adrian17