Bug 103602 - new documents fail ODF validation with Error: unexpected attribute "draw:fill"
Summary: new documents fail ODF validation with Error: unexpected attribute "draw:fill"
Status: ASSIGNED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
5.3.0.0.alpha0+
Hardware: All All
: high major
Assignee: Luke Deller
QA Contact:
URL:
Whiteboard: odf
Keywords: bibisected, bisected, regression
Depends on:
Blocks: 72238 72239 Area-Fill-Tab ODF-export-invalid 112195
  Show dependency treegraph
 
Reported: 2016-10-31 14:04 UTC by Justin L
Modified: 2017-10-18 07:34 UTC (History)
12 users (show)

See Also:
Crash report or crash signature:


Attachments
Test doc (before change) with illegal fill-related attributes (18.29 KB, application/vnd.oasis.opendocument.text)
2017-07-30 03:04 UTC, Luke Deller
Details
Test doc (after proposed change) using loext: namespace (18.29 KB, application/vnd.oasis.opendocument.text)
2017-07-30 03:07 UTC, Luke Deller
Details
Manually applied the suggested changes (12.53 KB, application/vnd.oasis.opendocument.text)
2017-08-17 08:19 UTC, Regina Henschel
Details
An analysis and some versions of schema changes (4.10 KB, application/zip)
2017-08-30 23:33 UTC, Regina Henschel
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Justin L 2016-10-31 14:04:28 UTC
Description:
New documents do not create a valid ODF 1.2 extended file if the properties dialog is opened (for sure with the page properties, likely any property that contains the fill area tab).

Steps to Reproduce:
1. Start a new Writer document 
2. Open the Page Properties (Format - Page) and press OK to close it.
3. Save as .odt format
4. Check validity at https://odf-validator.rhcloud.com/ (using 1.2 extended, not auto-detect)

Actual Results:  
The document is NOT extended conformant ODF1.2!

Expected Results:
The document is extended conformant ODF1.2!


Reproducible: Always

User Profile Reset: 

Additional Info:
Bibisected with Linux daily debug 5.3.
It broke somewhere between:
2016-10-11 update credits Christian Lohmaier 	29fe57afd79c96c5ff251c2468ababda5e506564

and 

2016-10-12 ADO: return correct css::sdbcx::CompareBookmark values
Stephan Bergmann 66b67f40a7785f08ae214e62b669e001148b474c

likely by either
[GSoC] Remove fill style tabs from area fill dialog 	d19ec9a9bd371248afd7c3ca091a54b1782fddf7

or

[GSoC] Move all fill style tabs inside area tab 686349476e03f951f4a9ff9755b9f71951b64ea5


User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Comment 1 Aron Budea 2016-10-31 15:12:04 UTC
It's the second candidate:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=686349476e03f951f4a9ff9755b9f71951b64ea5

"[GSoC] Move all fill style tabs inside area tab"

Full bibisect log:
# bad: [845539f0cf09355a7baa6d1cfbf6ad478a983da5] source sha:8dc495c93239739629683bb29e4110f5c57c94f0
# good: [33e60eae04c889baf52713a73dc9944015408914] source sha:5b168b3fa568e48e795234dc5fa454bf24c9805e
git bisect start '845539f0cf09355a7baa6d1cfbf6ad478a983da5' '33e60eae04c889baf52713a73dc9944015408914'
# good: [9e0af0bfb61955f2a7bcae3a25a403cfc01919f4] source sha:e8365711e817876ee45b282fc16977b55f4dbca8
git bisect good 9e0af0bfb61955f2a7bcae3a25a403cfc01919f4
# good: [45cf7c7435abbc97ae9773953e5f582784b91c1c] source sha:d5f1e7c5adac5f38379c56b536640f3aaf9ec7d5
git bisect good 45cf7c7435abbc97ae9773953e5f582784b91c1c
# good: [be8f9784b28d086ec451f86ed968fe0364b26382] source sha:8413d6a91d5d03c0c47a8fbd2e41cd45cec95a1c
git bisect good be8f9784b28d086ec451f86ed968fe0364b26382
# skip: [681fc5fe4f850e7885faf8fc43b1c7f377f6f902] source sha:613213240a84327a0a72bb5ee98cc88d50c1fd0e
git bisect skip 681fc5fe4f850e7885faf8fc43b1c7f377f6f902
# bad: [6892f3a21a61c0aeb726e060576b291e08b052d6] source sha:398d641664baa6eaeb34789f0aebfd21e73edef3
git bisect bad 6892f3a21a61c0aeb726e060576b291e08b052d6
# bad: [c8e7036b5e5248aa5b20315767d55832fc7d0375] source sha:0f613adbfa44fb92e84e73a3fa7ea050c072944c
git bisect bad c8e7036b5e5248aa5b20315767d55832fc7d0375
# good: [1c9e9a7ab9725516a6f9f20326439a6c025cd222] source sha:583ac140dcf6f11b21bc9c52516ba2bbcbd37a95
git bisect good 1c9e9a7ab9725516a6f9f20326439a6c025cd222
# bad: [3177d9338fb3d48f40a5a13b980d6e829f4bf76d] source sha:e6a05e9f3276cccce5d72adce24a8d2fee6b8b7b
git bisect bad 3177d9338fb3d48f40a5a13b980d6e829f4bf76d
# good: [459a28dbbb27faacb725b523cb25555671b2debd] source sha:34698034dbfe59bf444709fcf54d01f8a6f03f83
git bisect good 459a28dbbb27faacb725b523cb25555671b2debd
# bad: [281ec4b684aeba7aaf527e64e6cb187b8a1e95d5] source sha:a77ff8446e7ed2255b11406b4ccce7ec742d7c67
git bisect bad 281ec4b684aeba7aaf527e64e6cb187b8a1e95d5
# bad: [cff3c3864a46cefde1b6e3145995b28ae6f9f008] source sha:0d93900801224b797741e9a1abf305109fa35665
git bisect bad cff3c3864a46cefde1b6e3145995b28ae6f9f008
# good: [9c0485b505f97777dbb84accf1a902a234b234f5] source sha:ed3a4fcc878e8f3ce720520d60448bc962da0754
git bisect good 9c0485b505f97777dbb84accf1a902a234b234f5
# bad: [b3691ee4fdddd35b2dcea8bec7daecc981792844] source sha:686349476e03f951f4a9ff9755b9f71951b64ea5
git bisect bad b3691ee4fdddd35b2dcea8bec7daecc981792844
# good: [2d0914fa553e754fe5942c037898e21102d93388] source sha:da01e9ec5dfb7787b4a3669486b3940590933850
git bisect good 2d0914fa553e754fe5942c037898e21102d93388
# first bad commit: [b3691ee4fdddd35b2dcea8bec7daecc981792844] source sha:686349476e03f951f4a9ff9755b9f71951b64ea5
Comment 2 Regina Henschel 2016-10-31 16:08:23 UTC
It is a hard problem. The "draw:fill" attribute may not be used in the element <style:page-layout-properties>. And the element <style:page-layout> has no other children or attributes that are suitable.

For Draw and Impress, this is solved by an element <style:style> of style family "drawing-page". But this family is only usable with draw and presentation documents.

For Calc it is solved by restricting the background filling to color and image, because those do not need an attribute "draw:fill". Color is written by the attribute "fo:background-color" and an image by the child element <style:background-image> of the element <style:page-layout-properties>.
Writer was restricted in the same way in older versions of LibreOffice.

So currently exists no way to express other kind of fillings beside color or image for a text document or for a spreadsheet in ODF.

There are suggestions required, how to solve this problem in ODF. Such considerations would have been necessary _before_ implementing the new feature.

Another problem is, to get it into the namespace "loext", because only whole elements and attributes have a namespace, but not a single value.

Where should this be discussed, so that it is really public? Here in the issue, or on the dev mailing list, or in a Wiki page or ...?
Comment 3 Katarina Behrens (CIB) 2016-11-01 15:32:14 UTC
Can anyone please 'splain what exactly the issue here is, pretty please? For example, paste some ODF XML diff before vs. after

> So currently exists no way to express other kind of fillings beside color or
> image for a text document or for a spreadsheet in ODF.

As far as I can see, Format > Page dialog in Writer (Libreoffice 5.1, so the old one existing prior to GSoC code merge) offers *all* kinds of fills for a page, not only colour and bitmap/image. Does this mean that the other fill types have never been saved to ODF and now they are?

As for Calc (again in Libreoffice 5.1), I only see the option to set background to solid colour (Format > Cells > Background) and the tab page used for that is different than the tab page used for shape fill (Background vs. Area, they only look similar)
Comment 4 Regina Henschel 2016-11-01 16:51:16 UTC
(In reply to Katarina Behrens (CIB) from comment #3)
> As far as I can see, Format > Page dialog in Writer (Libreoffice 5.1, so the
> old one existing prior to GSoC code merge) offers *all* kinds of fills for a
> page, not only colour and bitmap/image. Does this mean that the other fill
> types have never been saved to ODF and now they are?

The file format error slipped in from LO 4 to LO 5.
Version 4.1 and 4.2 had the old background selection with color and image as still available in Calc.
Version 4.3 and 4.4 had the item "gradient" added, but that was without any function.
Version 4.5 had got fill kind "gradient" and "hatching" and the worked, but they were not written to file, and therefore any gradient or hatching was lost on save and replaced with a single color fill in LO 4.5.

Writing invalid ODF slipped in after version 4.5 and it is not a problem of this year GSoC.

The error itself was made in version 4, when adding a new kind of filling, without considering ODF file format.
Comment 5 Xisco Faulí 2016-11-03 21:39:00 UTC
Adding Cc: to Rishabh Kumar
Comment 6 Luke Deller 2017-07-24 13:01:35 UTC
I understand from these comments that ODF does not support page fill by gradient/hatching for text documents, so we will need to move this information into the "loext" extension namespace.

To achieve this how about we make a child element of <style:page-layout-properties> named <loext:page-fill>, and move all the draw:fill-* attributes onto this child element?  Or any better suggestions?
Comment 7 Regina Henschel 2017-07-24 15:48:42 UTC
Please have look at the similar issue bug 94768.
Comment 8 Regina Henschel 2017-07-24 21:33:02 UTC
Thorsten, can you please have a look at it, how a solution should look in principle?

(In reply to Luke Deller from comment #6)
> I understand from these comments that ODF does not support page fill by
> gradient/hatching for text documents, so we will need to move this
> information into the "loext" extension namespace.
> 
> To achieve this how about we make a child element of
> <style:page-layout-properties> named <loext:page-fill>, and move all the
> draw:fill-* attributes onto this child element?  Or any better suggestions?

I'm no sure about a solution. We need to keep in mind, how a final solution without namespace "loext:" can be realized in ODF 1.3. Do you have an idea about changing the schema and the specification?

From a technical point of view, you can put all the currently not allowed draw:fill-* attributes individually into the "loext:" namespace, without a new element. Please talk to Thorsten Behrens about a principle solution, perhaps via IRC.
Comment 9 Thorsten Behrens (CIB) 2017-07-24 22:14:30 UTC
(In reply to Regina Henschel from comment #8)
> Thorsten, can you please have a look at it, how a solution should look in
> principle?
> 
No real better ideas - but loext:page-fill seems redundant to me, too. Perhaps we  can restructure the schema a bit, so that fill attributes are grouped better, instead of repeatedly listed at various places?
Comment 10 Regina Henschel 2017-07-25 16:37:31 UTC
All the draw:fill-* attributes are in a <define name="style-graphic-fill-properties-attlist"> symbol in the schema. Unfortunately it has some attributes more than needed here. We could add a reference to the "style-graphic-fill-properties-attlist" symbol into the <define name="style-page-layout-properties-attlist"> symbol in the schema. I'm not sure, whether we need to exclude some of the attributes in the spec description and how this has to be done. Perhaps we need to extract some attributes from the <define name="style-graphic-fill-properties-attlist"> and use them instead.

The fo:background-color attribute is wrapped in a <define name="common-background-color-attlist"> symbol in the schema.

I prefer to have the draw:fill-* attributes at the same level as the fo:* attributes. ODF1.2 has already a rule how to handle draw:fill and fo:background-color, if both attributes exist on the same style, in the section 20.175.

So here it would mean to exchange the erroneous "draw:fill" with a "loext:fill", same for all other attributes on which the validator reports an error. All kind of fill styles have to be checked.

I know, that for paragraph background the way with new <loext:graphic-properties> was chosen. But the paragraph style has a different context. It is a <style:style> element and the selection of the attributes is done by family, whereas the page background belongs to a <style:page-layout> element.

Thorsten: Is it OK for you?

Luke: Do you want to work on this issue? If yes, then assign it to you. And if you have finished work on the issue, please add the new use of "loext:" namespace to https://wiki.documentfoundation.org/Development/ODF_Implementer_Notes/List_of_LibreOffice_ODF_Extensions
Comment 11 Luke Deller 2017-07-30 03:04:07 UTC
Created attachment 134987 [details]
Test doc (before change) with illegal fill-related attributes

Attached is a test document (before change) with various background fills on the page, header and footer.  These fill-related properties are saved as invalid attributes on the elements <style:page-layout-properties> and <style:header-footer-properties>
Comment 12 Luke Deller 2017-07-30 03:07:03 UTC
Created attachment 134988 [details]
Test doc (after proposed change) using loext: namespace

Here is the same test document, but using attributes on the loext: namespace as proposed.

Looks good?
Comment 13 Regina Henschel 2017-07-30 12:23:47 UTC
The file with loext: namespace looks good. Please test in addition, that the loext: namespace is not written in case, you set the save option to "1.2" instead of "1.2 Extended (recommended)" in Tools > Options > Load/Save. It should be correct automatically, but to be sure.
Comment 14 Regina Henschel 2017-08-16 14:03:30 UTC
Putting the needed attributes somehow and without plan into a private namespace, would make the documents validate against "ODF 1.2. extended". But such solution looks "quick&dirty" to me. A concept is needed how the specification for pages in Writer and Calc has to be changed. For example a solution for page-wide background images is effected by such concept too. So please develop such concept _before_ implementing a solution.
Comment 15 Regina Henschel 2017-08-17 08:19:22 UTC
Created attachment 135608 [details]
Manually applied the suggested changes

I suggest a different approach:
Add the attribute draw:style-name to the element <style:master-page> and let is refer a <style:style> element with style:family="drawing-page".

This family allows all the needed attributes.

Advantage:
(1) No private namespace needed. The document validates against the ODF1.2 schema.
(2) The style family "drawing-page" provides the attribute draw:background-size. With its value "full" it would be possible implement the long demanded background fill till page edge without any change to ODF1.2.

It might be needed to make some additions to the textual part in the ODF1.2 specification in "19.219.27<style:master-page>" to explicitly allow this for text documents and spreadsheets too.
Comment 16 Luke Deller 2017-08-28 12:48:10 UTC
Reusing style:family="drawing-page" certainly does sound more attractive than using a private namespace.

What would we do with the header and footer backgrounds, currently represented as attributes on <style:header-footer-properties>?  (See attachment 134987 [details] for an example of this).
Comment 17 Regina Henschel 2017-08-28 14:07:48 UTC
I have discussed this with Thorsten. He too thinks, that for the page properties we should use a <style:style> element of family "drawing-page" and the reference in the draw:style-name attribute.

For header and footer exists no solution with an existing element. Similar filling problems exist with paragraph, section, table, table-row and table-cell. They all have the attribute fo:background-color and the child <style:background-image> but no complex fillings with gradient, hatch or transparency. Therefore I currently investigate, whether a common solution is possible. I'm working on a solution for the schema, but I have not finished yet. There is my mail https://lists.freedesktop.org/archives/libreoffice/2017-August/078296.html. In case you have ideas, we can discuss it there.

So if you want to start, I suggest to first handle those parts, which can be solved in a "drawing-page". That is independent from the header-footer-properties. Unfortunately it is more complex than adding a namespace. Are you interested nevertheless?
Comment 18 Luke Deller 2017-08-29 13:17:43 UTC
Okay that sounds like a good plan.  Yes it does look more complex because Writer does not currently understand this style family, but I am reading through the code for importing this into Draw and will have a look into how this might be refactored to be used in Writer also.
Comment 19 Regina Henschel 2017-08-30 23:33:06 UTC
Created attachment 135889 [details]
An analysis and some versions of schema changes

I have examined, how changes of the schema might work. There is no decision yet, but I want to inform you about the current state.
Comment 20 Luke Deller 2017-08-31 13:24:40 UTC
Thank you for sharing that Regina.

Regarding variants A & B, and how LibreOffice can write valid documents before ODF 1.3: would we just emit these attributes in the "loext:" namespace, otherwise the same as what you have described, until ODF 1.3?  I appreciate that having so many attributes in the extension namespace is not particularly nice.

Regarding the issues with using <style:graphic-properties> that you mention, do you think we have a similar situation with <style:drawing-page-properties> already?  I am thinking of the attributes it can have which are intended only for presentation documents, such as smil:type for page transition effects.  I see some constraint suggested by the last sentence of section 16.39:

> Within presentation documents, the draw page style may contain presentation
> formatting properties.

(This seems a bit loose - it does not define precisely what "presentation formatting properties" means, nor does it explicitly specify the inverse: that non-presentation documents should not contain these properties)
Comment 21 Regina Henschel 2017-08-31 15:51:28 UTC

(In reply to Luke Deller from comment #20)
> Regarding variants A & B, and how LibreOffice can write valid documents
> before ODF 1.3: would we just emit these attributes in the "loext:"
> namespace, otherwise the same as what you have described, until ODF 1.3?  I
> appreciate that having so many attributes in the extension namespace is not
> particularly nice.


Yes putting attributes into loext namespace works for validators. Although the algorithm in http://docs.oasis-open.org/office/v1.2/os/OpenDocument-v1.2-os-part1.html#ForeignElementsAndAttributes does not explicitly mention attributes. (add this to my ToDo-List for ODF TC) So technical version B would be possible too.

Nevertheless it would generate a conflict for drawing and presentation documents having the same attributes in <style:page-layout-properties> and <style:drawing-page-properties> in case "style-graphic-fill-properties-attlist" would be add to <style:page-layout-properties> in the schema. Therefore I have suggested to allow <style:drawing-page-properties> for text documents and spreadsheets. That gives a unique place for page background formatting. Therefore I suggest to use <style:drawing-page-properties> also in Version B.

> 
> Regarding the issues with using <style:graphic-properties> that you mention,
> do you think we have a similar situation with
> <style:drawing-page-properties> already?

I think not, because <style:drawing-page-properties> only includes "style-graphic-fill-properties-attlist", so no conflict with margin/border/padding settings.

  I am thinking of the attributes it
> can have which are intended only for presentation documents, such as
> smil:type for page transition effects.  I see some constraint suggested by
> the last sentence of section 16.39:
> 
> > Within presentation documents, the draw page style may contain presentation
> > formatting properties.
> 
> (This seems a bit loose - it does not define precisely what "presentation
> formatting properties" means, nor does it explicitly specify the inverse:
> that non-presentation documents should not contain these properties)

And it does not explicitly specify, that these element cannot be used for text documents and spreadsheet documents. When the wording is improved, that can be added in the same run. You are right, that the description in section 16.39 needs improvement, perhaps something like "attributes in presentation and smil namespace are only applicable to presentation documents".



I see, that I have named the diffs different from my analysis :(

So to be clear, the difference is whether to include characters for gradient and hatch background fill, and whether to use a new element wrapping the attributes or use the attributes directly.

And I notice that using attributes directly in paragraph-properties and text-properties gives a problem, because the <style:style> element of family paragraph contains both paragraph-properties and text-properties. 

So perhaps better wrap the attributes as in version C/D ?
Comment 22 Michael Stahl 2017-09-08 14:49:32 UTC
there is already bug 94768 about the draw:fill attributes,
where i've fixed it to use the "loext" namespace,
but probably these are additional cases which i didn't notice back then.
Comment 23 Regina Henschel 2017-10-15 16:10:58 UTC
I have written https://issues.oasis-open.org/browse/OFFICE-3937. It contains the proposal to allow <style:drawing-page-properties> for all kind of documents. If the TC agrees, the page fill properties can be written to a <style:style> element of family drawing-page, which is then referenced in the master style be a draw:style-name attribute. Because such would not violate the schema, an immediate implementation without loext namespace would be possible.
Comment 24 Armin Le Grand (CIB) 2017-10-18 07:34:33 UTC
Hi Regina, thanks!