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

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Description

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: testcase

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: pass/fail testcase

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: pass/fail testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

Probably an inner/outer issue, fwiw.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See bug 200089.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

...and bug 200497.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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...

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See bug 372365

Comment 11

[Damon Sicore (:damons)]

Targeting/blocking M9 per request by dbaron.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

taking

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

I think this is what David had in mind.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For scrollframes, mInnerFrame == mOuterFrame, so the comments hold. Do you want better names for these frames?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm saying that, for scrollframes, maybe they shouldn't be ==.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

None of the properties of the inner "scrolled" frame are relevant to getComputedStyle for the element, as far as I can tell.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.)

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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?

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

dbaron, bz, do you want to comment on the above before I implement it?

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't have a preference either way.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Sounds fine to me, at first blush.  Let's get it into the beta and see what happens?

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I suggest we remove M9 blocking status since we don't have a clue what the right behaviour is here.

Comment 29

[Damon Sicore (:damons)]

Anyone object to removing M9 blocking status?

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll take silence as consent.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 282668 [details] [diff] [review]
fix v2

r+sr=dbaron

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So should this be landed?  Or do you think doing nothing is better than taking this?

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Anne (:annevk)]

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.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in.