sandflow / imscJS

JavaScript library for rendering IMSC Text and Image Profile documents to HTML5
BSD 2-Clause "Simplified" License
84 stars 31 forks source link

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

Closed btsimonh closed 1 year ago

btsimonh commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago
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.