Bug 121111 - FILESAVE: DOC/RTF: Paragraph fill becomes blue after roundtrip
Summary: FILESAVE: DOC/RTF: Paragraph fill becomes blue after roundtrip
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
6.2.0.0.alpha1+
Hardware: All All
: medium normal
Assignee: Justin L
URL:
Whiteboard: target:6.3.0 target:6.2.1
Keywords: bibisected, bisected, regression
Depends on:
Blocks:
 
Reported: 2018-11-01 23:29 UTC by Xisco Faulí
Modified: 2024-08-22 15:41 UTC (History)
4 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 Xisco Faulí 2018-11-01 23:29:55 UTC
Steps to reproduce:
1. Open attachment 119583 [details] from bug 95032
2. Save it as .DOC or .RTF
3. Reopen the new file

-> all paragraph have a blue background. Not reproduced saving to .ODT or .DOCX

Reproduced in

Version: 6.2.0.0.alpha1+
Build ID: 4326fb3ef3ddd7c6f9d08ba96add4f4736503ceb
CPU threads: 4; OS: Linux 4.15; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: threaded

[Bug found by office-interoperability-tools]
Comment 1 Xisco Faulí 2018-11-01 23:32:02 UTC
Regression introduced by:

https://gerrit.libreoffice.org/gitweb?p=core.git;a=commit;h=7d9e9ecd5bb5d5abec338c2ceb61ad623d2ac5cb

tdf#90906 writerfilter: Allow COL_AUTO to override non-auto 13/59313/4
author	Justin Luth <justin.luth@collabora.com>	
	Wed, 22 Aug 2018 07:25:37 +0100 (09:25 +0300)
committer	Miklos Vajna <vmiklos@collabora.co.uk>	
	Wed, 5 Sep 2018 09:17:32 +0100 (10:17 +0200)
commit 7d9e9ecd5bb5d5abec338c2ceb61ad623d2ac5cb
tree 1117a7f8fdbe9a6264f2a81392cb5020e028cc5e
parent 1d4f03114f3fb04fc1b493193e3eddee07cdabb8

Bisected with: bibisect-linux64-6.2

Adding Cc: to Justin Luth
Comment 2 Justin L 2018-11-05 17:19:03 UTC
Looks like the problem is with
  else if ( false && m_bFillSpecified && m_bAutoFillColor )
    pPropertyMap->Insert(PROP_FILL_STYLE, uno::makeAny(drawing::FillStyle_NONE));

So, likely the export code for these formats is not looking at FillStyle_NONE, and just using the fill color anyway.  Exposed existing problem.
Comment 3 Justin L 2018-11-28 17:33:50 UTC
Not 100% sure this is the right approach, but it seems likely: https://gerrit.libreoffice.org/#/c/64185/
Comment 4 Commit Notification 2019-01-14 19:32:35 UTC
Justin Luth committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/+/1a6367269df971dd6764db419162896170b4b21d%5E%21

tdf#121111 ww8export: fillstyle_NONE needs a nil background

It will be available in 6.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 5 Xisco Faulí 2019-01-16 15:25:41 UTC
Verified in

Version: 6.3.0.0.alpha0+
Build ID: 1bf68dbf53f4b5308e295058226abd6d6fb49c3d
CPU threads: 4; OS: Linux 4.15; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); UI-Language: en-US
Calc: threaded

@Justin Luth, thanks for fixing this!!
Comment 6 Xisco Faulí 2019-01-16 15:28:01 UTC
Cherry-picked to 6-2 -> https://gerrit.libreoffice.org/#/c/66463/
Comment 7 Commit Notification 2019-01-17 14:30:23 UTC
Justin Luth committed a patch related to this issue.
It has been pushed to "libreoffice-6-2":

https://git.libreoffice.org/core/+/f70c4fe32136fdab2fc9309b4f40d9a954c56dd8%5E%21

tdf#121111 ww8export: fillstyle_NONE needs a nil background

It will be available in 6.2.1.

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

Affected users are encouraged to test the fix and report feedback.
Comment 8 Buovjaga 2024-05-13 05:32:23 UTC
The RTF part of this bug was never fixed. I have a new contributor working on unit tests and he created this that found the RTF issue: https://gerrit.libreoffice.org/c/core/+/167179

He had not noticed that there was a related test in
sw/qa/extras/ww8export/ww8export3.cxx
CPPUNIT_TEST_FIXTURE(Test, testTdf121111_fillStyleNone)

but that test is only roundtripping docx and not saving to doc or rtf.
Comment 9 Justin L 2024-05-13 11:43:03 UTC
(In reply to Buovjaga from comment #8)
> but that test is only roundtripping docx and not saving to doc
By definition, tests in ww8export export to DOC format. If for some reason that isn't actually happening it would be a serious bug.
Comment 10 Buovjaga 2024-05-13 14:01:12 UTC
(In reply to Justin L from comment #9)
> (In reply to Buovjaga from comment #8)
> > but that test is only roundtripping docx and not saving to doc
> By definition, tests in ww8export export to DOC format. If for some reason
> that isn't actually happening it would be a serious bug.

It has this:

CPPUNIT_TEST_FIXTURE(Test, testTdf121111_fillStyleNone)
{
    loadAndReload("tdf121111_fillStyleNone.docx");
...

So calling this in sw/qa/unit/swmodeltestbase.cxx

void SwModelTestBase::loadAndReload(const char* pName)
{
    loadURL(createFileURL(OUString::createFromAscii(pName)));
    saveAndReload(mpFilter);
}

I didn't find any deducing of .doc filter based on the suite we're in. Shouldn't it be explicit?
Comment 11 Justin L 2024-05-14 23:02:31 UTC
(In reply to Buovjaga from comment #10)
>     saveAndReload(mpFilter);
mpFilter in ww8export* is "MS Word 97", so no, it doesn't need to be explicit.
Comment 12 Buovjaga 2024-05-15 06:05:09 UTC
(In reply to Justin L from comment #11)
> (In reply to Buovjaga from comment #10)
> >     saveAndReload(mpFilter);
> mpFilter in ww8export* is "MS Word 97", so no, it doesn't need to be
> explicit.

Ok, now I get it, set in the beginning with

class Test : public SwModelTestBase
{
public:
    Test()
        : SwModelTestBase("/sw/qa/extras/ww8export/data/", "MS Word 97")
    {
    }
};
Comment 13 Xisco Faulí 2024-08-22 15:41:06 UTC
The fix for this issue is already covered with testTdf121111_fillStyleNone, a unittest added in

author	Justin Luth <justin.luth@collabora.com>	2018-11-28 16:17:22 +0300
committer	Justin Luth <justin_luth@sil.org>	2019-01-15 19:50:35 +0100
commit 3539a1efb41a787237e4333ebc715db96ffacb5b (patch)
tree a671c2f9d57a33bd6e55d5835223c30461f557c1
parent e1e772e40f83a8f1d7c4947c5784e1fe8b640d5b (diff)
tdf#116071 ww8import: import to XATTR_FILL instead of RES_BACKGROUND