search

sandflow / imscJS

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

bugfix - textcombine #233

Closed btsimonh closed 1 year ago

btsimonh commented 2 years ago

fixes https://github.com/sandflow/imscJS/issues/232

nigelmegitt commented 1 year ago

Does the test at https://github.com/w3c/imsc-tests/blob/main/imsc1_1/ttml/textCombine/textCombine001.ttml expose this behaviour?

btsimonh commented 1 year ago

@nigelmegitt - gave a PR for imsc-tests to test this specific point. Images need re-rendering....

btsimonh commented 1 year ago

What was wrong with your original proposal

I think @nigelmegitt wanted the code to highlight if _isd_element was not set, because it may clearly be an error? Hence the move of the test - but no logging in that fn, so console.error is the best I could do without a lot more mods?

palemieux commented 1 year ago

I think @nigelmegitt wanted the code to highlight if _isd_element was not set, because it may clearly be an error?

AFAIK _isd_element is set only on elements that are available for merging. Wasn't that the intent @nigelmegitt ?

nigelmegitt commented 1 year ago

@palemieux Yes that was the intent, but I think we did not have test cases for textCombine and the code goes through a different path in that case.

It should not be the case that any elements that are candidates for merging do not have _isd_element so if that does happen we should have some mechanism for identifying it, like raising a console warning; at the same time we should fix those cases, and the elements that go through the textCombine path are one of the cases to be fixed, as I understand it.

palemieux commented 1 year ago

textCombine should not have _isd_element since it is not subject to span-per-character processing.

For the purpose of flagging things that should have _isd_element but do not have it, how would one differentiate an element that should have it but does not vs. an element that does not have because it should not have it.

btsimonh commented 1 year ago

can you highlight a case of 'should not have it' where the recombine function would get called?

palemieux commented 1 year ago

Replaced by #239