Prevent Table Elements Shading Style from Parent Styles Cascading

[AyUsH18102001]

Problem Statement:

The html content provided is from TinyMCE editor, this content is referenced from Word/Outlook like source, hence we have this white background parent/wrapper divs for the copied content

<div style="background-color:#ffffff">
  <div style="background-color:#ffffff">
    <p>before</p>
    <table border="1"><tr><td>cell</td></tr></table>
  </div>
</div>

the table run elements inherit the shading value of "#fff", this is due to the cascading of parent styles onto childrens

This behaviour is not what we desire in and its also stated in TableExpressionBase ComposeStyles method i.e.

protected override void ComposeStyles(ParsingContext context)
{
    base.ComposeStyles(context);

    var valign = Converter.ToVAlign(styleAttributes["vertical-align"]);
    if (!valign.HasValue) valign = Converter.ToVAlign(node.GetAttribute("valign").AsSpan());
    if (!valign.HasValue)
    {
        // in Html, table cell are vertically centered by default
        valign = TableVerticalAlignmentValues.Center;
    }

    cellProperties.TableCellVerticalAlignment = new() { Val = valign };

    var bgcolor = styleAttributes.GetColor("background-color");
    if (bgcolor.IsEmpty) bgcolor = HtmlColor.Parse(node.GetAttribute("bgcolor").AsSpan());
    if (bgcolor.IsEmpty) bgcolor = styleAttributes.GetColor("background");
    if (!bgcolor.IsEmpty)
    {
        cellProperties.Shading = new() { Val = ShadingPatternValues.Clear, Color = "auto", Fill = bgcolor.ToHexString() };
        // we apply the bgcolor on the cell level, not the run (this is an exception)
        runProperties.Shading = null; // Shading for runProperties set to null
    }...

Proposal:

During cascading of styles i.e. parent and childeren we explicitly propoagte this idea that table elements (runs) Shading must not be initialized

Introduce struct StylesCascade to keep in check of such cascading rules

PS. Also fixed the CreateFrame method to ensure all table cell elements <w:tc/> have Paragrsphs (<w:p/>) as the last child element, we encountered corruped xml for the above mentioned html

[onizet]

Hello, very nice finding for the frame creation. It's always annoying to generate a corrupted file as this breaks confidence in this library.

About the shading cascading issue. This library follows the "Interpreter" design pattern, in which ParsingContext stores global information required during interpretation, such as variables, data structures, or state information. That's why I don't like a second "context" class such as StyleCascade.
ParsingContext is always calling up the hierarchy for CascadeStyles. This is the exact location where you can set the Shading for a Run.

Thank you for this PR, this counts a lot for me as an open-source maintainer.

[AyUsH18102001]

Hello,

I added StyleCascade so that later we could control any other inherited properties. I’ll drop StyleCascade, keep InsideTable on the context, and remove run shading there

Thanks,

[AyUsH18102001]

Hello @onizet

Just wanted to follow up, its been about a week, let me know if there is any other changes you want from my side

Thanks

[onizet]

Hello, yes I left a comment during the review. Please see this link: https://github.com/onizet/html2openxml/pull/241/changes/BASE..2ccef12d3da23f6642d182917ec9aa7cc7982b27#r4062204747

[AyUsH18102001]

Hello @onizet

I didn't find any other comments apart from sonarcube flag comments

[onizet]

Ok but now that you see my comment, will you address it? ParsingContext should be agnostic of the discovered nodes. The logic for the table must remains ownership of TableExpression only. It's just about moving your code to the most appropriate location.
Thank you for you support.

[AyUsH18102001]

Hello @onizet ,

Changes:

• CascadeToParentContext() cascading method in HtmlElementExpression
• ParsingContext ComposeStyle agnostic to OOXML nodes
• ParsingContext invoking CascadeToParentContext for parent styles composing into child
• TableExpression owns the run shading i.e. while cascading parent styles in <w:tc>/<w:p>/<w:rPr/> strip shading element

Thanks

[onizet]

hello, I was thinking about this issue and my understanding of what people would like is that table should not propagate upwards the styles. That would mean extending this behaviour to the background color, bold, text color, etc...

What do you think of this simpler solution? Just replacing this method ParsingContext.CreateChild:

public ParsingContext CreateChild(HtmlElementExpression expression, bool isStyleScoped = false)
{
    var childContext = new ParsingContext(Converter, HostingPart, ImageLoader) {
       propertyBag = propertyBag,
       parentExpression = expression,
       parentContext = isStyleScoped? null : this,
       IsLandscape = IsLandscape
    };
    return childContext;
}

Now, the only code needed sits in TableExpression:
var tableContext = context.CreateChild(this, isStyleScoped: true);

[AyUsH18102001]

Hello @onizet

So <table/> acts as child of <body/> and none of the parent tags (div,) styles would influence the table elements

That would mean extending this behaviour to the background color, bold, text color, etc...

yes, the CascadeToParentContext() method explicitly removes the shading from run elements, by your fix we would skip all the styles added by parent div tags cause

[AyUsH18102001]

Hello,

So, let me know how can we cherry pick, will wait for your response after you merge your change in dev branch

[onizet]

I didn't created any new branch, just testing locally with no commit, but that passes all my unit tests. So happy for you to proceed with that simpler solution. Thank you!

[AyUsH18102001]

Hello,

Great, so I'll keep the CreateFrame() change (BlockElementExpression.cs) and add your borrow your suggestion and keep the tests just to make sure the shading rule on table is satisfied

[AyUsH18102001]

Hello @onizet

Sorry for the delay, please review thelatest changes

Thanks

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[onizet]

Very good, thank you so much for your valuable contribution.