From 9a13f526c53abb9cac00fc5a644bcfcd966e644d Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Thu, 25 May 2023 13:05:55 -0700 Subject: [PATCH] Redo Calculation of Color Tinting (#3580) * Redo Calculation of Color Tinting Fix #3550. Some colors are specified in Excel by specifying a theme color to which a tint is applied. The original PHPExcel algorithm for doing this was developed by trial and error, and is good enough a lot of the time. However, for the issue at hand, the resulting color is detectably different from the calculation that Excel makes. Searching the web, I found https://gist.github.com/Mike-Honey/b36e651e9a7f1d2e1d60ce1c63b9b633 which comes much closer for the case in hand, and for all the other cases that I've looked at. That code depends on Python colorsys package; I have adapted the code from the Python gist and package into a new Php class. This doesn't agree perfectly with Excel. However, if each of the red, green, and blue components (each a value between 0 and 255 inclusive) agree within plus or minus 3 (arbitrary choice) of Excel's result, I think that is good enough. I have added a new test member which reads from a spreadsheet with Xml altered by hand to set up several theme/tint cells. These tests use the plus-or-minus-3 criterion. They result in 100% code coverage of the new class. Unsuprisingly, some existing tests failed with the new code. Issue2387Test reads a theme/tint font color, and is changed to use the plus-or-minus-3 criterion, comparing against the color as Excel shows it. ColorChangeBrightness showed 9 failures with the new code. It consists of calculations not involving a spreadsheet. For that reason, I felt it was sufficient to just do an exact match test, changing the 9 old results for new results confirmed with the Python code. I also added one new test case, the one that kicked off this entire PR. * Scrutinizer Being Stupid It strikes again. --- src/PhpSpreadsheet/Style/Color.php | 17 +- src/PhpSpreadsheet/Style/RgbTint.php | 175 ++++++++++++++++++ .../Reader/Xlsx/Issue2387Test.php | 6 +- .../Reader/Xlsx/RgbTintTest.php | 50 +++++ tests/data/Reader/XLSX/RgbTint.xlsx | Bin 0 -> 11025 bytes .../Style/Color/ColorChangeBrightness.php | 23 ++- 6 files changed, 245 insertions(+), 26 deletions(-) create mode 100644 src/PhpSpreadsheet/Style/RgbTint.php create mode 100644 tests/PhpSpreadsheetTests/Reader/Xlsx/RgbTintTest.php create mode 100644 tests/data/Reader/XLSX/RgbTint.xlsx diff --git a/src/PhpSpreadsheet/Style/Color.php b/src/PhpSpreadsheet/Style/Color.php index 3c002b270..282defc0c 100644 --- a/src/PhpSpreadsheet/Style/Color.php +++ b/src/PhpSpreadsheet/Style/Color.php @@ -362,23 +362,8 @@ class Color extends Supervisor $green = self::getGreen($hexColourValue, false); /** @var int $blue */ $blue = self::getBlue($hexColourValue, false); - if ($adjustPercentage > 0) { - $red += (255 - $red) * $adjustPercentage; - $green += (255 - $green) * $adjustPercentage; - $blue += (255 - $blue) * $adjustPercentage; - } else { - $red += $red * $adjustPercentage; - $green += $green * $adjustPercentage; - $blue += $blue * $adjustPercentage; - } - $rgb = strtoupper( - str_pad(dechex((int) $red), 2, '0', 0) . - str_pad(dechex((int) $green), 2, '0', 0) . - str_pad(dechex((int) $blue), 2, '0', 0) - ); - - return (($rgba) ? 'FF' : '') . $rgb; + return (($rgba) ? 'FF' : '') . RgbTint::rgbAndTintToRgb($red, $green, $blue, $adjustPercentage); } /** diff --git a/src/PhpSpreadsheet/Style/RgbTint.php b/src/PhpSpreadsheet/Style/RgbTint.php new file mode 100644 index 000000000..582ae4839 --- /dev/null +++ b/src/PhpSpreadsheet/Style/RgbTint.php @@ -0,0 +1,175 @@ += 0.0) ? $hue : (1.0 + $hue); + } + + /** + * Convert red/green/blue to HLSMAX-based hue/luminance/saturation. + * + * @return int[] + */ + private static function rgbToMsHls(int $red, int $green, int $blue): array + { + $red01 = $red / self::RGBMAX; + $green01 = $green / self::RGBMAX; + $blue01 = $blue / self::RGBMAX; + [$hue, $luminance, $saturation] = self::rgbToHls($red01, $green01, $blue01); + + return [ + (int) round($hue * self::HLSMAX), + (int) round($luminance * self::HLSMAX), + (int) round($saturation * self::HLSMAX), + ]; + } + + /** + * Converts HLSMAX based HLS values to rgb values in the range (0,1). + * + * @return float[] + */ + private static function msHlsToRgb(int $hue, int $lightness, int $saturation): array + { + return self::hlsToRgb($hue / self::HLSMAX, $lightness / self::HLSMAX, $saturation / self::HLSMAX); + } + + /** + * Tints HLSMAX based luminance. + * + * @see http://ciintelligence.blogspot.co.uk/2012/02/converting-excel-theme-color-and-tint.html + */ + private static function tintLuminance(float $tint, float $luminance): int + { + if ($tint < 0) { + return (int) round($luminance * (1.0 + $tint)); + } + + return (int) round($luminance * (1.0 - $tint) + (self::HLSMAX - self::HLSMAX * (1.0 - $tint))); + } + + /** + * Return result of tinting supplied rgb as 6 hex digits. + */ + public static function rgbAndTintToRgb(int $red, int $green, int $blue, float $tint): string + { + [$hue, $luminance, $saturation] = self::rgbToMsHls($red, $green, $blue); + [$red, $green, $blue] = self::msHlsToRgb($hue, self::tintLuminance($tint, $luminance), $saturation); + + return sprintf( + '%02X%02X%02X', + (int) round($red * self::RGBMAX), + (int) round($green * self::RGBMAX), + (int) round($blue * self::RGBMAX) + ); + } +} diff --git a/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2387Test.php b/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2387Test.php index 870ea6ab0..7e59418ad 100644 --- a/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2387Test.php +++ b/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2387Test.php @@ -15,7 +15,11 @@ class Issue2387Test extends TestCase $reader = IOFactory::createReader('Xlsx'); $spreadsheet = $reader->load($filename); $sheet = $spreadsheet->getActiveSheet(); - self::assertSame('335593', $sheet->getCell('B2')->getStyle()->getFont()->getColor()->getRgb()); + // Font color being tested uses theme color with tint. + // Excel shows final color as 305496. + $expectedColor = '305496'; + $calculatedColor = $sheet->getCell('B2')->getStyle()->getFont()->getColor()->getRgb(); + self::assertSame($expectedColor, RgbTintTest::compareColors($calculatedColor, $expectedColor)); self::assertSame(Fill::FILL_NONE, $sheet->getCell('B2')->getStyle()->getFill()->getFillType()); self::assertSame('FFFFFF', $sheet->getCell('C2')->getStyle()->getFont()->getColor()->getRgb()); self::assertSame('000000', $sheet->getCell('C2')->getStyle()->getFill()->getStartColor()->getRgb()); diff --git a/tests/PhpSpreadsheetTests/Reader/Xlsx/RgbTintTest.php b/tests/PhpSpreadsheetTests/Reader/Xlsx/RgbTintTest.php new file mode 100644 index 000000000..96e2003bc --- /dev/null +++ b/tests/PhpSpreadsheetTests/Reader/Xlsx/RgbTintTest.php @@ -0,0 +1,50 @@ + $maxDiff) { + return $style; + } + if (abs($styleGreen - $textGreen) > $maxDiff) { + return $style; + } + if (abs($styleBlue - $textBlue) > $maxDiff) { + return $style; + } + + return $text; + } + + public function testRgbTint(): void + { + $filename = 'tests/data/Reader/XLSX/RgbTint.xlsx'; + $reader = IOFactory::createReader('Xlsx'); + $spreadsheet = $reader->load($filename); + $sheet = $spreadsheet->getActiveSheet(); + $row = 0; + while (true) { + ++$row; + $text = (string) $sheet->getCell("B$row"); + if ($text === '') { + break; + } + $style = $sheet->getStyle("A$row")->getFill()->getStartColor()->getRgb(); + self::assertSame($text, self::compareColors($style, $text), "row $row"); + } + $spreadsheet->disconnectWorksheets(); + } +} diff --git a/tests/data/Reader/XLSX/RgbTint.xlsx b/tests/data/Reader/XLSX/RgbTint.xlsx new file mode 100644 index 0000000000000000000000000000000000000000..0ef26da69fd3594067cb47a9b5170e75e7aaa853 GIT binary patch literal 11025 zcmeHt^(epf> z!#VF?@O}0Vdxj5tU7y+OUe~?Wwbrew2t*(NAOTPS000fZ8}gCH3l0DXL<9iv0Vwd# zr5qi=AP2Cqrl%9g)sWM}-i|sS0iO9a03P=I|E~YT9w*1uI^KC(?@Gb?Zb zoj;NrB@7vmZ1tI|0!K;&Oz6U?YP^vkVGL!#aW-S#P_qmmYn61@E21EcF$IZ9IPEc^ z9FFXkxj7sE5DglSSJHf~GVS!$#c+s#Pb!2lP;DuSaq+W8NlqVs{2#@nTos2(_kO~Fb+$4*sv!&?6}+=U2M%9 z9c}-}aAjKdjs-$Ew|)~h$UW|n!_VPuTF~IY$@5ytTk{nJ?_55hYQwqq@xLp%IT8(x zZ^)cvooQ#THXr?VnVxR-=_=Jw&w5moKUxQ$GMC1%Kx>ds+ny1+64gVpuTLkRoDie~ z&hHqLy>mI##k<*QgnVSlNlnt;X zgG}{NJT2vQi!M+Cg%E=)XV{ByRTd!HHORPi)Ob6B!ljvT?(I!=)i66jV%>ngJ^7koL0Q>KSaclh_doHEW z-1>!uFb8-LeJx8uk+r+7>Di6~HroSD&lapn3a4K%1Z?=?4U0R~o<*Gsu!_C~5VmN?%l45b@@MYQ>7H=hRME_f*y+J(57m|@I2ciE_|VlwSI9sAh=}4D5H|8n zY{ey+3B_Drj+K>mH(Cb|+!VL(E-+&{>Vkzfvvro6f{GS{2o2ITrGN2Qdl)mJ`E#UM z{%Fu<=s?kI8GY%7`jhBkUOEW-_L!9>J5_}PRmzctLag`NBb4MqRxkXnBbM)X{>q5l zgq_rV(HHEA!U(&fL``Sj{%Q>afjm7$5A%o#Y9nrK zdLsmD+7oc~8|;PN3-I-kc!uVBXh(Ahy2UT?@UE!sIcU88Jgh(c@3m_z;G>48;RQ`Glag=_z3-iQfB@*P@=&A03dYd0vN6@>9~R@Tl5J`SwXn^;dJ4 zZ%Jz(#I7-88ER3TKSa5W6Rn=~;cl}e7|KFv#-aBON}bHAkeUd14Gjo5@~+7f#Y(^= z$VOla4feHomA6gekTm-h>Z(Nul6R1@e5WXv1O(=A}W>L)?O`lV2ND6=RMfn zxLX~JQwL}jeagjK)QM44|H{`uiKNt1*ne7JDUJ+)0te&kuPpah*8V5s!NH1CSUUW_ zeU+&{Q|#f!X-B^e<95$}80vmj15wNl%O3adSb zsr*?WuABMkty)K4z)!9RSvnR@*oV%JEH?)Fr&nMr{m(7lH+J0@3%13dAp-y;uqXc9 z;$5vlAh0Xf&kvqI_WCS{Ufeu4LC6Z*`6FFFl>>He9ERvnxGB%l;-OY&hwzZpsfU>* zw-+LLj~R@{NhGS5#N zyHsL6pu?Et59;pvmJNS6ih~ZhQthyn+2-W$rRrp{Y;5IRlYI$SO{9J~3FwtOUb8QV z0ojuJP$gzLgX{8&@~nu{O>dOtQ5|CVV&Z`5S+?&RA2DcjT_%H%?r4-C_~_o_{;QbD zer1IgU{H~u?sv)AOSJvmAtF&VolC)IE+V;N!v$IIISYO;kUvcnHk6s%G~@9cgbeT& z`F|@blezq|Ojt9fdC9=Ch1zSOjRgl~SzWy)ryKpe@)-UCM;b30>QfOj_Z@kq3lCFH zkYXnx+PgYTC%&djoT%A2c<>Rb%XC_D84fK2d?^sFV4ui+PC?z zqPHsOPGW`p;r(;WNw>s*EbY(rO`3d;7R-%`qXyXDJv`3~YS-(iEs(XI zSh2gS7;G`@G6`usrhFPwKcv*+##nzgw*27rvpeLbXjPOd#H;0T)2F>c2C}Mas;k(L zFgQw}6R9Nn<3Qv{Zq+`3fh;rPY*6JRt`KCW_{4t$q zFmWaA&v1aODLVq>UUOop3#>%FD4_{WA1s9eR~V!F<+|u{ ziTWuhgO}5Y*oxH4N$4P#MnPt(-oCsvbO)Hp3jW^j+El*qBs{^4W?s=LsbO1TZ8`d{Tuy`hKVAY5*BdsB#4qBQf)l5Rh5=^X$s{G zskd=&o+q^E+KI(q#t4>dhp>#Q#bcHpsB0+c(h6?sM}Os&00h9R)61_7^kS%tq0}@* zF$=HuEHF%>;5mlkF`6`KvS2D`T~xFHlgXeWQt}{#NnD{D?8iG!X0h817Ma z5BHNE$Yb0cx3^!1KEfM4r{3#&S#{X8K%v`**e2FtJJ&*W+)N*vUMX9RxM&{jM&d5A zT9PsJ?VD4M(o*7Sb3|*zSc2_WPK7eS#Wvq-oi+8jm@3c~W+Yxju~6rPMn|<&G-YY4 zRQTqMIPaQLRm9m>oXX(vNd_qw69#q8Iw`a8p)L^htND}RW&6tRx3<&bJ{EMQu;0P$ z7yfRuZ^)Wc?Lb{vnn!^0eU!xpuZw@@Q^pD|dTFmnmQ$!X`zysQk+5?z$qqD(b*$?L zhCL4G%^Y9!ScjDadV>5;!+fNwpC*$ufhu!wqTekl&9%(X&eRBKqoM=)~sdSle%6yexkB z9)Z-yhLrkyFiQ{yRS)^vzZUEfdsPJ?f8isF}p^} zm93h~S~A|DBSNk`SPWvMQWc7K^u>>s%cu7Jb~N4fQjwVFCP_?{EXmbglBgMDJs~i% zdSx`1t~(Ca9ar5;&C{24-bCZJCnWUP{JtvFz8NMtFBxh{k+f}2_@*wsExH12Bwf}k zk?&nPoT?cYA9Wm9W3I*7)8^%il6ILw4!US?Z4CDWcM$OxcG2OKYrVdf^sp&jm1eVM zau>e67;Icy8Cs^MKt@K5u^v%}t9l!hMpin0+bjGXlI|G~q-MIQ=fwFMO>U&}IS!ZZ zrYWSvHDS_8A_j@dgGf9HY2Haa9L5~}$(a1(6#4dSIsrHm`s*2w*{Cv|aloPNXbO}UMd>U6_qasn6%jnG8=M+mT0e){+SrVk0 z9&|-|v1L`<%eprOPTUO>-M}n8k(#PuEpoU~=5VR7P`ZJ~~j9R)*W>fxpROIacD;|iunAG1H{zkJNTuaJES9b9aiwnTOh6!UWO z7kZ<dk z<8u@n29RHY-dp5k1<|*S1*UMeMmM0 zTZooHiBm$5K95GfYs=}kbaaguub2^V+T>%ri}s2@0$1YioN93dzI;<*u?Qhl+FZ7= zk#`#3c}7@kuz*#Lrr~ggGQ`?}P}p^Hn)lFEZCZDn5~N8f$_~`JI_b_qk8sC$+YlS) zIwg_^R%2697TI_AW6e+;4_D^^%;16KT~EZ9dKB2R;^YE;h|x1u*0D39OX|Q zzl27gmk<~TFcTwJP^UlEr0P83BR2LdzMJUtoA#Ri5%Yi>7-g=zuf>@bFDJRTx*>wz zPq?dNU@Gel*#j|DFpGY-)FE%N!`^2_v#N|q+2Ro+o0TRd*K5vz)BVi$F_#u%#Vy%= z`BgXAAFEP7ZnFG3ZrVE{E|%QjeI9N!QAY2r4Vge2G;%Mvme>zMUrlx8wt?(0i`YU= zq+t}FLhue3n%ystBeJH5a@XVUzQ=XcN7)Ug8DVxg+Q+xlVKv+MwFRO}ihf+Ws1WLg#s%5h1}5V!x8DJ+{jOfSoc)>-5a2@h06t zgY*woP%;#7f@t$b3(AkML>4M;^l1E2qwaw>TIuwU-KMu3qgb4?m=FQb%jFm+!`g!P zk3vQF;o0MJ=c*nnSa!o}rD3vV;j(4mqI(U@nZ2sX#T*JWOT)&1R?L0yQ#crsWOkvG zmtXj@bc9Bz;HrKXS8`|2FX!&|?l{ch6X{vF6ifb6Ag^_RpXybBUR^=Tto~7wLyc&% zus2){BFk~$yVld(cRaC=5`rGEGH;nqWBTHh1k1%Z|4>uRnOssU)S@D{rvDOw1u5=( zG?U3ZXJl1Dx{x`0CXSaTUp1Ez=wCnQP}}2nSn8Oledo^)T}nj_&s^ssN6H1dptw={ zi_WW?^X~4tm(z^pxTq|S`B4cqqp_%*C=!}cOR(?3MDeGss zL>WF+nsv0%CmI#Xq#61&^4vEvIG$Ko`*Rl{-H^Inr z9c5*%s2yfYGKt5z5|<(6j#i*e z<%JJnkR5jC7;9`m^F%L8T2k($-k4CJ^AxaEEQ4Zw`Eya`ml|5ipNj^cC$*%UTXt|vPbsF%zkE2#T<>sEem z*skNq<=;-bqVh|h-FJkjJ+ERX%mGhej-dPHh(DS&f9mgF-J9Pk{AcgxRa|Fe7dL^- z1@KnN!#nYF2!@n~AZ?S@HqZ<@kF<~)e?f5LT}uJ_u(|0py7@5kTI?OUB~}9aR118_ zhB>iy)i>{XjtUCoqh zLPhhY6Hvy9AVnsn@m=D;j?Y)lb&AUR>Ama2?t;7Qlfv07uBHd8gduCVtlU=>mY(OX zK|Z3Fg;i0Pu~JjVSR!{25i$co)!>Lm>l*C>Qr-13+)0Zi3r(vEKF6`fbb)c!8b^yW zrIjjn@H#UlA7S&2S{ig*5@iSjelj>w)->zllsV#yh|#J&7~aD)1;agxOvF;4=yw?P;QBLow#yv7a02`WH&CZ zUWur@tjQ7}%@1IvMhl1kmyb{RA2?u({Ld^-<`m;63k!o6u-+vutaoYQXs+tw=;X>} z?&t#gbDsXcmL<$}(U4A+ZfcU=BgR{7%nPFV^1k>6#i@fj8GI}LWt{i+Jx@?yFWq{- zXq?^?OLGdmxIA7aj+|4T9%s^JA~($Er(uR4CZ{lEvzT>Pc{k);f$U(s4nW2YF*#FRFBDU8IMR021kq>q(8tw+Wn4rUk~S)rP9J@Bm&8Wi zBU$tB;eTU6_Bq@GUK3H`nqnH(Df^e;GnGfhiGT$kF072i`D^f*IywC>_Fyji^U8uq zIxcWygd72(R2WO-!b_?cfn{cLI%id;aF7P8XT|1uHT2$sV&yhJJ*I5Y+l(p2rNa0c*0xKI)ki_!ce!h))s9 zq!p92)${H1_?k?Ft0sCDN!K18f$YevzC4^y#eyFwt!YbfqDB#zO5G)3F7DA%p~5UL zpuGI%?5>1SHr<%iRvUVm0)0~}Y-acN(ywYYIV&o1x531?rl5c`IjhmeHYQ8}dB4vC zfnfz{@4=_4x)^8D`n?y^!9{84V1EyLo%~CC9HcCTGe%MT@M@%mspxtYIlV`{KcYDR zc@Vp_P;=pn5!(wHuBSla?LHb4c+)Qw&f6b>w>A$MCq;XPgpWURA1ezj1UibijtCm1 zt3a8&DO)YN5<)c2yF;veyg zi`}lDb)TPjsrhkU8Cvl%tlZYzqW@d?hHoZL48y|r@YiXizwDhaUBQm_|BK-N#4i9) zm!ufA0Gnf6gxn$*wD&=1duyC|l^y_r*}U;}Q4T@uhR-mnE$kTVnQxAguhCe*>yVho zaumMXm(ZjO;|wX6po+@&Od7AJs_@F*8m%*=&??^^JO*FYk+;F_+)`FM<2OGJGf#?6 zIly_L5)pV6xp}@Q_6f!9%TfiC?;0CESDip-1JGIMEHd`YseFPL z9=YtiF>*#TXNZ{r8s0g3JJ5Y?`@!I+Y=>{dCuOW-)~t_8%l&*?Q3=&>)dymgCii4E zh=b}h>|8LZt{W7J_a?*o#`E;7#`_Jc&_Rg@<|`9jO$Bo-z0l-342}5D)*yb8*5{&H zlplI7RPu!Ca<9jRrQBcfwjT_g2`>1nEBC28P?u>2uqnvSB4F>1ao{565=8sApbAha z-e8y>RTeD$NShd0M9_>z^rd>b&H5au?VN$DgfYxR!?V@oWblH8Z;;7=#0?*PavWbB z*`sA-e#?gyEaS`wiftYstO#A{B$GeXhutJyCxYn>*PNduVK$Bx+#tqXBKdL8=e9G0D{VxY6T5(EU)xz>-IK`jbCcAmEk8od4{YH?dxbK61Raf@`@5__lfQpnq0q=>Ee-$hDLH}Ot z{)Pkq)M1nTf0VxW&HrAm{MCGv<}c=d7BBa$|DG-WY8^}e7i;tX<_=XwL|732(Okm> NSiyLZ^oL9X{6BIneOmwk literal 0 HcmV?d00001 diff --git a/tests/data/Style/Color/ColorChangeBrightness.php b/tests/data/Style/Color/ColorChangeBrightness.php index 8ddf188da..1c552e157 100644 --- a/tests/data/Style/Color/ColorChangeBrightness.php +++ b/tests/data/Style/Color/ColorChangeBrightness.php @@ -9,7 +9,7 @@ return [ ], // RGBA [ - 'FF99A8B7', + 'FF92A8BE', 'FFAABBCC', -0.1, ], @@ -20,17 +20,17 @@ return [ 0.1, ], [ - '99A8B7', + '92A8BE', 'AABBCC', -0.1, ], [ - 'FF1919', + 'FF1A1A', 'FF0000', 0.1, ], [ - 'E50000', + 'E60000', 'FF0000', -0.1, ], @@ -40,7 +40,7 @@ return [ 0.1, ], [ - 'E57373', + 'FF5959', 'FF8080', -0.1, ], @@ -50,7 +50,7 @@ return [ 0.15, ], [ - 'D80000', + 'D90000', 'FF0000', -0.15, ], @@ -60,18 +60,23 @@ return [ 0.15, ], [ - 'D86C6C', + 'FF4646', 'FF8080', -0.15, ], [ - 'FFF783', + 'FFF984', 'FFF008', 0.5, ], [ - '7F7804', + '847D00', 'FFF008', -0.5, ], + 'issue 3550' => [ + '558ED5', + '1F497D', + 0.39997558519241921, + ], ];