-
Notifications
You must be signed in to change notification settings - Fork 31
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
Bugfix - ruby+linepadding #234
Conversation
…and no need as it can't wrap.
…ack off to the parent RUBY. Fixes end padding when the line starts or ends with ruby.
linepadding001: simplest case, 2 spans with BR between linepadding002: first line ends with ruby. Illustrates redner issue resolved in sandflow/imscJS#234 linepadding003: nested spans, ruby spans with no ruby text, ruby wrapped onto next line. linepadding004: Simple wrapped span.
@btsimonh Can you provide a test? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide a test file and create an issue?
see imsc-tests PRs. |
@nigelmegitt It looks like getSpanAncestorColor() does not pick up the backgroundColor specified on the
|
@palemieux The spec (pdf, see Appendix D) for In particular I'd be concerned that the layout requirements are unclear. Should line padding cause adjacent rubys applied to different ruby base text to be spaced apart further? What if that would cause a line break? I don't think this is straightforward at all. |
The below sample file illustrates various boxing with Ruby. In the test above, @palemieux has boxed the ruby base; in this case even with the patch, the ruby container has been padded, but it's background is transparent, and so the padding not shown. We can't duplicate the color from the ruby text to the ruby container, else we end up with (6) below - undesirable for semi-transparent boxing. We could pad both the ruby container and the ruby base... The other thing I'm not sure about is if a ruby container's height should be the size of the base and text, or the base only. For background color render in Chrome it's the height of the base span. For other purposes, it's the combined height? In the below, speaking from a personal 'what would look good' perspective, (3) should get line padding either side of the text '0.25em'. For me, this would satisfy the line padding intent. @nigelmegitt - if line padding from two ruby texts were to conflict, they should not change the position of anything - just like line padding currently is only visual, an has no impact on line length (or does it?)? Edit - I see it DOES impact line length. Would have been nicer if it did not :(. However, in terms of the constructs, (1) and (4) make most sense to me, the others produce undesirable results in browsers (box overlaps), and are more complex to author. I tried really hard to understand how to control browser ruby text background presentation, and decided it was variable amongst browsers and likely to change, so not worth further effort for the moment. Another 'boxed' presentation which I do not think can be simply achieved would be a rectangular box which (just) covered both base text and ruby text for the line. I think (5) should probably do this, but does not. Generates: Original renderer: With PR:
|
@btsimonh I'm struggling to see how the renders you pasted are an improvement looking at the ruby? In particular, if we were applying linePadding to ruby then I would expect to see it on the "0.25em" immediately following "3 Padding", but it looks like it is absent both pre- and post-PR. Perhaps I'm missing something... |
@nigelmegitt - the PR only fixes the padding on the base text. i.e. the primary line is padded 'correctly' - now the word 'padding' has padding on the end :). |
Ah, I see it now, thank you @btsimonh - this is an improvement for those test cases. For the line beginning 6, why is the background colour on the right edge of the line grey not black? If we're making a change, we should try to make it fit the spec here. Just an observation: the line beginning 5 seems to have a big problem with the current render, because it should have a background colour, but doesn't. That should be orthogonal to the line padding, but maybe there's a common cause. |
|
This is a bit confusing from a terminology perspective. To pad the outer element (the The right edge of the line area is the bit at the right edge of the word "padding", where the darker colour applies, formed by the multiple application of If the former, the behaviour is correct, but if the latter, then not. This isn't a case that is dealt with in the EBU-TT-D specification, and indeed it does not apply there, because nested Arguably this is something we should consider as some kind of clarification in the IMSC specification, i.e. what to do if there are nested |
We must be able to have colours on separate spans on a line, else you can't change background mid line. The issue of background colours combining does make things difficult, and not ideal :). but nothing to be done. I believe in this case the behaviour (of the patched version) is to spec, extending the background colour of the last span of the line (which happens to be a ruby container). The presence of extra background colour on the ruby text is an example of how not to do it? |
agree. It's meant to mean 'the outer of the two nested spans'. i.e. the (modified) line-padding mechanism has padded the ruby container which has a background on it. And the ruby base has not been padded by line-padding, but also has a background on it, which results in the double application of opacity on the word 'padding', hence the word's background being darker. This points to it being undesirable to apply background with opacity to multiple spans which are children or parents (unless you want that presentation). Interesting; Added one more case (7) to the above, renders OK in pre and post PR:
|
I've opened #238 which takes a different tack in addressing this: I found that the |
Replaced by #238 |
When applying line padding, if the start or end of line element is a span inside an RB element, then instead apply the padding to it's RUBY parent.
Not that this is more of a bandaid than a fix, as nested spans will have the same issue - start and end elements are only taken from leaf nodes, so any enclosing nodes may cut off the padding.
Also addresses background colors and ruby
Also avoids splitting rt text into spans (as they are not recombined).