Add support for background-color span style to wxHTML

[gevorg]

There is no code for background color in m_span.cpp, so specifying background-color like <span style="background-color:#FF8080;"> doesn't work.
I tried to add it copy-pasting from "color", using wxHTML_CLR_BACKGROUND for wxHtmlColourCell and deleting the line calling SetActualColor, but that didn't work for some reason.

[vadz]

wxHTML doesn't really support CSS, any patches adding support for more styles would be welcome, of course, but there are no plans to ever have full CSS support in it. If you need this, please use wxWebView.

[gevorg]

Replying to gevorg:

I tried to add it copy-pasting from "color", using wxHTML_CLR_BACKGROUND for wxHtmlColourCell and deleting the line calling SetActualColor, but that didn't work for some reason.

I found why that didn't work. There are some changes needed in htmlcell.cpp to make it work. I'm just not sure what the correct changes are. Here's what I did in htmlcell.cpp that at least starts to display background colors.
First I added {{{
dc.SetBackgroundMode(wxBRUSHSTYLE_SOLID);
}}} calls in wxHtmlColourCell::DrawInvisible(), as even though the background color was being set, it didn't get used as the background mode stayed wxBRUSHSTYLE_TRANSPARENT. E.g. in wxHtmlWordCell::Draw() there is this

        else if ( selstate == wxHTML_SEL_OUT &&
                  dc.GetBackgroundMode() == wxBRUSHSTYLE_SOLID )
        {
            SwitchSelState(dc, info, false);
        }

Here background mode check evaluated false so SwitchSelState wasn't called, and because background mode stayed wxBRUSHSTYLE_TRANSPARENT, no background was drawn.
After adding those SetBackgroundMode calls, SwitchSelState started to get called, but no background was drawn still as SwitchSelState() was reverting background mode to transparent before the text got chance to be drawn.
Then the second change I did was to replace {{{
dc.SetBackgroundMode(wxBRUSHSTYLE_TRANSPARENT);
}}} with {{{
dc.SetBackgroundMode(wxBRUSHSTYLE_SOLID);
}}}
This feels wrong/inconsistent with the rest of code, but does display background colors. In particular, after that change the selstate and background mode checks in wxHtmlWordCell::Draw() make no sense, but I can't figure out why would we want background mode to be transparent in the first place. Perhaps I'm missing something...

[gevorg]

Changes to make background-color in span tag work. Most likely some of the changes in htmlcell.cpp aren't correct, if so I'd be glad to know what is wrong and why.

[gevorg]

Sorry for misformatted code in http://trac.wxwidgets.org/ticket/14443#comment:2, sadly I can't edit it...

[vadz]

I'm not sure about these changes neither but we do clearly need to set background mode to opaque to draw text background. However unconditionally using solid background mode in SwitchSelState() doesn't seem right, this is probably going to overwrite any background bitmap even if the background colour is not set, for example. So I think you need to call SetBackgroundMode() in some other place or perhaps rename SwitchSelState() to SwitchBgState() and call it either when the selection is toggled or when the background cell starts/ends.

But I'm almost sure that the current patch is incorrect, please try testing it with non-default background.

[gevorg]

Indeed the current patch has problems. I had each of my HTML elements a background color specified, so didn't notice the problem earlier. Removing background color specification from some of them it became clear that when an element has background color, all subsequent elements are drawn with that background color as well, until another element specifies a background color. In retrospect this was entirely expected from the current patch, as a background color, once set, doesn't get cleared after it's scope ends. But I think the problem has to be fixed not by forcing wxBRUSHSTYLE_TRANSPARENT in wxHtmlWordCell::Draw(), but by restoring background color and mode after the span tag's scope ends. This would be just like how foreground color and other attributes are handled, e.g.

        // m_span.cpp, lines 157-162
        if (oldclr != m_WParser->GetActualColor())
        {
            m_WParser->SetActualColor(oldclr);
            m_WParser->GetContainer()->InsertCell(
                new wxHtmlColourCell(oldclr));
        }

Something like this needs to be done for background color and mode as well. Does this make sense? Do you see other potential problems?
And if we go this way, should background mode restoring capability be added to wxHtmlColourCell or a new cell type be created for that with a name something like wxHtmlBackgroundModeCell?

[vadz]

I think restoring the background mode would indeed be the right thing to do, thinking more about this, the mode has to be opaque for the background colour to work so it must be restored later.

I don't think creating a new cell type is worth it just for this but I don't know this code well, perhaps Vaclav can provide a better-founded advice.

[vaclavslavik]

Replying to vadz:

I don't think creating a new cell type is worth it just for this but I don't know this code well, perhaps Vaclav can provide a better-founded advice.

I think it's OK to have just one kind of settings-switching cell. And yes, I think this is the right approach.

[gevorg]

Updated patch. Seems to work so far. Comments welcome.

[gevorg]

Replying to gevorg:

There is no code for background color in m_span.cpp, so specifying background-color like <span style="background-color:#FF8080;"> doesn't work.
I tried to add it copy-pasting from "color", using wxHTML_CLR_BACKGROUND for wxHtmlColourCell and deleting the line calling SetActualColor, but that didn't work for some reason.

I'd like to edit this description like so:
using wxHTML_CLR_BACKGROUND for wxHtmlColourCell and deleting the line calling SetActualColor, but that didn't work for some reason. The patch span_background_color.1.patch provides a working implementation.
but I don't seem to have the permission. If an admin could do it for me, that'd be great''

[vadz]

Could you please add an example of use of this tag to some of the HTML files in samples/html/test please to make it possible to easily check that it does indeed work correctly?

[gevorg]

Replying to vadz:

Could you please add an example of use of this tag to some of the HTML files in samples/html/test please to make it possible to easily check that it does indeed work correctly?

Sure. Below is the updated patch.

[gevorg]

Added demo usage of background-color in <span> to samples/html/test/test.htm

[vadz]

Thanks for the update and sorry for the delay with reviewing the patch!

Now that I finally did it I realize that I'm not entirely happy with it unfortunately. The confusing thing is that adds a new wxHtmlColourCell ctor taking wxBrushStyle and then uses this for both creating the brush with this style and calling wxDC::SetBackgroundMode() with it. The latter does work because wxBRUSHSTYLE_SOLID == wxSOLID and wxBRUSHSTYLE_TRANSPARENT == wxTRANSPARENT but I'm uncomfortable relying on this. The former bothers me too as creating a transparent brush is entirely unnecessary when the background mode is transparent -- the background brush is not used at all in this case.

Also, the existing 2 argument wxHtmlColourCell ctor is documented so it actually is part of the public API (and it does make sense, you could use it as a base class for your custom cell class) and so shouldn't be modified in incompatible way as the patch does by removing its second argument.

So I think the best would be to actually just add some new wxHTML_CLR_OPAQUE_BACKGROUND (or wxHTML_CLR_TRANSPARENT_BACKGROUND? I'm a bit hazy about which one does the current wxHTML_CLR_BACKGROUND correspond to, but the idea is to add another constant for the other kind of background) and call wxDC::SetBackgroundMode() accordingly and not create the background brush unnecessarily.

Would it be possible for you to modify the patch like this if you don't see any problems with this approach?

Thanks again!

[gevorg]

Updated patch according to feedback received in http://trac.wxwidgets.org/ticket/14443#comment:13

[gevorg]

Replying to vadz:

Thanks for the update and sorry for the delay with reviewing the patch!

Now that I finally did it I realize that I'm not entirely happy with it unfortunately. The confusing thing is that adds a new wxHtmlColourCell ctor taking wxBrushStyle and then uses this for both creating the brush with this style and calling wxDC::SetBackgroundMode() with it. The latter does work because wxBRUSHSTYLE_SOLID == wxSOLID and wxBRUSHSTYLE_TRANSPARENT == wxTRANSPARENT but I'm uncomfortable relying on this. The former bothers me too as creating a transparent brush is entirely unnecessary when the background mode is transparent -- the background brush is not used at all in this case.

Also, the existing 2 argument wxHtmlColourCell ctor is documented so it actually is part of the public API (and it does make sense, you could use it as a base class for your custom cell class) and so shouldn't be modified in incompatible way as the patch does by removing its second argument.

So I think the best would be to actually just add some new wxHTML_CLR_OPAQUE_BACKGROUND (or wxHTML_CLR_TRANSPARENT_BACKGROUND? I'm a bit hazy about which one does the current wxHTML_CLR_BACKGROUND correspond to, but the idea is to add another constant for the other kind of background) and call wxDC::SetBackgroundMode() accordingly and not create the background brush unnecessarily.

Would it be possible for you to modify the patch like this if you don't see any problems with this approach?

Thanks again!

Thanks for your feedback and sorry for the slow reply!
Didn't see that wxHtmlColourCell ctor was documented, thanks for pointing that out!
Please have a look at the new patch and let me know if it's close to what you had in mind.

[gevorg]

Just realized that I still didn't address your comment on using wxBrushStyle for calling wxDC::SetBackgroundMode(). I can fix that as well, if everything else is OK.

[VZ]

(In [72589]) Add support for background-color style to span element in wxHTML.

Add code for setting/restoring background mode and use it to implement support
for changing the text background colour.

Closes #14443.

[VZ]

(In #14599) (In [72630]) Support some CSS styles for the links in wxHTML too.

Refactor limited CSS styles support for <span> tag to reuse it for <a> tag as
well.

Closes #14599.

[VZ]

(In #14599) (In [72631]) No real changes, just don't use brush styles for background mode in wxHTML.

Use just wxTRANSPARENT and wxSOLID instead of wxBRUSHSTYLE_TRANSPARENT and
wxBRUSHSTYLE_SOLID when changing the background mode.

See #14599.

[VZ]

(In [73393]) Really fix the background colour used for the cells in wxHTML tables.

Respect "bgcolor" attributes of <td> tags, they were ignored since the changes
of r72589 (see #14443).

Closes #14909.