Last Comment Bug 653930 - Setting marginWidth and marginHeight on an iframe should trigger a reflow
: Setting marginWidth and marginHeight on an iframe should trigger a reflow
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: HTML Frames (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla6
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-30 10:18 PDT by Bobby Holley (busy)
Modified: 2011-05-14 17:33 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Bug 653930 - Tests. v1 (2.03 KB, patch)
2011-05-05 21:25 PDT, Bobby Holley (busy)
bzbarsky: review+
Details | Diff | Review
Bug 653930 - v1 (3.47 KB, patch)
2011-05-05 21:26 PDT, Bobby Holley (busy)
bzbarsky: review+
Details | Diff | Review
Bug 653930 - v2 (3.42 KB, patch)
2011-05-06 10:57 PDT, Bobby Holley (busy)
bobbyholley: review+
Details | Diff | Review

Description Bobby Holley (busy) 2011-04-30 10:18:48 PDT
Code that modifies these properties dynamically need to do a little song and dance to get the properties to take effect: http://help.dottoro.com/ljkpxqht.php
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-30 11:01:11 PDT
The simplest solution here is to implement nsSubDocumentFrame::AttributeChanged (since nsSubDocumentFrame::GetMarginAttributes is where the attributes are handled).

Probably both of them should also check that the element in question is an HTML iframe element, though (or perhaps also xul iframe element, if needed).
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-03 21:42:33 PDT
Bobby, you want to work on this?  If not, I'll steal.
Comment 3 Bobby Holley (busy) 2011-05-03 22:39:27 PDT
(In reply to comment #2)
> Bobby, you want to work on this?  If not, I'll steal.

I was hoping to get around to it when midterm craziness dies down, but we'll see. So I'll take it for now, and also take the responsibility for pinging you back in a week or two if I decide not to.

If there are release timetable reasons why this should happen sooner let me know. ;-)
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-04 18:44:19 PDT
There is no great rush here.  It's not like it ever worked....
Comment 5 Bobby Holley (busy) 2011-05-05 00:01:29 PDT
I've got the new marginWidth and marginHeight onto nsFrameLoader::mDocShell, but I'm having trouble getting it to take effect. What should I be doing to trigger the restyle here? nsIPresShell::StyleChangeReflow() doesn't seem to be cutting it.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-05 06:51:36 PDT
You probably want to call RebuildAllStyleData() on the prescontext (and not do any manual reflowing; the style data rebuild should do the trick).
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-05 07:35:15 PDT
I don't think that'll work, since I think the marginwidth and marginheight attributes never actually end up in style data.

(Maybe they should, though, but we'd need to figure out as what...)
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-05 07:52:39 PDT
> since I think the marginwidth and marginheight attributes never actually end up
> in style data

They do.  See BodyRule::MapRuleInfoInto where it grabs those from the docshell.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-05 08:01:08 PDT
Should we consider making BodyRule follow the nsIStyleRule contract correctly?
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-05 08:07:36 PDT
Sure.  I don't think we should make Bobby do that, though....
Comment 11 Bobby Holley (busy) 2011-05-05 21:25:27 PDT
Created attachment 530533 [details] [diff] [review]
Bug 653930 - Tests. v1
Comment 12 Bobby Holley (busy) 2011-05-05 21:26:42 PDT
Created attachment 530534 [details] [diff] [review]
Bug 653930 - v1

Adding tests and fix. Flagging bz for review.
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-06 09:42:03 PDT
Comment on attachment 530534 [details] [diff] [review]
Bug 653930 - v1

Just nix the comment in AttributeChanged and don't leave a blank line before the else, please.  r=me with that.

We should clean up the FrameLoader() thing; I'm not sure it can actually be null here.  But good enough for now.
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-06 09:45:03 PDT
Comment on attachment 530533 [details] [diff] [review]
Bug 653930 - Tests. v1

r=me
Comment 15 Bobby Holley (busy) 2011-05-06 10:57:58 PDT
Created attachment 530667 [details] [diff] [review]
Bug 653930 - v2

Addressed bz's comments. Carrying over review.

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