Bug 123139 - Reading XLSX format ignores horizontal alignment (generated with Apache POI Java library)
Summary: Reading XLSX format ignores horizontal alignment (generated with Apache POI J...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.2.8.2 release
Hardware: All All
: medium trivial
Assignee: Justin L
URL:
Whiteboard: target:7.4.0 target:7.6.0 target:7.4....
Keywords: bibisected, bisected, filter:xlsx, regression
: 144097 (view as bug list)
Depends on:
Blocks: XLSX-External-Generators
  Show dependency treegraph
 
Reported: 2019-02-03 09:51 UTC by Yuichi Sugimura
Modified: 2024-03-21 13:00 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
Calc ignores horizontal alignment of this XLSX (3.26 KB, application/zip)
2019-02-03 09:51 UTC, Yuichi Sugimura
Details
compare excel and LO 6.3 (44.00 KB, image/png)
2019-02-04 16:31 UTC, raal
Details
out_NoApplyCellXF5.xlsx: even applies in Excel 2016 with applyAlignment="0" (4.30 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2021-11-29 13:38 UTC, Justin L
Details
out_exaggerated.xlsx: includes applyProtection elements (9.01 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2021-12-01 11:38 UTC, Justin L
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yuichi Sugimura 2019-02-03 09:51:00 UTC
Created attachment 148870 [details]
Calc ignores horizontal alignment of this XLSX

I don't have full confidence wheather this is a bug or not because I don't have Microsoft Excel.
Anyway I've created a very simple XLSX file using Apache POI Java library. But LibreOffice Calc ignores horizontal alignment of cells.

It seems that this file completely conforms XLSX format. I checked its styles.xml and sheet1.xml file in the attached XLSX file.
Comment 1 raal 2019-02-04 16:30:41 UTC
I can confirm that result is different than in excel2010, althought I cannot reproduce it with file created in excel. So it's related to  Apache POI Java library.

Version: 6.3.0.0.alpha0+
Build ID: 1ac54de7869d5809a312acad0a37d48028ad9d3b
CPU threads: 4; OS: Linux 4.15; UI render: default; VCL: gtk3; 


regression, in version Version 4.1.0.0.alpha0+ (Build ID: efca6f15609322f62a35619619a6d5fe5c9bd5a)  is the result same as in excel.
Comment 2 raal 2019-02-04 16:31:12 UTC
Created attachment 148883 [details]
compare excel and LO 6.3
Comment 3 raal 2019-02-04 20:49:08 UTC
This seems to have begun at the below commit.
Adding Cc: to Noel Power ; Could you possibly take a look at this one?
Thanks
 7768846a5953a624076782f3cc0fbdcca5f5c160 is the first bad commit
commit 7768846a5953a624076782f3cc0fbdcca5f5c160
Author: Matthew Francis <mjay.francis@gmail.com>
Date:   Sat Sep 5 17:55:40 2015 +0800

    source-hash-bf8e9b29aaebcbdd8f2f06b42ac97b8d9f8f4503
    
    commit bf8e9b29aaebcbdd8f2f06b42ac97b8d9f8f4503
    Author:     Noel Power <noel.power@suse.com>
    AuthorDate: Wed May 22 10:00:34 2013 +0100
    Commit:     Noel Power <noel.power@suse.com>
    CommitDate: Wed May 22 10:03:26 2013 +0100
    
        fix for bnc#819865 itemstate in parent style incorrectly reported as set
    
        Problem occurs because attrs set with default values are reported as set when queried
    
        Change-Id: I89d6c3b09312fb78052d87ff20aa12c6fbe7bc98
Comment 4 SheetJS 2020-02-28 22:06:29 UTC
Running into this issue with spreadsheets generated by SheetJS Pro.

In the <xf> records, Excel assumes that applyAlignment is true if an <alignment> child is present.  You are expected to specify applyAlignment="0" if alignment does not apply.

LibreOffice (including the latest version 6.4.1.2) assumes that applyAlignment is false even if an <alignment> child is present.  The default behavior is incorrect.

To verify this is the root cause, you can take the original sample file and add the applyAlignment="1" attribute to the <cellXfs> <xf> tag:

```xml
    <xf numFmtId="0" fontId="0" fillId="0" borderId="0" xfId="0" applyAlignment="1">
      <alignment horizontal="left"/>
    </xf>
```
Comment 5 Justin L 2021-11-27 16:22:22 UTC
/*  Default value of the apply*** attributes is dependent on context:
    true in cellStyleXfs element, false in cellXfs element... */
maModel.mbAlignUsed  = rAttribs.getBool( XML_applyAlignment, !maModel.mbCellXf );

This came from the original import in 2008.
According to comment 4, this is not correct. The documentation I saw didn't mention anything about the default. Changing this to true fixes out.xlsx and passes a make check.

I texted out.xlsx with old version Excel 2003, and it matched excel in comment 2.
Comment 6 Justin L 2021-11-29 13:38:51 UTC
Created attachment 176571 [details]
out_NoApplyCellXF5.xlsx: even applies in Excel 2016 with applyAlignment="0"

Am I doing something wrong? I tried getting the apply to be false, but Excel seems to ignore it completely, so it always applies.
Comment 7 Justin L 2021-11-30 15:40:18 UTC
I wouldn't say that commit 3 is truly regressive - it seems to have just exposed some kind of flaw or poor handling in the import process.

I note that some handling has been done in 2021 Apache POI v5.0 that takes into account the adjustAlignment variable.


I'm not sure why commit 3 would be involved here.
I have narrowed it down to SheetDataBuffer::addColXfStyles, which tries to consolidate styles using addIfNotInMyMap(). In that comparison, it only checks if mbAlignUsed.
(stylesbuffer.cxx's bool operator==(rXf1, rXf2))
Comment 8 Justin L 2021-12-01 11:38:03 UTC
Created attachment 176617 [details]
out_exaggerated.xlsx: includes applyProtection elements

In the current implementation, the first style that is applied affects all cells. The alignment/protection settings are accepted regardless of whether it is considered "used" or not.

Proposed patch at http://gerrit.libreoffice.org/c/core/+/126177
Comment 9 Commit Notification 2021-12-08 17:32:57 UTC
Justin Luth committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/ab99d16e21e56596803b7d9a28af0895b435ef3e

tdf#123139 sc oox: CellXf should default to applyAlignment

It will be available in 7.4.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 10 Justin L 2021-12-08 17:38:12 UTC
No intention to backport a trivial fix which goes against documentation just because that matches Excel in testing.
Comment 11 Eike Rathke 2023-02-06 18:13:01 UTC
Fwiw, the actual documentation now is
https://learn.microsoft.com/en-us/openspecs/office_standards/ms-oe376/59922f8b-0edc-4e93-a822-9f22254aec46
and the initial 2008 code based on the standard plus observed MS implementation quirks was in the right direction but slightly off-track..

Note that doc for each attribute says "The standard does not define different behaviors for @apply... depending on whether the xf is in the CellXfs element or the CellStyleXfs element." and then continues explaining the actual difference between the two for each. Great. :-/
Comment 12 Commit Notification 2023-02-07 13:18:22 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/909a25d30b09ebd3a023105a9c3cc4d20add094a

Resolves: tdf#139934 always apply cellXfs xf explicits (tdf#123139 related)

It will be available in 7.6.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 13 Commit Notification 2023-02-13 11:53:09 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-7-4":

https://git.libreoffice.org/core/commit/e53c02eac4b3d231fef7a6156321edfdea1fbb27

Resolves: tdf#139934 always apply cellXfs xf explicits (tdf#123139 related)

It will be available in 7.4.6.

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 14 Commit Notification 2023-02-13 11:53:14 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-7-5":

https://git.libreoffice.org/core/commit/321c1a6db442a6d2afee25d1d1256ff36bdb004e

Resolves: tdf#139934 always apply cellXfs xf explicits (tdf#123139 related)

It will be available in 7.5.2.

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 15 Commit Notification 2023-02-22 18:34:30 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-7-5-1":

https://git.libreoffice.org/core/commit/58c84f408c4a2d5f1067b4ca5e7c204d6a9c9372

Resolves: tdf#139934 always apply cellXfs xf explicits (tdf#123139 related)

It will be available in 7.5.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 16 Justin L 2024-03-21 13:00:29 UTC
*** Bug 144097 has been marked as a duplicate of this bug. ***