Last Comment Bug 816458 - Removing an inline css transform on a table applies twice the resulting computed transform
: Removing an inline css transform on a table applies twice the resulting compu...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla20
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
: Cornel Ionce [QA] (:cornel_ionce)
Mentors:
Depends on:
Blocks: 691651
  Show dependency treegraph
 
Reported: 2012-11-29 02:52 PST by Daniel Glazman (:glazou)
Modified: 2013-02-13 07:44 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
+
verified
+
fixed


Attachments
test case showing issue (2.85 KB, text/html)
2012-11-29 02:52 PST, Daniel Glazman (:glazou)
no flags Details
fix (9.46 KB, patch)
2012-12-02 21:07 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
fix v2 (11.15 KB, patch)
2012-12-03 20:50 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Daniel Glazman (:glazou) 2012-11-29 02:52:06 PST
Created attachment 686503 [details]
test case showing issue

A HTML table is rotated through 'transform: rotate(22deg)'. A script then
applies the inline style 'transform: none', flushes the layout changes
and removes the inline style. See attached test case.

Steps to reproduce
  1. load attached test case
  2. click on first link, inline 'transform: none' is applied
  3. click on second link, the above is removed

Expected result: table is rotated 22 degrees.
Actual result  : table is rotated 44 degrees...

As far as I can tell, this happens only for tables.
Comment 1 Boris Zbarsky [:bz] 2012-11-29 08:41:34 PST
My money is on us managing to apply the transform on both the inner and outer table somehow...
Comment 2 aveclafaux 2012-11-29 09:56:22 PST
Either I don't get it or it works for me on 16.0.1 - after the second click the table is in the same position than the original (and reference) position.
Comment 3 aveclafaux 2012-11-29 09:58:57 PST
(and it is the same WFM on 17.0 - I had a doubt and upgraded just now. I'm on Windows 7)
Comment 4 Boris Zbarsky [:bz] 2012-11-29 10:00:08 PST
Yep.  It works in 16 and 17, but not a nightly.

(On another note, if you're really using 16.0.1 you need to update ASAP: you're two security updates behind.)

I'll bet money this regressed with bug 691651.
Comment 5 Boris Zbarsky [:bz] 2012-11-29 10:02:00 PST
Yeah, regression range on nightlies includes bug 691651.
Comment 6 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2012-11-29 10:19:13 PST
The ::-moz-table-outer { -moz-transform: inherit } rule in ua.css is almost certainly related.

I checked that nsChangeHint_AddOrRemoveTransform was added to nsChangeHint_Hints_NotHandledForDescendants and NS_HintsNotHandledForDescendantsIn correctly; it looks to me like it was.

So I don't see anything immediately.
Comment 7 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2012-11-29 10:21:03 PST
I do wonder what makes us not apply the transform to the inner table.  (Or perhaps what used to but doesn't anymore.)
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-12-02 20:14:54 PST
(In reply to David Baron [:dbaron] from comment #7)
> I do wonder what makes us not apply the transform to the inner table.  (Or
> perhaps what used to but doesn't anymore.)

This, in nsTableFrame.cpp:
  // Transforms need to affect the outer frame, not the inner frame (bug 722777)
  mState &= ~NS_FRAME_MAY_BE_TRANSFORMED;
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-12-02 21:07:16 PST
Created attachment 687622 [details] [diff] [review]
fix

Instead of removing the MAY_BE_TRANSFORMED bit in certain frame classes, add an nsIFrame::SupportsCSSTransforms method and call it when necessary.
Comment 10 Mats Palmgren (vacation) 2012-12-03 06:24:16 PST
Comment on attachment 687622 [details] [diff] [review]
fix

Since we have IsFrameOfType for this sort of thing - why not IsFrameOfType(eSupportsCSSTransforms) instead of SupportsCSSTransforms() ?

This comment needs updating?
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7791
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-12-03 20:50:10 PST
Created attachment 688097 [details] [diff] [review]
fix v2
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-12-07 03:09:13 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd4859261347
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-12-09 21:14:36 PST
Comment on attachment 688097 [details] [diff] [review]
fix v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 691651
User impact if declined: Possibly broken rendering on a small number of pages
Testing completed (on m-c, etc.): just landed on m-c
Risk to taking this patch (and alternatives if risky): Seems relatively low-risk for Aurora
String or UUID changes made by this patch: None
Comment 15 Lukas Blakk [:lsblakk] use ?needinfo 2012-12-10 15:38:46 PST
Comment on attachment 688097 [details] [diff] [review]
fix v2

agree that this is low-risk enough for aurora, approving.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-12-10 18:54:19 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d1c8ffeeeaa
Comment 17 Cornel Ionce [QA] (:cornel_ionce) 2013-02-13 07:44:09 PST
Verified fixed on Firefox 19 beta 6
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130212082553

Note You need to log in before you can comment on or make changes to this bug.