Bug Hunting Session
Bug 94088 - FILESAVE: paragraph background colour not saved in HTML file
Summary: FILESAVE: paragraph background colour not saved in HTML file
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
4.4.5.2 release
Hardware: x86-64 (AMD64) Windows (All)
: medium minor
Assignee: Armin Le Grand
URL:
Whiteboard: target:5.2.0
Keywords: bibisected, bisected, filter:html, regression
: 95860 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-09-10 08:32 UTC by zwerk
Modified: 2018-01-29 05:37 UTC (History)
9 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description zwerk 2015-09-10 08:32:17 UTC
Paragraph background colour is not saved when saving an ODT as HTML, nor when saving an HTML (as HTML).

This happens when using direct formatting and also when using a style with a background colour.

This was not broken in the latest LO4 Still version.
Comment 1 zwerk 2015-09-10 09:37:58 UTC
Sorry, it is in fact also broken in the latest LO4 Still version; but earlier in the 4.x series it still worked.
Comment 2 Buovjaga 2015-09-18 14:55:18 UTC
Export as xhtml works ok.
Save as html does not.

Win 7 Pro 64-bit, Version: 5.0.1.2 (32-bit)
Build ID: 81898c9f5c0d43f3473ba111d7b351050be20261
Locale: fi-FI (fi_FI)
Comment 3 raal 2015-11-17 15:50:48 UTC
Armin, can this be related to your commit? Some of bisect steps was not clear, paragraph background was not imported from my test file (in paragraph properties color palette lost). Export was also without background, so I marked bisect as "bad", but I'm not sure maybe it's not correct.   
Thanks


e642812606be49244f2f775e90169fbd1be29a97 is the first bad commit
commit e642812606be49244f2f775e90169fbd1be29a97
Author: Matthew Francis <mjay.francis@gmail.com>
Date:   Sat Mar 14 22:28:59 2015 +0800

    source-hash-7d9bb549d498d6beed2c4050c402d09643febdfa
    
    commit 7d9bb549d498d6beed2c4050c402d09643febdfa
    Author:     Armin Le Grand <alg@apache.org>
    AuthorDate: Mon Jun 2 15:00:50 2014 +0000
    Commit:     Miklos Vajna <vmiklos@collabora.co.uk>
    CommitDate: Tue Jul 1 13:30:09 2014 +0200
    
        Related: #i124638# Second step of DrawingLayer FillAttributes...
    
        for Writer objects, now added support for Paragraph and PageStyle (including
        Header and Footer) for direct attributes and style attributes
    
        (cherry picked from commit cc25c58f7052827bfebdc9fbeec668c8fa29ed1b)

bibisect-44max$ git bisect log
# bad: [cf6ea17155fabb2a120ba07c150735591ac861d7] source-hash-3f94c9e9ddfd807b449f3bb9b232cf2041fa12d2
# good: [fc71ac001f16209654d15ef8c1c4018aa55769f5] source-hash-c15927f20d4727c3b8de68497b6949e72f9e6e9e
git bisect start 'latest' 'oldest'
# bad: [8cf60cc706948588e2f33a6d98b7c55d454e362a] source-hash-f340f0454627939f1830826fb5cc53a90e6c62a4
git bisect bad 8cf60cc706948588e2f33a6d98b7c55d454e362a
# skip: [d9885f526fc7a09cc8f9f8ee643af1b966be24bb] source-hash-d1465c64c6f64ad8dd25e40cdc69649b24b305ea
git bisect skip d9885f526fc7a09cc8f9f8ee643af1b966be24bb
# bad: [8f23c2dfe58573c754fe11c1ea7633ed01fc36af] source-hash-8624eafde1ed307a3ec212309462b7ed73bb1477
git bisect bad 8f23c2dfe58573c754fe11c1ea7633ed01fc36af
# good: [47be504ca4a67a305447856dbb0af6ec3fe47ea4] source-hash-00a007be5ad88bac9905b373bc5e02d02acab11a
git bisect good 47be504ca4a67a305447856dbb0af6ec3fe47ea4
# good: [926a93ef97c3c28dfdc9263695f7b2c51295a30e] source-hash-f829d088a6db46f8f921e7d80cf0d10bb4fd0744
git bisect good 926a93ef97c3c28dfdc9263695f7b2c51295a30e
# good: [648783dac7927a816d61a97dd27bd517fd9d37e4] source-hash-ef16d765306c932c49254f295f57e5853129c1ea
git bisect good 648783dac7927a816d61a97dd27bd517fd9d37e4
# good: [36c35de00685c5512a3a1e790c65a464bf7f8a83] source-hash-e820df579d9be4c1f9bb1ad8f02a8072c69b52da
git bisect good 36c35de00685c5512a3a1e790c65a464bf7f8a83
# good: [07e27f3d93e6f52c35e89ae0723e86340358f9c7] source-hash-3f177756dbdb67d901453000c3f11694770d2761
git bisect good 07e27f3d93e6f52c35e89ae0723e86340358f9c7
# good: [23e86aed940b757adc649810c1918fa469f6940c] source-hash-2c157249207552e193e52f5ab7fad5b37ae0a748
git bisect good 23e86aed940b757adc649810c1918fa469f6940c
# good: [569a2ff636b96e7c6fb477d368224ded535c4919] source-hash-05832e110f5d67a20bea5bc0bd138c0ad7033674
git bisect good 569a2ff636b96e7c6fb477d368224ded535c4919
# good: [0ebc833ad6dfadc2c67a2097960683fb67552daf] source-hash-691f0b3c387033e34bba4b6cb65f2ad1250ef818
git bisect good 0ebc833ad6dfadc2c67a2097960683fb67552daf
# bad: [d3a0e6c621ceb08e11867a2e17f155ae1977cee2] source-hash-c7853b5b5cb71899b6b60fd2175763785b8afb7f
git bisect bad d3a0e6c621ceb08e11867a2e17f155ae1977cee2
# good: [1b9125c983db686bc5a1711c5030e30b600b4c1b] source-hash-42fcd888ae537152f9d59c03e945d4bf9aaeb7dd
git bisect good 1b9125c983db686bc5a1711c5030e30b600b4c1b
# good: [e3c84f2b17b881007208ac54c45cbfcd7dc395b2] source-hash-a5e137eb1d37361c60175e8fba780fc46b377a23
git bisect good e3c84f2b17b881007208ac54c45cbfcd7dc395b2
# bad: [e642812606be49244f2f775e90169fbd1be29a97] source-hash-7d9bb549d498d6beed2c4050c402d09643febdfa
git bisect bad e642812606be49244f2f775e90169fbd1be29a97
# first bad commit: [e642812606be49244f2f775e90169fbd1be29a97] source-hash-7d9bb549d498d6beed2c4050c402d09643febdfa
Comment 4 Armin Le Grand 2015-11-18 14:57:50 UTC
@raal: Depends if someone had added the new functionality that paragraph backgrounds can now be various fill types to the HTML im/export or not. Taking a look...
Comment 5 Armin Le Grand 2015-11-19 08:50:41 UTC
As I thought - noone adapted the HTML export to the set of FillStyle items now available. It is still using SvxBrushItem. This should give the fallback by stuffing as much from the new representation to the SvxBrushItem (also used for compatibility im/export). I will check why not even that currently is working...
Comment 6 Armin Le Grand 2015-11-20 10:19:52 UTC
I see already some changes to the better in Bug 86857 - HTML export: Page background color not saved to document, but seems not complete.
Comment 7 Armin Le Grand 2015-11-20 14:49:54 UTC
Found the place where the Items get exported. All in all, the HTML im/export is pretty old and not in good shape. It ses the writer core directly and there is a fixed table mapping ItemIDs to function pointers for creating the HTML output tags - this will break immediately should the ItemIDs ever change.
In that range the new fill attribute items in the [XATTR_FILL_FIRST .. XATTR_FILL_LAST] range already appear, but are beyond the table (XATTR_FILLSTYLE is around 1014). Added code to create the classic SvxBrushItem for now as a fallback to provide the same functionality as before. Checked for im/export of paragraph background.
Paragraph background re-import with bitmap does not work yet, gradient shows the expected fallback, hatch seems too dark (using only hatch color?).
Comment 8 Armin Le Grand 2015-11-23 10:12:32 UTC
Bitmap: export okay, checking import. Import sets a SvxBrushItem using completely other mechanisms at the paragraph. It would be possible to use teh XATTR_FILL range instead, but it should work with SvxBrushItem, too.
It does not work since the graphicLink string at the SvxBrushItem contains the base64 directly (at the string) and there is no code to handle this. There must be code somewhere, taking a look.
Comment 9 Armin Le Grand 2015-11-23 13:23:42 UTC
Export graphic to HTML uses XOutBitmap::GraphicToBase64 to directly add the graphic data to the URL. Formally, the graphic was written besides the html file as separate graphic file - maybe this can even be configured someway.
At the load code there is nothing to convert that back - thus, SvxBrushItem::GetGraphicObject fails badly. Checking how the GraphicToBase64 code and export was added (but no load code)...
Comment 10 Armin Le Grand 2015-11-23 14:41:55 UTC
Looks as if fdo#63211 'embed images in HTML export.' added this as a feature, but did not take care about re-importing them. Added a re-import to SvxBrushItem::GetGraphicObject, this should be pretty central to get all cases handled (e.g. graphic bullets are also saved embedded).
Comment 11 Armin Le Grand 2015-11-23 15:03:34 UTC
Re-checked all HTML paragraph background im/exports, looks good. Hatch gets the hatch color, this happens since the SvxBrushItem-creator (getSvxBrushItemFromSourceSet) creates a color with transparence to not let the hatch color get too dominant, but that is not supported with the HTML export, so gets reset to the pure hatch color.
More in this area would require a deeper HTML im/export redesign, then as filter and using UNO API.
Doing some more tests...
Comment 12 V Stuart Foote 2015-11-23 15:11:01 UTC
(In reply to Armin Le Grand from comment #10)
> Looks as if fdo#63211 'embed images in HTML export.' added this as a
> feature, but did not take care about re-importing them. Added a re-import to
> SvxBrushItem::GetGraphicObject, this should be pretty central to get all
> cases handled (e.g. graphic bullets are also saved embedded).

Armin, thanks for looking at this.

A bit of history -- bug 48887 and bug 63211 are diametrically opposed -- but in the interest of returning function as a visual HTML editor, embedded base64 of tdf#63211 needs to be controlled to only apply for email merge corner case. It really screws up HTML source view in Writer/Web mode. 

bug 88038 is the more general UX-advise "HTML editor is broken, what do we do now?" 

and more recent bug 95861 asks "if we fix this, should probably be brought current to use HTML5 and inline CSS3 (or maybe even support style sheets)?
Comment 13 Armin Le Grand 2015-11-23 15:26:33 UTC
Solution added to gerrit for review
Comment 14 Armin Le Grand 2015-11-23 15:32:52 UTC
Hi Stuart, thanks for these background infos.

I have no idea if it is by purpose, but saving a SW doc to HTML uses the embedded format - so it is currently not only used for email merge. My proposed change will enable to re-import embedded graphics in HTML (in whatever case), thus should in that aspect lead to a better/more as before HTML im/export/roundtrip/experience.
Future plans are interesting, but as a minimum the original functionality should be preserved/brought back, it is currently broken. Of course implementing a completely new HTML im/export (using UNO API, ...) is more than welcome.
Comment 15 Armin Le Grand 2015-11-27 09:06:11 UTC
Checked how to add a UnitTest, stumbled over some still not good internal stuff with the URL handling involved, taking a 2nd look...
Comment 16 Armin Le Grand 2015-11-27 11:12:26 UTC
2nd look shows that there is a problem with CSS style URLs in 'background: url('data:image/jpeg;base64,/' statements - these do not get stripped from the single quotwes, but only by leading/trailing spaces. That quote prevents correct INetURLObject of the data: schema URLs. Fixed that in the importer, so no own Base64 interpretation is needed.
It is still needed to add importing data schema URLs in SvxBrushItem, but the standard way can then be used.
Comment 17 Commit Notification 2015-11-28 11:32:59 UTC
Armin Le Grand committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=29dfcc7521311e547fc069466cc3edc9fcbdbe03

tdf#94088 add import of HTML inline graphics

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 18 Robinson Tryon (qubit) 2015-12-14 05:35:21 UTC Comment hidden (obsolete)
Comment 19 V Stuart Foote 2015-12-31 18:03:34 UTC
*** Bug 95860 has been marked as a duplicate of this bug. ***
Comment 20 V Stuart Foote 2016-03-19 16:00:16 UTC
Hey Armin,

Oliver S. is reworking the bug 63211 code in  https://gerrit.libreoffice.org/#/c/23359/ and gave me a bump to look over things.

Not sure about the paragraph background color, but the embedded URI graphic rendering whne reopening .HTML has been solid on 5.2 builds of master since commit. Probably could have been back ported to 5.1

https://gerrit.libreoffice.org/#/c/20129/
Comment 21 V Stuart Foote 2016-03-20 15:05:50 UTC
(In reply to V Stuart Foote from comment #20)
Armin, *

Rats, never mind. The base64 image handling did make the 5.1 branch, the back port would be to 5.0--and not sure that makes any sense for just a 5.0.6 release.

Sorry for the noise.
Comment 22 Xisco Faulí 2016-09-26 10:07:29 UTC
Hello Armin,
Is this bug fixed?
If so, could you please close it as RESOLVED FIXED?
Comment 23 Armin Le Grand 2016-09-29 08:47:39 UTC
re-checked, works.