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

Color of text-decoration shouldn't be changed by children #417

Open
johnbeard opened this issue Feb 4, 2021 · 12 comments
Open

Color of text-decoration shouldn't be changed by children #417

johnbeard opened this issue Feb 4, 2021 · 12 comments

Comments

@johnbeard
Copy link

johnbeard commented Feb 4, 2021

From the CSS Level 1 spec: (https://www.w3.org/TR/CSS1/#text-decoration)

This property is not inherited, but elements should match their parent. E.g., if an element is underlined, the line should span the child elements. The color of the underlining will remain the same even if descendant elements have different 'color' values.

For example:

  Before <span style="color:red; text-decoration:underline;">Underlined Red <span style="color:blue;">Blue</span> text</span> After

Should look like this:

Sigil preview

However, in crengine, the text decoration takes the child element's color:

Koreader rendering

EPUB with the above:
text-decoration-color-test.epub.zip

@poire-z
Copy link
Contributor

poire-z commented Feb 4, 2021

Given the way our text drawing is implemented, this is very complicated to solve:

  • color/bgcolor is set on the buffer object when drawing, it would need to have a underlinecolor added
  • when we manipulate text fragments, we're far from CSS: we propagate text drawing flags to child element without any notion of color or from which parent the text-decoration originates. - so it would need this and added slots to store the underline color to be passed down.

So, for me, looks like too much kludged needed for this little minor issue (and well, on eInk I wouldn't have noticed :)
We have other limitations on inline contents, like no margin/padding, background color also has issues... so, would probably better be handled as part of a rework of all that (but I have no plan to rework all that :)

@johnbeard
Copy link
Author

This is very complicated to solve:

Fair enough

and well, on eInk I wouldn't have noticed :)

True for the red/blue case above, but the real motivation is because it'll go badly wrong if the child sets color: transparent; (see #418).

@Frenzie
Copy link
Member

Frenzie commented Feb 4, 2021

On that note, what happens if you set visibility: hidden?

@poire-z
Copy link
Contributor

poire-z commented Feb 4, 2021

We don't yet support visibility: - but it will be added if/when we add MathML support.
edit: I mean I added support for it in my MathML w.i.p. branch - could of course be added even before MathML goes in.

@johnbeard
Copy link
Author

johnbeard commented Feb 4, 2021

@Frenzie: the text-decoration should be hidden in that case.

https://www.w3.org/TR/css-text-decor-3/#line-decoration

The visibility property, text-shadow, filters, and other graphical transformations likewise affect text decorations as part of the text they’re drawn on, even if the decorations were specified on an ancestor box, and do not affect the calculation of their initial positions or thicknesses.

<p>Before <span style="color:red; text-decoration:underline;">Underlined Red <span style="visibility:hidden;">Hidden</span> text</span> After</p>

Sigil/Firefox/Chrome:
Hidden

This doesn't work at all in Koreader:
No hiding in Koreader

@poire-z
Copy link
Contributor

poire-z commented Feb 4, 2021

Btw, what's your use cases for this and #418 ?
I guess we're quite unlikely to find all that in books.

@johnbeard
Copy link
Author

I found some books using

<span style="text-decoration:line-through;">
  <span style="color:transparent;">&#8212;&#8212;&#8212;</span>
</span>

(&#8212; is an em-dash) to simulate a "long dash" that degrades to a number of em-dashes on copy-paste.

@Frenzie
Copy link
Member

Frenzie commented Feb 4, 2021

@johnbeard Very much not my question. ;-) Assume I know what should happen or where to find it, but that I don't want to or can't create a testcase to load in crengine right now. :-P

@johnbeard
Copy link
Author

@Frenzie is this the answer to your question?
transparent-color.epub.zip

@Frenzie
Copy link
Member

Frenzie commented Feb 4, 2021 via email

@poire-z
Copy link
Contributor

poire-z commented Feb 7, 2021

Was giving this some thoughts, and thought that for such case, we might just have another flag LTEXT_HAS_COMPLEX_STUFF so when seeing that in a few limited places, the text rendering/drawing code could not trust the flags and go looking at the node hierarchy and their styles (which the current drawing code does not yet have to do), so it can see from where the text-decoration originates, and pick the decoration color from there. And have buf->SetTextDecorationColor() where we already have buf->SetTextColor() and buf->SetBackgroundColor().

But that would solve only the color issue when there is a single text decoration - because we could have underline from a red parent, and strikethough from a blue grandparent :/
And we can even have 2 underlines, here if both the A and the SUP have it:
image.

Also, from https://www.w3.org/TR/CSS2/text.html#lining-striking-props:

Some user agents have implemented text-decoration by propagating the decoration to the descendant elements as opposed to preserving a constant thickness and line position as described above. This was arguably allowed by the looser wording in CSS2. SVG1, CSS1-only, and CSS2-only user agents may implement the older model and still claim conformance to this part of CSS 2.1. (This does not apply to UAs developed after this specification was released.)

crengine indeed has implemented text-decoration by propagating the decoration to the descendant elements. It's somehow OK in real-life. But re-working it all for CSS2.1++ conformance seems like a heavy task for little real-life benefit, so I'd rather not take the risk.

The hack above would just solve the color issue, and not much else - so I guess I can drop the idea.
(I may still reconsider such a flag for rare use cases, like padding/margin on a inline element, if I even want to go fix that.)

#418 is doable #418 (comment) by just picking and passing the bg color as the fg color.
But it won't solve the issue with this publisher hack :) where the publisher could have used one of the japanese superlong dashes (I think I've read about that, may be there wasn't any in Unicode, and some tricks have to be used...)

@johnbeard
Copy link
Author

And we can even have 2 underlines, here if both the A and the SUP have it:

You can even have lots of decorations all at once (on both the A and SUP) 😱:

<a style="text-decoration: underline overline;">To be
  <sup style="text-decoration: line-through; text-decoration-style: double;">or not to be?</sup>
 That is the question</a>

Firefox:
2021-02-10_100243_257x24_screenshot

Chromium:
2021-02-10_100141_235x27_screenshot

Koreader actually gets the superimposition aspect of the A and SUP decorations right:
2021-02-10_100430_487x55_screenshot

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

No branches or pull requests

3 participants