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.

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

adjacent tts:textCombine spans are combined into a single span #232

Closed
btsimonh opened this issue Jul 14, 2022 · 3 comments · Fixed by #239
Closed

adjacent tts:textCombine spans are combined into a single span #232

btsimonh opened this issue Jul 14, 2022 · 3 comments · Fixed by #239

Comments

@btsimonh
Copy link
Contributor

btsimonh commented Jul 14, 2022

When there are two adjacent spans with tts:textCombine=all, they are combined into a single span containing the text from both.
when debugging, it seems that _isd_element is undefined on both spans (not sure why), and so are equal.

It would seem prudent to replace (in html.js):

        if (first.tagName === "SPAN" &&
            second.tagName === "SPAN" &&
            first._isd_element === second._isd_element) {

with

        if (first.tagName === "SPAN" &&
            second.tagName === "SPAN" &&
            first._isd_element &&
            first._isd_element === second._isd_element) {

this may mean there are other times when spans are combined when they should not be?

@btsimonh
Copy link
Contributor Author

an alternative or additional fix would be

            if (imscStyles.byName.textCombine.qname in isd_element.styleAttrs &&
                    isd_element.styleAttrs[imscStyles.byName.textCombine.qname] === "all") {

                /* ignore tate-chu-yoku since line break cannot happen within */
                e.textContent = isd_element.text;
                e._isd_element = isd_element;

@nigelmegitt
Copy link
Contributor

Interesting. I'm fairly certain that I didn't test what would happen with textCombine when we submitted this code. Thanks for catching this. On first glance we should only need the second fix, and the first fix masks the problem. Will comment on the pull request. Wonder what @palemieux thinks about this?

@palemieux
Copy link
Contributor

if (first.tagName === "SPAN" &&
            second.tagName === "SPAN" &&
            first._isd_element &&
            first._isd_element === second._isd_element) {

This feels most natural since _isd_element is described as enabling span merging, and we do not want tts:textCombine to be span-merged.

palemieux added a commit that referenced this issue Nov 21, 2022
Co-authored-by: Nigel Megitt <nigel.megitt@bbc.co.uk>
Co-authored-by: Simon Hailes <simon@yellaumbrella.tv>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants