Bug 83088
Summary: | TransformationMatrix::hasPerspective() should consider the entire bottom row of the matrix | ||
---|---|---|---|
Product: | WebKit | Reporter: | Shawn Singh <shawnsingh> |
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED WONTFIX | ||
Severity: | Normal | CC: | cmarrin, enne, reveman, simon.fraser, vangelis |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | Unspecified | ||
OS: | Unspecified |
Shawn Singh
TransformationMatrix::hasPerspective() checks only the 4th row, 3rd column to see that it is non-zero. I think it is more correct to consider the entire bottom row of the matrix when there is a perspective projection.
Take the following example:
TransformationMatrix A;
A.makeIdentity()
A.applyPerspective(1);
A.translate(50, 0);
A.rotate3d(0, 90, 0);
The resulting matrix does have perspective, but the 4th row, 3rd colum element is zero. Specifically that matrix has a bottom row of (1, 0, 0, 1). Technically, its -0.0 on the element, and hasPerspective() is likely to work, but it is probably not a good idea to rely on IEEE floating-point mechanisms to get this correct by luck.
Should we be testing the entire bottom row of elements, instead? i.e. anything that may cause w != 1 should be considered a perspective projection.
Please let me know if you all agree, and I'll submit a quick two-line patch to fix this.
Attachments | ||
---|---|---|
Add attachment proposed patch, testcase, etc. |
Simon Fraser (smfr)
When does this matter?
Shawn Singh
I have not seen any issue with this in real-world bugs. But given that css will allow the web developer to enter any arbitrary matrix for an element, it seems appropriate that we should get this correct.
Simon Fraser (smfr)
Perhaps we should rename hasPerspective() to something more general if we change this.
Shawn Singh
Sure I could do that. But in my opinion hasPerspective is still a very appropriate name. What would you suggest instead?
other options are: hasSkewAlongW() ... willCausePerspectiveDivision() ... isBottomRowModified()
Shawn Singh
Also, I'll try to make a layout test that fails with existing code. If I can't then this patch probably isn't worth doing.
Shawn Singh
This bug has been sitting here for a while; I think it's WONTFIX for WebKit. Thanks!