New issue
Advanced search Search tips

Issue 756789 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows , Mac
Pri: 2
Type: Bug
Team-Blink-Paint



Sign in to add a comment

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.
 
In case this isn't obvious from the last sentence, setting m33 or m44 to 0 or -0 does not result in is2D being set to false but it should.

Comment 2 by f...@opera.com, Aug 21 2017

Components: Blink>Geometry
Labels: Hotlist-Interop OS-Mac OS-Windows
Status: Untriaged (was: Unconfirmed)
I tested Firefox and it indeed outputs true, while Chrome outputs false.

Comment 4 by f...@opera.com, Aug 21 2017

Owner: f...@opera.com
Status: Started (was: Untriaged)
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Comment 6 by f...@opera.com, Aug 22 2017

Status: Fixed (was: Started)
Is this fix going to make its way into Chrome 61 before release?  Is there anything I should do to increase the probability?

Comment 8 by f...@opera.com, 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.
That would be a shame as it causes our app to have severe rendering issues that would require a very messy workaround to avoid.

Comment 10 by f...@opera.com, Aug 30 2017

So, this is not a Pri-3 issue?

Comment 11 by phistuck@gmail.com, 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);
    }
   });
 )());

Comment 12 by f...@opera.com, Aug 30 2017

Labels: -Pri-3 Merge-Request-61 Pri-2
Project Member

Comment 13 by sheriffbot@chromium.org, Aug 30 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
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
#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.

Comment 15 by phistuck@gmail.com, 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. :)
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.
Labels: -Merge-Review-61 Merge-Approved-61
Approving merge to M61 branch 3163 based on comment #16 and per offline chat with  schenney@. Please merge ASAP. Thank you.
Project Member

Comment 18 by bugdroid1@chromium.org, Aug 30 2017

Labels: -merge-approved-61 merge-merged-3163
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

Thank you, very much appreciated...
Labels: TE-Verified-M61 TE-Verified-61.0.3163.73
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!!
Aug 31 2017 3_41 PM.webm
2.2 MB View Download

Sign in to add a comment