Bug 51222

Summary: [PATCH] XSSFColor.getARGBHex() returns wrong color for Excel 2007 xlsx file
Product: POI Reporter: jxz164
Component: XSSFAssignee: POI Developers List <dev>
Severity: critical CC: gjungman
Priority: P2 Keywords: PatchAvailable
Version: 3.8-dev   
Target Milestone: ---   
Hardware: All   
OS: All   
Attachments: Java code and sample xls file to extract cell foreground color of Excel 2007 file
Themes.xlsx - Shows colors in theme number order
patch.tar.gz – Changed files, changed test cases, new files

Description jxz164 2011-05-18 19:38:43 UTC
Created attachment 27026 [details]
Java code and sample xls file to extract cell foreground color of Excel 2007 file

I am using poi 3.8 beta 2 to extract the foreground fill color of cells for Excel 2007 xlsx spreadsheet. I found that the color is wrong for the attached xlsx file.

Specifically, please look at cells A4 and A5. In Excel 2007, the foreground color in hex for A4 is EEECE1, and for A5 is 1F497D.

However, through the poi library, the colors are reversed (A4 gets 1F497D and A5 gets EEECE1).

I am also attaching the java code so that you can reproduce this problem.

Comment 1 Nick Burch 2011-05-20 20:50:08 UTC
There seems to be an issue with how we look up colours in the themes table. I've added a unit test in r1125559 that shows the issue. A4 references theme 2, A5 references theme 5, and in the themes table the colours look to be the wrong way around.

More investigation is needed to figure out why the themes appear to be the wrong way round in the file
Comment 2 Nick Burch 2011-05-20 21:01:12 UTC
Having read the spec again, I'm with POI on this one. The file really does seem to be requesting the colours in the wrong order!

There's presumably something somewhere the inverts a meaning of something, or swaps something round, but I've no idea what or where, and everything I've read says we're doing the right thing...
Comment 3 Mick Sear 2011-11-15 21:42:18 UTC
I've also had this problem.  3.8 beta 4 seems to invert black and white.

I tested with 3.8 beta 4, which shows the problem, and 3.7, which is correct.

	public void testXSSFColor(){
		//Ver 3.8beta4 of poi-xml seems buggy
		Color c = new Color(0,0,0);
		XSSFColor c2 = new XSSFColor(c);
		//Actually, ver 3.7 seems wrong too - Shouldn't ARGB black be FF000000?
		//3.8 beta 4 returns FFFFFFFF
		//3.7 returns 000000
		Color c3 = new Color(255,255,255);
		XSSFColor c4 = new XSSFColor(c3);
		//3.8 beta 4 returns FF000000
		//3.7 returns FFFFFF

Net result is that if you specify new new XSSFColor(Color(0,0,0)) as the font colour, you get white text.
Comment 4 Randy Gettman 2013-01-09 20:52:43 UTC
Created attachment 29832 [details]
Themes.xlsx - Shows colors in theme number order
Comment 5 Randy Gettman 2013-01-09 20:53:26 UTC
Created attachment 29834 [details]
patch.tar.gz – Changed files, changed test cases, new files
Comment 6 Randy Gettman 2013-01-09 20:54:26 UTC
I believe I have the explanation for why these specific colors get reversed.  This also explains why black and white get reversed in Bugs 51236, 52079, and 53274.

Whenever certain Theme colors are used, the following colors get swapped in POI:
- FFFFFF and 000000 (white and black) (themes 0 and 1, respectively)
- EEECE1 and 1F497D (tan and dark blue) (themes 2 and 3, respectively)

Here is an excerpt from a typical “theme1.xml” file from an .xlsx file:

<a:clrScheme name="Office">
  <a:dk1><a:sysClr val="windowText" lastClr="000000"/></a:dk1>
  <a:lt1><a:sysClr val="window" lastClr="FFFFFF"/></a:lt1>
  <a:dk2><a:srgbClr val="1F497D"/></a:dk2>
  <a:lt2><a:srgbClr val="EEECE1"/></a:lt2>
  <a:accent1><a:srgbClr val="4F81BD"/></a:accent1>
  <a:accent2><a:srgbClr val="C0504D"/></a:accent2>
  <a:accent3><a:srgbClr val="9BBB59"/></a:accent3>
  <a:accent4><a:srgbClr val="8064A2"/></a:accent4>
  <a:accent5><a:srgbClr val="4BACC6"/></a:accent5>
  <a:accent6><a:srgbClr val="F79646"/></a:accent6>
  <a:hlink><a:srgbClr val="0000FF"/></a:hlink>
  <a:folHlink><a:srgbClr val="800080"/></a:folHlink>

However, theme 0 is NOT black, it’s white, and theme 1 is NOT white, it’s black.  Additionally, theme 2 is NOT dark blue, it’s tan, and theme 3 is NOT tan, it’s dark blue.  (The other theme colors are in order.) 
I’ve attached a spreadsheet “Themes.xlsx” that contains one cell per row, with text cells colored in theme color order (0-11).

Changing the order of the color elements in the color scheme in theme1.xml does not affect the colors of the text in the resultant spreadsheet.  Therefore, Excel is not taking the theme number as an array index into the list of theme colors; it must be mapping the theme number to specific theme color elements:
- 0 => a:lt1 (LISTED SECOND in theme1.xml!)
- 1 => a:dk1 (LISTED FIRST!)
- 2 => a:lt2 (LISTED FOURTH!)
- 3 => a:dk2 (LISTED THIRD!)
- 4 => a:accent1
- 5 => a:accent2
- 6 => a:accent3
- 7 => a:accent4
- 8 => a:accent5
- 9 => a:accent6
- 10 => a:hlink
- 11 => a:folHlink

POI versions 3.8 and 3.9 applied a fix to this issue in XSSFColor.java and ThemesTable.java that is incorrect, namely, using the “correctRGB” method in XSSFColor.java, which flips black and white.  But the fix itself is incorrect.  It did not resolve the issue or explain why black and white get reversed.

I have attached a patch that makes the following changes to resolve the above issues:
- src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFColor.java: Removed the flawed “correctRGB” method, and removed all calls to it.  (It was private, so all calls to it were only in XSSFColor.java.)
- src/ooxml/java/org/apache/poi/xssf/model/ThemesTables.java: Introduced a Map of theme numbers to XSSFColors.  Created a new private method “cacheThemeColors” that is called by the constructors to initialize the Map.  Moved the logic that extracts the XSSFColor from the theme color element to a new private method “getXSSFColor”.  The existing method “getThemeColor” now accesses the Map to retrieve an XSSFColor.
- src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFColor.java: Changed a few incorrect “assertEquals” statements.  When tinting white darker, the black/white flip no longer occurs, so white (255, 255, 255) gets tinted darker to (165, 165, 165), not to (0, 0, 0); the byte array is really [-91, -91, -91].  Later, when assigning the color black, now the correct byte array [0, 0, 0] is passed in instead of [-1, -1, -1], and the ARGB string is expected to be “FF000000” instead of “FFFFFFFF”.
- src/ooxml/testcases/org/apache/poi/xssf/usermodel/extensions/TestXSSFCellFill.java: This attempted to verify a tinted theme color, dark blue (dk2), but the RGB codes in the test were for tan (lt2).  These values are now corrected.
- src/ooxml/testcases/org/apache/poi/xssf/model/TestThemesTable.java: A new JUnit test for ThemesTable.java.
- test-data/spreadsheet/Themes.xlsx: This new spreadsheet is the same as the one mentioned above.  It helps test ThemesTable.java in TestThemesTable.java.
Comment 7 Randy Gettman 2013-01-24 00:33:14 UTC
I guess I needed to set the status of this bug too.
Comment 8 Nick Burch 2013-12-02 17:25:00 UTC
This doesn't appear to have been fixed yet, merely has a patch available, so re-opening and tagging as such
Comment 9 jason.gessel 2014-08-13 17:20:06 UTC
Is there a target release for incorporating this patch?
Comment 10 Nick Burch 2014-08-13 19:03:40 UTC
It would be good if someone could try the patch, and confirm:
 * Does it apply cleanly?
 * Do all existing unit tests pass?
 * Do new unit tests pass?
 * Do new unit tests cover all of the changed functionality, and all cases?
 * Does Excel then show the right thing?

If any of those are a no, then more work is needed before it can be applied
Comment 11 Andreas Beeker 2014-08-29 22:22:43 UTC
Applied with some modifications in r1621393

> * Does it apply cleanly?
> * Do all existing unit tests pass?
> * Do new unit tests pass?
Jenkins will tell us ;)

> * Do new unit tests cover all of the changed functionality, and all cases?
There are 12 (indexed) theme attributes, which are tested -> yes

> * Does Excel then show the right thing?
The TestThemesTable test can be used to generate themed and fixed color cells side-by-side, which looks good.
Comment 12 Yoann Rodière 2016-07-12 07:26:28 UTC
*** Bug 52079 has been marked as a duplicate of this bug. ***
Comment 13 Yoann Rodière 2016-07-12 07:27:27 UTC
*** Bug 51236 has been marked as a duplicate of this bug. ***