#14599 closed enhancement (fixed)
Add styles support for links in wxHtml
Reported by: | gevorg | Owned by: | |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | wxHtml | Version: | stable-latest |
Keywords: | html links href style a | Cc: | |
Blocked By: | #14443, #14443, #14443, #14443, #14443, #14443, #14443, #14443 | Blocking: | |
Patch: | yes |
Description
Processing of style attribute in <a> tag is currently missing. The attached patch adds it. #14443 needs to be committed before this patch can be applied.
Attachments (3)
Change History (12)
Changed 6 years ago by gevorg
comment:1 Changed 6 years ago by vadz
- Blocked By
(In #14443) 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?
comment:2 Changed 6 years ago by gevorg
- Blocked By
comment:3 Changed 6 years ago by vadz
- Blocked By
(In #14443) 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!
comment:4 Changed 6 years ago by gevorg
- Blocked By
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.
comment:5 Changed 6 years ago by gevorg
- Blocked By
(In #14443) 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.
comment:6 Changed 6 years ago by VZ
- Blocked By
Changed 6 years ago by gevorg
Use wx{SOLID,TRANSPARENT} instead of wxBRUSHSTYLE_{SOLID,TRANSPARENT} in the remaining appropriate places throughout wxHtml
comment:7 Changed 6 years ago by VZ
- Resolution set to fixed
- Status changed from new to closed
#14443 needs to be committed before this patch can be applied, as it uses background color/mode related functions that are added in http://trac.wxwidgets.org/attachment/ticket/14443/span_background_color.1.patch