getComputedStyle is broken for tables (returns computed style of previous element?)

RESOLVED FIXED

Status

()

Core
Layout
P4
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Gavin, Assigned: roc)

Tracking

({regression, testcase})

Trunk
regression, testcase
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dbaron-1.9:RwCr])

Attachments

(3 attachments, 2 obsolete attachments)

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.
Created attachment 247994 [details]
pass/fail testcase
Created attachment 247995 [details]
pass/fail testcase
Attachment #247994 - Attachment is obsolete: true
Keywords: testcase
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.

Updated

11 years ago
OS: Windows XP → All
Hardware: PC → All
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.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
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.
taking
Assignee: nobody → roc
Created attachment 282651 [details] [diff] [review]
fix

I think this is what David had in mind.
Attachment #282651 - Flags: superreview?(dbaron)
Attachment #282651 - Flags: review?(dbaron)
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.
Created attachment 282668 [details] [diff] [review]
fix v2

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.)
Attachment #282651 - Attachment is obsolete: true
Attachment #282668 - Flags: superreview?(dbaron)
Attachment #282668 - Flags: review?(dbaron)
Attachment #282651 - Flags: superreview?(dbaron)
Attachment #282651 - Flags: review?(dbaron)
(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.
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
Attachment #282668 - Flags: superreview?(dbaron)
Attachment #282668 - Flags: superreview+
Attachment #282668 - Flags: review?(dbaron)
Attachment #282668 - Flags: review+
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.

Comment 34

10 years ago
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]
Checked in.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
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.