404309 - [FIX]Colgroup span changes have no effect

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Perhaps like so

Comment 2

[Boris Zbarsky [:bzbarsky]]

That patch depends on the patch for bug 403249.

Comment 3

[Bernd]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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

[Bernd]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Bernd]

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

the content stuff is up to David

Comment 8

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

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

Comment 9

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

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

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

This is very safe.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 303404 [details] [diff] [review]
Updated to comments

a=beltzner for 1.9

Comment 12

[Boris Zbarsky [:bzbarsky]]

Checked in.