[FIX]Colgroup span changes have no effect

RESOLVED FIXED

Status

()

Core
Layout: Tables
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
x86
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

For some reason, we only treat "span" as mapped into style for cols.  We should be doing the same thing for colgroups, no?

Patch and tests coming up, but maybe there's a reason we do it this way?
Created attachment 289280 [details] [diff] [review]
Perhaps like so
Attachment #289280 - Flags: superreview?(dbaron)
Attachment #289280 - Flags: review?(bernd_mozilla)
That patch depends on the patch for bug 403249.

Comment 3

10 years ago
Is that a clean patch against trunk or is it bitrotted by bug 403249?

I don't see a reason why should not map.
We had before a seperate nsHTMLTableColGroupElement.cpp (http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla%2Fcontent%2Fhtml%2Fcontent%2Fsrc%2FAttic%2FnsHTMLTableColGroupElement.cpp&rev=&cvsroot=%2Fcvsroot)
which did not map, but probably at this time colgroups have been only a step child.

> Is that a clean patch against trunk or is it bitrotted by bug 403249?

It's a clean patch against trunk after bug 403249 landed.

Mapping definitely makes the most sense to me if we support span on colgroups.

Comment 5

10 years ago
Comment on attachment 289280 [details] [diff] [review]
Perhaps like so

I was looking at https://bugzilla.mozilla.org/attachment.cgi?id=289280&action=diff#mozilla/content/html/content/src/nsHTMLTableColElement.cpp_sec2
vs 

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/html/content/src&command=DIFF_FRAMESET&file=nsHTMLTableColElement.cpp&rev2=1.86&rev1=1.85

Anyway as I do not understand what the benefit is from a  change from ColMapAttributesIntoRule to MapAttributesIntoRule I am probably the wrong person to review this.
Attachment #289280 - Flags: review?(bernd_mozilla)
Comment on attachment 289280 [details] [diff] [review]
Perhaps like so

> I was looking at

Bugzilla's diff is just confused, as far as I can tell.

The benefit of this change is that it'll make changes to "span" on colgroups reframe them.  That was the point of the patch this bug depends on.  David will review that part just fine, I would assume; I'd like you to review the change to using the mapped attr value in the frame code and the general idea that the span attribute should be treated basically the same way for columns and colgroups on the content side.
Attachment #289280 - Flags: review?(bernd_mozilla)

Comment 7

10 years ago
Comment on attachment 289280 [details] [diff] [review]
Perhaps like so

the content stuff is up to David
Attachment #289280 - Flags: review?(bernd_mozilla) → review+
Comment on attachment 289280 [details] [diff] [review]
Perhaps like so

>+      PRInt32 val = value->GetIntegerValue();
>+      if (val >= 1) {

I think you shouldn't add this test.  It doesn't break anything now (but does it add anything?).  However, if we add a column-span property (likely in css3) it would.

>   static const MappedAttributeEntry* const col_map[] = {

Could you rename this to |map| to match most of the other implementations?

sr=dbaron with that; sorry for the delay
Attachment #289280 - Flags: superreview?(dbaron) → superreview+
(In reply to comment #8)
> (From update of attachment 289280 [details] [diff] [review])
> >+      PRInt32 val = value->GetIntegerValue();
> >+      if (val >= 1) {
> 
> I think you shouldn't add this test.  It doesn't break anything now (but does
> it add anything?).  However, if we add a column-span property (likely in css3)
> it would.

Er, sorry, this is fine, as long as you add a warning not to copy that test to the handling of colspan on cells.  (And given that the spec says > 0, maybe that's better.)
Created attachment 303404 [details] [diff] [review]
Updated to comments

This fixes some code to make sure we don't get confused when "span" changes.

This is very safe.
Attachment #289280 - Attachment is obsolete: true
Attachment #303404 - Flags: approval1.9?
Comment on attachment 303404 [details] [diff] [review]
Updated to comments

a=beltzner for 1.9
Attachment #303404 - Flags: approval1.9? → approval1.9+
Checked in.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.