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

Create `span` instead of `rb` elements #238

Closed nigelmegitt closed 1 year ago

nigelmegitt commented 2 years ago

Closes #237.

Empirically, it appears that browsers (Firefox and Chrome) do not implement layout on the currently deprecated rb element in the same way as on other elements. Specifically, they seem not to extend the padding rectangle to include the padded rectangles of child spans. Replacing the rb elements with span elements works around this.

If at some point rb is un-deprecated, and this feature/bug of layout is improved, this change should be reverted.

nigelmegitt commented 2 years ago

Checking back against the example provided in #237, this may in fact only partially resolve that issue. However it does solve the problem with linePadding caused when the last text on the line is a ruby base.

nigelmegitt commented 2 years ago

2nd commit abff02b addresses the problem of background-color not being propagated to descendants of ruby elements.

nigelmegitt commented 2 years ago

There may be an additional problem, probably should be opened as a separate issue, where the computation of line edges for applyFillLineGap() is incomplete because it does not take into account the presence of ruby text, because ruby text is not (directly) in lineList[]

palemieux commented 2 years ago

@nigelmegitt @btsimonh See resulting render at https://github.com/w3c/imsc-tests/pull/107

Note that backgroundColor as applied to ruby only applied to the base text. Not clear if this is by design or a browser bug.

nigelmegitt commented 1 year ago

Not clear if this is by design or a browser bug.

https://w3c.github.io/csswg-drafts/css-ruby-1/#box-style (similarly on the older TR version) says (last 2 paragraphs):

Neither the margin, padding, border properties nor the any properties that do not apply to inline boxes apply to base containers or annotation containers. Additionally, line-height does not apply to annotation containers.

The UA is not required to support any of the background properties or outline properties, or any other property that illustrates the bounds of the box on ruby base container boxes or ruby annotation container boxes. The UA may implement these boxes simply as abstractions for inheritance and control over the layout of their contents.

(NB the "nor the any" grammar errors in both those paragraphs are correctly quoted)

It is not clear from this what properties apply to ruby containers, but the fact that they are omitted from the lists suggests that the background properties should apply to ruby containers.

Additionally, the section on Line Spacing shows that ruby annotations can overflow the lines, but doesn't say anything about the span children of p elements, and the inline areas that they generate.

However, assuming that ruby text generally behaves like inline text that is positioned outside its inline area parent, it looks like the normal expectation is that the padding area of the parent does not extend to encompass the inline children. This is true for both ruby and span, demonstrated by this Codepen.

TTML2 on the other hand doesn't say anything about this, that I can find.

So my conclusion is that the behaviour introduced in this pull request is by design. ~currently wrong, and I'll revert some of the changes.~ Authors who want background colour on ruby text will need to specify it explicitly.

Edited to say that the behaviour is by design, not wrong.

nigelmegitt commented 1 year ago

Apologies, previous comment edited: the behaviour is by design and I believe that with this pull request the behaviour is correct. I realised that my test setup was confounding my view of the output by doing some BBC-specific behaviours, namely: 1. Pushing backgroundColor down to span elements, enforcing no p-level background colours and 2. Setting the default for fillLineGap to true.

Incidentally, I have verified that setting fillLineGap to true does behave correctly with the changes in this pull request, but that the browser reports (as per the CSS spec) line areas that overlap in the block progression dimension when ruby space is needed or reserved. For semi-opaque background areas, the impact of that is that some areas are darker than expected. It is also the case that the background area of one line can partially obscure the ruby text associated with an adjacent line due to this. In my view this is undesirable, but I do not have a proposal to address it, other than computing all background areas and explicitly drawing them separately to, and prior to, all foreground text.

Rendering of the example from #237 with fillLineGap forced to true: image

Rendering of the same example with fillLineGap default set to false:

image

(renderings from Firefox 106.0.5 on MacOS 11.7.1)

btsimonh commented 1 year ago

I agree that the definition and behaviour of ruby in browsers is not well defined. However, TTML2 is not intended to directly reflect the browser; admittedly it's easier if the expected behaviour is close to that of browsers, but since browser behaviour is not well defined, I think it's up to us to decide what TTML2 could say about the expected presentation, so that TTML2 users could have a reasonable presentation expectation.

@nigelmegitt - what's going on with overlapping lines between 1 & 2 in the first render? looks like 'filllinegap' is always filling 'up' for all three lines.

nigelmegitt commented 1 year ago

@btsimonh yes, I tried to describe the reason why in the paragraph above that first rendering. Unfortunately, I believe it is correct as per CSS, even if undesirable. It matches the Line Spacing section in the CSS Ruby spec.

Authors can do something about this in a couple of possible ways:

  1. Use tts:rubyReserve to effectively increase the line height to accommodate fully on each line space for ruby text, even if no ruby text is actually present on a particular line (the value after seems to work well with this example.
  2. If using fillLineGap then don't set an additional background colour on the ruby text (assuming that the only reason for doing so is to ensure that there is the same background colour on the ruby text as on the rest of the line),
nigelmegitt commented 1 year ago

Render with fillLineGap="true" and tts:rubyReserve="after" on the <p> containing the first lines beginning 1, 2 and 3:

image

btsimonh commented 1 year ago

again, it's not right. The first line has a ruby above, has reserved below, but has doubled the space above? and 3 is not covering down....

nigelmegitt commented 1 year ago

Yeah, I'm not sure how the rubyReserve is implemented, I see what you mean @btsimonh - I can't spend more time on this now, but I wonder if we need to return to the fact that ruby text is not (directly) included in lineList - it should clearly be taken into account when calculating actual line edges.

I'm not sure if what we're seeing is an artefact of how the edges are discovered when applying fillLineGap or how the line space is reserved when applying rubyReserve... Either way, for the issue with linePadding as described at #237, I believe this pull request does fix it correctly in all the cases.