Closed Bug 363183 Opened 14 years ago Closed 13 years ago
Computed Style is broken for tables (returns computed style of previous element?)
I discovered this while trying to reduce the testcase for bug 363146. See bug 363146 comment 6. It seems as though getting the computed style for the a table gets the computed style of the previous node in the document.
Probably an inner/outer issue, fwiw.
I think there's a longstanding bug that it uses the outer frame when it should use the inner frame; the reflow branch just made the outer frame correlate a bit less with what you expect -- in particular, the width of the outer frame is now generally the width of the container rather than the width of the table.
See bug 200089.
...and bug 200497.
So.... The problem as I recall it is that the inner table frame for width/height doesn't give quite what people want (e.g. doesn't include the caption). Just using the inner table throughout in computed style would in fact be really easy...
I think the best solution is to use the inner table frame throughout, except computing margins / positioning offsets including the offsets of both inner and outer table.
See bug 372365
Targeting/blocking M9 per request by dbaron.
Target Milestone: --- → mozilla1.9 M9
This bug needs an assignee. I have no other M9 blockers that lack patches, except for a Talos space regression. I'll take this tomorrow if no-one else volunteers.
Assignee: nobody → roc
I think this is what David had in mind.
The comments make it sound awfully general -- for scroll frames, wouldn't we want the border from the outer frame rather than the inner? Should we fix this for scroll frames too?
For scrollframes, mInnerFrame == mOuterFrame, so the comments hold. Do you want better names for these frames?
I'm saying that, for scrollframes, maybe they shouldn't be ==.
None of the properties of the inner "scrolled" frame are relevant to getComputedStyle for the element, as far as I can tell.
I added a mochitest. And of course while writing the test, I found a bug ... the margins must be taken from the inner frame. It's a bit confusing that with this approach, (top,left,width,height) is not a rectangle for any actual box in the layout. Since top and left captions are pretty rare, perhaps we should just get top/left from the inner frame as well? (In that case we might be able to get rid of mOuterFrame altogether, which would simplify things.)
(In reply to comment #18) > None of the properties of the inner "scrolled" frame are relevant to > getComputedStyle for the element, as far as I can tell. Padding is -- the inner has 'padding: inherit' and the outer ignores it -- although since it looks like we still return the bogus GetUsedPadding it's actually ok for now (I didn't test, though). (In reply to comment #19) > It's a bit confusing that with this approach, (top,left,width,height) is not a > rectangle for any actual box in the layout. Since top and left captions are > pretty rare, perhaps we should just get top/left from the inner frame as well? > (In that case we might be able to get rid of mOuterFrame altogether, which > would simplify things.) Getting top/left from the inner frame would probably be reasonable, as long as the coordinates are relative to the outer frame's parent. Any idea what other browsers do?
> Getting top/left from the inner frame would probably be reasonable, as long as > the coordinates are relative to the outer frame's parent. I'll do that. > Any idea what other browsers do? I'll check.
> I'll do that. Actually I changed my mind back again. Yes, (top,left,width.height) is not a box in the layout, but if you have an abs-pos table with a top caption and you specify top, left, width and height, we position the table at (top,left) and give the inner table (width,height) ... so returning those values for the computed style is the right thing to do. So I think we should go with the current patch. I'll check what some other browsers do but I don't think that will change my opinion --- consistency between the meaning of style rules and computed style is most important. So please carry on with the existing patch.
Safari 3 returns the height of the outer table in my testcase. So maybe we should union the inner table with the caption and use that, relative to the outer frame's container?
dbaron, bz, do you want to comment on the above before I implement it?
I don't have a preference either way.
Sounds fine to me, at first blush. Let's get it into the beta and see what happens?
So ... Safari 3's behaviour has a problem too :-(. Testcase: <table id="t1" border="0" cellspacing="0" cellpadding="0" style="border:14px solid black; margin:20px; position:absolute; left:50px; top:35px;"> <caption align="top" style="height:101px;">Caption</caption> <tr><td><div style="width:400px; height:102px;">Cell</div></td></tr> </table> <script> var t1 = document.getElementById("t1"); var cs = window.getComputedStyle(document.getElementById("t1"), ""); alert(cs.width + "," + cs.height); </script> I get "400px,203px", so it's just adding together the caption height and the table body height. Which is kinda reasonable since we want the content height, but it's also unreasonable because there's a border between the caption and the body so again we get a measurement that doesn't correspond to the dimensions of anything on the page. Maybe Anne can break the deadlock ... any thoughts?
I suggest we remove M9 blocking status since we don't have a clue what the right behaviour is here.
13 years ago
Whiteboard: [needs feedback]
Anyone object to removing M9 blocking status?
I'll take silence as consent.
Target Milestone: mozilla1.9 M9 → ---
Whiteboard: [needs feedback] → [needs feedback][dbaron-1.9:RwCr]
Comment on attachment 282668 [details] [diff] [review] fix v2 r+sr=dbaron
13 years ago
Priority: -- → P4
So should this be landed? Or do you think doing nothing is better than taking this?
I think this is really a standards issue and should be resolved that way. I'd at least like to get Anne's input. I'll email directly.
I haven't looked that much at getComputedStyle yet I'm afraid. I would like it to simply map to the CSS model described by the CSS specification, but I guess it's a bit more tricky than that.
I think we should go with comment #22 after all. My concerns in comment #19 aren't well-founded ... the rectangle formed from the left, top, width and height values of getComputedStyle does not represent any rectangle in the layout, in general, because left and top are the top-left corner of the margin-box for absolutely positioned elements, and width and height are for the content-box. So I'll just land the patch that's already reviewed.
Whiteboard: [needs feedback][dbaron-1.9:RwCr] → [dbaron-1.9:RwCr][needs landing]
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [dbaron-1.9:RwCr][needs landing] → [dbaron-1.9:RwCr]
You need to log in before you can comment on or make changes to this bug.