DOMMatrix is2D property incorrectly set to false
Reported by
gerryile...@gmail.com,
Aug 18 2017
|
||||||||||
Issue description
Chrome Version : 61.0.3163.49 (Official Build) beta (64-bit)
URLs (if applicable) :
Other browsers tested:
Add OK or FAIL, along with the version, after other browsers where you
have tested this issue:
Safari:
Firefox:
IE:
What steps will reproduce the problem?
(1) Open browser console
(2) Enter the following:
m = new DOMMatrix; m.m44 = 1; console.log(m.is2D)
What is the expected result?
According to https://drafts.fxtf.org/geometry/#dommatrix-attributes setting m33 or m44 to 1 should not set is2D to false so 'true' should be output.
What happens instead?
'false' is output.
Please provide any additional information below. Attach a screenshot if
possible.
It appears that all of the 3d mxy elements are being tested for 0 or -0 rather than m33 and m44 being tested for 1 instead.
,
Aug 21 2017
,
Aug 21 2017
I tested Firefox and it indeed outputs true, while Chrome outputs false.
,
Aug 21 2017
,
Aug 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ac6d4866aa926c3edde2154eb7c207a21d19aae9 commit ac6d4866aa926c3edde2154eb7c207a21d19aae9 Author: Fredrik Söderquist <fs@opera.com> Date: Tue Aug 22 05:49:33 2017 Setting DOMMatrix.m33 or m44 to 1 should preserve is2D Per [1], setting a value other than one (1) should clear the is2D flag. We had the logic reversed. [1] https://drafts.fxtf.org/geometry/#dommatrix-attributes Bug: 756789 Change-Id: I37fcd4e20fedee6ba29bb164e81cdf324971f9a1 Reviewed-on: https://chromium-review.googlesource.com/623410 Commit-Queue: Dominic Cooney <dominicc@chromium.org> Reviewed-by: Dominic Cooney <dominicc@chromium.org> Cr-Commit-Position: refs/heads/master@{#496224} [add] https://crrev.com/ac6d4866aa926c3edde2154eb7c207a21d19aae9/third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-attributes.html [modify] https://crrev.com/ac6d4866aa926c3edde2154eb7c207a21d19aae9/third_party/WebKit/Source/core/geometry/DOMMatrix.h
,
Aug 22 2017
,
Aug 30 2017
Is this fix going to make its way into Chrome 61 before release? Is there anything I should do to increase the probability?
,
Aug 30 2017
Based on prio and known size of affected it doesn't seem like an obvious candidate for merge (IMHO.) The risk should be low however.
,
Aug 30 2017
That would be a shame as it causes our app to have severe rendering issues that would require a very messy workaround to avoid.
,
Aug 30 2017
So, this is not a Pri-3 issue?
,
Aug 30 2017
#9 - I do not advocate it or anything (and I think the fix, since it is very small, should be merged), but would a small polyfill-like code at the startup of your application not do the trick?
Something like -
(function ()
{
var m = new DOMMatrix;
m.m44 = 1;
if (m.is2D)
{
return;
}
// Work around the wrong is2D.
const originalIs2D = Object.getOwnPropertyDescriptor(DOMMatrix.prototype.is2D);
Object.defineProperty(
DOMMatrix,
"is2D",
{
...originalIs2D,
get()
{
return !originalIs2D.get.call(this);
}
});
)());
,
Aug 30 2017
,
Aug 30 2017
This bug requires manual review: We are only 5 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 30 2017
#11 Interesting idea but I don't see how that can work. is2D is only wrong if you set m33 or m44. With your get() a newly created DOMMatrix would give false for is2D and any DOMMatrix that should be 3d would give true. Replacing the toString function also sounds like a reasonable approach but, since we do need both 2d and 3d to work, I don't think that would be possible via a polyfill approach either (except, maybe, by replacing more/all of DOMMatrix, if possible). The messy workaround I alluded to involves adding a function that, for affected browsers, always creates a "matrix(...)" string from the 2d elements of the matrix even if is2D says false. This could then be called from those places that absolutely require a 2d "matrix(...)" but this will involve checking and possibly tweaking quite a few bits of code.
,
Aug 30 2017
#14 - sorry for the incomplete workaround - I do not know DOMMatrix that well, I was merely suggesting a general way of overriding that function with the right implementation when it is wrong. Replacing DOMMatrix altogether with a full blown polyfill when the feature-without-bug detection fails sounds more appropriate, really. Anyway, we will see what the release owners say, hopefully - they will approve the merge and this will be moot. :)
,
Aug 30 2017
I would argue for merging. Small user group also means low chance of negatively affecting people, and right now we are clearly totally wrong. The patch is extremely local.
,
Aug 30 2017
Approving merge to M61 branch 3163 based on comment #16 and per offline chat with schenney@. Please merge ASAP. Thank you.
,
Aug 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/526c288432fb14dbeb11bb01cb5dce36cbc737b4 commit 526c288432fb14dbeb11bb01cb5dce36cbc737b4 Author: Fredrik Söderqvist <fs@opera.com> Date: Wed Aug 30 20:15:41 2017 Setting DOMMatrix.m33 or m44 to 1 should preserve is2D Per [1], setting a value other than one (1) should clear the is2D flag. We had the logic reversed. [1] https://drafts.fxtf.org/geometry/#dommatrix-attributes TBR=fs@opera.com (cherry picked from commit ac6d4866aa926c3edde2154eb7c207a21d19aae9) Bug: 756789 Change-Id: I414985018282a98bb7edea67fc1fa060bd611ba8 Reviewed-on: https://chromium-review.googlesource.com/623410 Commit-Queue: Dominic Cooney <dominicc@chromium.org> Reviewed-by: Dominic Cooney <dominicc@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#496224} Reviewed-on: https://chromium-review.googlesource.com/644326 Reviewed-by: Fredrik Söderquist <fs@opera.com> Cr-Commit-Position: refs/branch-heads/3163@{#1004} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [add] https://crrev.com/526c288432fb14dbeb11bb01cb5dce36cbc737b4/third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-attributes.html [modify] https://crrev.com/526c288432fb14dbeb11bb01cb5dce36cbc737b4/third_party/WebKit/Source/core/geometry/DOMMatrix.h
,
Aug 31 2017
Thank you, very much appreciated...
,
Aug 31 2017
Tested the issue using #61.0.3163.73 on Mac 10.12.6 and Win 10 as per the steps mentioned in comment #0. Observed now the output is true. Please find the screen cast for the same. Hence adding verified labels. Thanks!!
,
Sep 1 2017
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by gerryile...@gmail.com
, Aug 18 2017