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

Create span instead of rb elements #238

Merged
merged 3 commits into from
Nov 21, 2022
Merged

Conversation

nigelmegitt
Copy link
Contributor

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.

Closes sandflow#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 `span`s. 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
Copy link
Contributor Author

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.

Correctly puts the background-color attribute on merged spans.
@nigelmegitt
Copy link
Contributor Author

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

@nigelmegitt
Copy link
Contributor Author

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[]

src/main/js/html.js Outdated Show resolved Hide resolved
@palemieux
Copy link
Contributor

@nigelmegitt @btsimonh See resulting render at w3c/imsc-tests#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.

Co-authored-by: Pierre-Anthony Lemieux <pal@sandflow.com>
@nigelmegitt
Copy link
Contributor Author

nigelmegitt commented Nov 21, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

nigelmegitt commented Nov 21, 2022

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

image

@btsimonh
Copy link
Contributor

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
Copy link
Contributor Author

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.

@palemieux palemieux merged commit 7716bcc into sandflow:master Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

if a line starts/ends with a ruby definition, line padding is not applied
3 participants