Add support for background-color span style to wxHTML

Description

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.

Change History (23)

Summary
changed from background-color processing is missing from wxHtml span styles to Add support for background-color span style to wxHTML

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.

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

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...

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.

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.

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?

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''

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 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.