Bug Hunting Session
Bug 82216 - FILEOPEN: DOCX with OLE Chart missing chart name
Summary: FILEOPEN: DOCX with OLE Chart missing chart name
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: filters and storage (show other bugs)
Version:
(earliest affected)
4.3.0.4 release
Hardware: All All
: high major
Assignee: Markus Mohrhard
URL:
Whiteboard: target:5.0.0
Keywords: bibisected, bisected, filter:docx, regression
: 89002 (view as bug list)
Depends on: 90314
Blocks: OOXML-Chart OOXML-2007
  Show dependency treegraph
 
Reported: 2014-08-06 00:46 UTC by Yousuf Philips (jay) (retired)
Modified: 2019-02-19 22:14 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
LibO 4.2.6 VS 4.3.1 (50.07 KB, image/png)
2014-08-06 00:46 UTC, Yousuf Philips (jay) (retired)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yousuf Philips (jay) (retired) 2014-08-06 00:46:00 UTC
Created attachment 104115 [details]
LibO 4.2.6 VS 4.3.1

Steps:
1) Open attachment 104062 [details]
2) Notice that the chart title 'Chart Title' isnt there.

Tested in 4.4 and 4.3.1 on Linux. Regression that started in 4.3.x.
Comment 1 Joel Madero 2014-08-06 02:05:25 UTC
Bodhi Linux 2.4 - E17
LibreOffice 4.4 Built August 7, 2014

Confirmed:
New
Major - at least for user it appears like loss of data
High - default seems fine
Comment 2 Xisco Faulí 2014-08-06 11:41:07 UTC
Regression does not appear in latest version of 43all and must be younger. Add
bibisected and bibisectednewer as specified in the QA Wiki
Comment 3 Michael Stahl (CIB) 2014-08-11 10:42:57 UTC
bibisect range
a2aa0e6087254165613d323a5cfb083c79e7bbd9..113866583c5a34b9242aeb9040c4ce80e65da1ba

regressoin from:

commit 2f74e52a6efe7a51d6575cbb9b5f30b1ad99ee7c
Author:     Markus Mohrhard <markus.mohrhard@collabora.co.uk>
AuthorDate: Sun Jun 22 04:47:36 2014 +0200

    use the correct default value, fdo#78080
Comment 4 Markus Mohrhard 2014-08-12 04:56:35 UTC
Let us start by realizing that this file has been written by MSO 2007 and our current import is valid according to OOXML.

The funny part is that 2007 is expecting most default values not as specified by the standard. So for me this is not a real regression but an enhancement. At some point we need to implement another import filter that deals with the differences between OOXML and the MSO 2007 dialect.
Comment 5 Michael Stahl (CIB) 2014-08-12 08:44:10 UTC
imho what OOXML standard says is only of academic interest;
we should do what MSO does when importing these files.

would it be possible to add something like the generator-based
checks in xmloff (which is used to apply fix-ups for files
written by old and buggy OOo versions) to the OOXML filters too,
to have the defaults depend on the MSO version that produced
the file?
Comment 6 Markus Mohrhard 2014-08-13 22:36:00 UTC
(In reply to comment #5)
> imho what OOXML standard says is only of academic interest;
> we should do what MSO does when importing these files.

Modern MSO versions expect the OOXML spec behavior while 2007 and 2003 with the OOXML extension write the wrong files.

You can now decide which behavior is the one that we want to implement by default.


> 
> would it be possible to add something like the generator-based
> checks in xmloff (which is used to apply fix-ups for files
> written by old and buggy OOo versions) to the OOXML filters too,
> to have the defaults depend on the MSO version that produced
> the file?

Yes. And that is what I meant with another import filter. However it is quite a bit of work as quite a bit of default values have been changed already based on bug reports. Currently I see it as low importance as more and more 2010+ documents are produced and less 2007 ones.
Comment 7 Xisco Faulí 2014-10-20 09:10:12 UTC
It seems that the commit that caused this regression was identified. (Or at
least a commit is suspected as the offending one.)

Thus setting keyword "bisected".
Comment 8 Luke 2015-03-29 02:35:52 UTC
I can confirm that saving this file with MS 2013 resolves the issue with the missing chart title (on save it warns that it's upgrading to a newer OOXML). 


(In reply to Markus Mohrhard from comment #6)
> Modern MSO versions expect the OOXML spec behavior while 2007 and 2003 with
> the OOXML extension write the wrong files. You can now decide which behavior 
> is the one that we want to implement by default.

Are you saying that we need to pick one version and can only import that version correctly? If so, this is not an acceptable answer to corporate users.

I worked in IT at a company that had different versions of MS office in different departments that sent and received files from clients with a wide range of MS Office versions.  The only time I ever encountered an MSO interoperability issue was with a client that used an old version of the Mac MSO.  MS Office somehow manages to set the correct import/export defaults for all the major MSO versions. 

I agree with Michael that we must also do this. If we chose to only read one idealized version of OOXML, it will reflect poorly on us and limit the number of users that can use us to migrate away from MSO.
Comment 9 Luke 2015-03-29 02:52:32 UTC
Added feature enhancement request Bug 90314 - filter: Add support for multiple OOXML versions 

Please feel free to correct any technical errors I may have made in my understanding of our current OOXML import system.
Comment 10 Yousuf Philips (jay) (retired) 2015-03-29 12:18:58 UTC
Markus has already added in code to detect the MSO 2007 dialect.

https://mmohrhard.wordpress.com/2015/03/11/supporting-more-ooxml-dialects-in-chart-import/

http://cgit.freedesktop.org/libreoffice/core/commit/?id=15174177091367332b57cd79575e2f7dd27388b2

http://cgit.freedesktop.org/libreoffice/core/commit/?id=23a1717881ebfa3638b969aa4bad38a81d26d29d

(In reply to Markus Mohrhard from comment #6)
> Modern MSO versions expect the OOXML spec behavior while 2007 and 2003 with
> the OOXML extension write the wrong files.
> 
> You can now decide which behavior is the one that we want to implement by
> default.

I believe the default should be the OOXML spec and we should implement workarounds to hand the 2007/2003 exceptions.

(In reply to Luke from comment #9)
> Added feature enhancement request Bug 90314 - filter: Add support for
> multiple OOXML versions 

I believe Markus's above commits handle the 2007/2003 exception, but believe it should be extended to know if its a 2010 ooxml files as well, because there are ooxml differences that show up between these versions as mentioned in bug 77794.
Comment 11 Markus Mohrhard 2015-03-29 14:26:37 UTC
(In reply to Jay Philips from comment #10)
> Markus has already added in code to detect the MSO 2007 dialect.
> 
> https://mmohrhard.wordpress.com/2015/03/11/supporting-more-ooxml-dialects-in-
> chart-import/
> 
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=15174177091367332b57cd79575e2f7dd27388b2
> 
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=23a1717881ebfa3638b969aa4bad38a81d26d29d


There are still a few dozen cases that need to be adapted. However this has to wait until a later time as the customer document only had this one problem.

> 
> (In reply to Markus Mohrhard from comment #6)
> > Modern MSO versions expect the OOXML spec behavior while 2007 and 2003 with
> > the OOXML extension write the wrong files.
> > 
> > You can now decide which behavior is the one that we want to implement by
> > default.
> 
> I believe the default should be the OOXML spec and we should implement
> workarounds to hand the 2007/2003 exceptions.
> 
> (In reply to Luke from comment #9)
> > Added feature enhancement request Bug 90314 - filter: Add support for
> > multiple OOXML versions 
> 
> I believe Markus's above commits handle the 2007/2003 exception, but believe
> it should be extended to know if its a 2010 ooxml files as well, because
> there are ooxml differences that show up between these versions as mentioned
> in bug 77794.

The differences between MSO 2010 and MSO 2013 are totaly different to the chart problems with MSO 2007. They changed default values between these versions whereas the MSO 2010 and MSO 2013 versions don't require to know who produced the file. They are in the file in either MCE records or ExtLst records which we just ignore right now.
Comment 12 Yousuf Philips (jay) (retired) 2015-03-29 15:03:20 UTC
(In reply to Markus Mohrhard from comment #11)
> There are still a few dozen cases that need to be adapted. However this has
> to wait until a later time as the customer document only had this one
> problem.

True that the fix related to a single case from your customer, but the part of the code that i was addressing in my comment relates to the push having MSO 2007 detection in it, which is the primary discussion between you and Michael from comment 4 to comment 6. :D
Comment 13 Luke 2015-04-11 02:41:49 UTC
by changing oox/source/drawingml/chart/chartspacefragment.cxx :
-mrModel.mbAutoTitleDel = rAttribs.getBool( XML_val, !bMSO2007Document );
+mrModel.mbAutoTitleDel = rAttribs.getBool( XML_val, false );

and oox/source/drawingml/chart/chartspacemodel.cxx

-mbAutoTitleDel( !bMSO2007Doc ),
+mbAutoTitleDel( false ),

The chart tile is correctly displayed. Is the !bMSO2007Document detection logic turned off or just not working correctly in this case?
Comment 14 Markus Mohrhard 2015-04-13 19:47:53 UTC
(In reply to Luke from comment #13)
> by changing oox/source/drawingml/chart/chartspacefragment.cxx :
> -mrModel.mbAutoTitleDel = rAttribs.getBool( XML_val, !bMSO2007Document );
> +mrModel.mbAutoTitleDel = rAttribs.getBool( XML_val, false );
> 
> and oox/source/drawingml/chart/chartspacemodel.cxx
> 
> -mbAutoTitleDel( !bMSO2007Doc ),
> +mbAutoTitleDel( false ),
> 
> The chart tile is correctly displayed. Is the !bMSO2007Document detection
> logic turned off or just not working correctly in this case?

The conditions are correct as can be seen by the unit test. Also a quick look into the spec, oart1 21.2.2.7 shows that the default value in OOXML is true.

Of course if your document is not detected as MSO2007 which might not be the case for DOCX then the code will still use the OOXML values. So best to create a new bug report for the problem that DOCX 2007 documents are recognized early enough as MSO 2007 documents.
Comment 15 Yousuf Philips (jay) (retired) 2015-04-13 21:35:30 UTC
(In reply to Markus Mohrhard from comment #14)
> The conditions are correct as can be seen by the unit test. Also a quick
> look into the spec, oart1 21.2.2.7 shows that the default value in OOXML is
> true.

Could you point to the unit test, as it wasnt in any of the 3 commits in your blog post.

> Of course if your document is not detected as MSO2007 which might not be the
> case for DOCX then the code will still use the OOXML values. So best to
> create a new bug report for the problem that DOCX 2007 documents are
> recognized early enough as MSO 2007 documents.

It should be noted that the chart app seems to be exporting MSO2007 charts, as reopening an LO saved xlsx in LO shows no chart title (bug 90530).
Comment 16 Markus Mohrhard 2015-04-13 21:44:07 UTC
(In reply to Jay Philips from comment #15)
> (In reply to Markus Mohrhard from comment #14)
> > The conditions are correct as can be seen by the unit test. Also a quick
> > look into the spec, oart1 21.2.2.7 shows that the default value in OOXML is
> > true.
> 
> Could you point to the unit test, as it wasnt in any of the 3 commits in
> your blog post.

http://cgit.freedesktop.org/libreoffice/core/commit/chart2/qa?id=e6dbd5b03cf923fa505f8313fbae56f2d287be30

> 
> > Of course if your document is not detected as MSO2007 which might not be the
> > case for DOCX then the code will still use the OOXML values. So best to
> > create a new bug report for the problem that DOCX 2007 documents are
> > recognized early enough as MSO 2007 documents.
> 
> It should be noted that the chart app seems to be exporting MSO2007 charts,
> as reopening an LO saved xlsx in LO shows no chart title (bug 90530).

The export code as a huge part of the import code are a mix of MSO 2007 and OOXML export. We are slowly moving to a pure OOXML export and an import that handles both.
Comment 17 Yousuf Philips (jay) (retired) 2015-04-13 22:05:11 UTC
(In reply to Markus Mohrhard from comment #16)
> The export code as a huge part of the import code are a mix of MSO 2007 and
> OOXML export. We are slowly moving to a pure OOXML export and an import that
> handles both.

Thanks for the info and the link.
Comment 18 Luke 2015-04-15 02:55:26 UTC
Jay Philips
Is your attachment 104062 [details] a native 2007, 2010 or LO export? If it's 2007, Markus wants a new bug report that 2007 DOCX documents are not recognized early enough as MSO 2007 documents.

If it's a LO export, then we need a bug report that exported .DOCX files are recognized as 2007 instead of strict OOXML. (DOCX analog to Bug 90530)

If it's 2010, I give up :)
Comment 19 Yousuf Philips (jay) (retired) 2015-04-15 08:28:47 UTC
(In reply to Luke from comment #18)
> Is your attachment 104062 [details] a native 2007, 2010 or LO export? If
> it's 2007, Markus wants a new bug report that 2007 DOCX documents are not
> recognized early enough as MSO 2007 documents.

I had tested the doc when i was triaging bug 82182, so i'm not 100% of its origins, but looking into app.xml, it says the doc was modified for 2-3 minutes in word 2007.

> If it's a LO export, then we need a bug report that exported .DOCX files are
> recognized as 2007 instead of strict OOXML. (DOCX analog to Bug 90530)

Definitely not an LO export as synerzip does roundtrip work from MS to LO and then back.
Comment 20 Commit Notification 2015-04-19 05:24:09 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=914b4fb5a3c159bfd7cd8be2639185144f11d7ed

import chart MSO 2007 streams correctly for docx files, tdf#82216

It will be available in 5.0.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 21 Luke 2015-05-11 06:59:12 UTC
*** Bug 89002 has been marked as a duplicate of this bug. ***
Comment 22 Robinson Tryon (qubit) 2015-12-17 04:36:15 UTC Comment hidden (obsolete)
Comment 23 vihsa 2017-06-02 02:32:41 UTC
verified
chart title 'Chart Title' is visible, chart is not visibleVersion: 5.5.0.0.alpha0+
Build ID: 066665644b398a882e6cded98af5bb060af41d76
TinderBox: Android-ARM@24-Bytemark-Hosting, Branch: Master, Time: 2017-06-01 00:30:43