width on <td> should be content-box width not border-box width

RESOLVED FIXED in mozilla1.4alpha

Status

()

Core
Layout: Tables
P1
normal
RESOLVED FIXED
17 years ago
16 years ago

People

(Reporter: karnaze (gone), Assigned: Bernd)

Tracking

({testcase})

Trunk
mozilla1.4alpha
x86
Windows 2000
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

17 years ago
When a width is specified on a <td> it should be the content width. I'm fairly 
sure that it used to work correctly.
(Reporter)

Comment 1

17 years ago
Created attachment 64249 [details]
test case
(Reporter)

Updated

17 years ago
Status: NEW → ASSIGNED
Keywords: testcase
Target Milestone: --- → mozilla0.9.9
(Reporter)

Updated

17 years ago
Target Milestone: mozilla0.9.9 → mozilla1.0
Moving to Moz1.1
Target Milestone: mozilla1.0 → mozilla1.1
(Assignee)

Comment 3

16 years ago
how did I miss this one???
Assignee: karnaze → bernd_mozilla
Status: ASSIGNED → NEW
Priority: -- → P1
Target Milestone: mozilla1.1alpha → mozilla1.4alpha
(Assignee)

Comment 4

16 years ago
Created attachment 116774 [details]
testcase +bc + fixed layout
(Assignee)

Comment 5

16 years ago
Created attachment 116786 [details] [diff] [review]
patch rev0
(Assignee)

Comment 6

16 years ago
Comment on attachment 116786 [details] [diff] [review]
patch rev0

I runned the regression tests, I had only 80 failures, that I inspected
visually. It looked good to me. We are much more compatible with IE and opera
with this patch. But not to much compatible, as one might see from attachment
#116774 [details]
Attachment #116786 - Flags: superreview?(bzbarsky)
Attachment #116786 - Flags: review?(jkeiser)
Comment on attachment 116786 [details] [diff] [review]
patch rev0

+	   colWidths[colX] = nsTableFrame::RoundToPixel(cellWidth / colSpan,
p2t);

Shouldn't that use (float)colSpan just like the chunk below it does?

Other than that, I just wish we had a good way not to repeat this code all
over... Any way we could refactor it?

sr=bzbarsky in any case.... but it would be good to clean this stuff up a bit.
Attachment #116786 - Flags: superreview?(bzbarsky) → superreview+

Comment 8

16 years ago
Comment on attachment 116786 [details] [diff] [review]
patch rev0

I think it'd be worth creating a GetBorderPadding() method now that nearly
every call to GetPadding() adds in the border.	You can do this here or in
another bug, your choice.

Comment 9

16 years ago
Comment on attachment 116786 [details] [diff] [review]
patch rev0

I think it'd be worth creating a GetBorderPadding() method now that nearly
every call to GetPadding() adds in the border.	You can do this here or in
another bug, your choice.
Attachment #116786 - Flags: review?(jkeiser) → review+
(Assignee)

Comment 10

16 years ago
Created attachment 118027 [details] [diff] [review]
revised patch
(Assignee)

Comment 11

16 years ago
Comment on attachment 118027 [details] [diff] [review]
revised patch

John and Boris could you please reevaluate your r/sr. Its the the same patch
only some more code refactoring.
Attachment #118027 - Flags: superreview?(bzbarsky)
Attachment #118027 - Flags: review?(jkeiser)
Comment on attachment 118027 [details] [diff] [review]
revised patch

awesome.  ;)
Attachment #118027 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 118027 [details] [diff] [review]
revised patch

Aye, happy about that I am :)
Attachment #118027 - Flags: review?(jkeiser) → review+
(Assignee)

Comment 14

16 years ago
fix checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.