Last Comment Bug 43178 - Table's "rules" should be implemented with CSS equivalent
: Table's "rules" should be implemented with CSS equivalent
Status: RESOLVED FIXED
[awd:tbl]
: testcase
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: P3 normal with 6 votes (vote)
: mozilla1.9.3a1
Assigned To: fantasai
:
Mentors:
: 472327 (view as bug list)
Depends on: 573864 620648 646722 665918 2436 41262 539880 540228 540360 573960 577654 648531
Blocks: 21076 25228 41411 114100 144899 155507 167496 229591 276244 320979 484825 1045161
  Show dependency treegraph
 
Reported: 2000-06-20 11:15 PDT by fantasai
Modified: 2014-07-28 11:49 PDT (History)
33 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Attempt to make rules=groups work (4.72 KB, text/html)
2004-02-09 00:01 PST, Boris Zbarsky [:bz] (TPAC)
no flags Details
Slightly updated to handle interaction of rules and border better. (4.92 KB, text/html)
2004-02-09 00:45 PST, Boris Zbarsky [:bz] (TPAC)
no flags Details
no outer borders on rules=all (4.96 KB, text/html)
2004-02-09 00:53 PST, Boris Zbarsky [:bz] (TPAC)
no flags Details
Fix CSS error caught by that testcase (4.95 KB, text/html)
2004-02-09 12:26 PST, Boris Zbarsky [:bz] (TPAC)
no flags Details
first scetch (21.81 KB, patch)
2004-07-19 11:09 PDT, Bernd
no flags Details | Diff | Splinter Review
first scetch updated to tip (21.71 KB, patch)
2004-10-12 03:16 PDT, Bernd
no flags Details | Diff | Splinter Review
first scetch updated to tip again (19.49 KB, patch)
2006-09-02 08:16 PDT, Bernd
no flags Details | Diff | Splinter Review
patch with fix from bug 272341 (26.07 KB, patch)
2006-09-03 08:35 PDT, Bernd
no flags Details | Diff | Splinter Review
New tablerules.css (draft) (4.43 KB, text/css)
2006-09-14 02:53 PDT, fantasai
no flags Details
New tablerules.css (5.46 KB, text/css)
2006-10-16 23:39 PDT, fantasai
no flags Details
updated patch (31.58 KB, patch)
2006-10-16 23:52 PDT, fantasai
no flags Details | Diff | Splinter Review
updated patch (31.59 KB, patch)
2006-10-17 00:05 PDT, fantasai
no flags Details | Diff | Splinter Review
updated patch (26.35 KB, patch)
2007-07-18 00:04 PDT, fantasai
no flags Details | Diff | Splinter Review
Color Weirdness Testcase (928 bytes, text/html)
2007-07-18 00:09 PDT, fantasai
no flags Details
next rev. (77.05 KB, patch)
2008-05-17 04:20 PDT, Bernd
no flags Details | Diff | Splinter Review
updated table rules (5.58 KB, text/css)
2008-05-17 04:21 PDT, Bernd
no flags Details
reftests archive (55.50 KB, application/x-zip-compressed)
2008-05-17 04:26 PDT, Bernd
no flags Details
revised to keep quirks mode gray (5.72 KB, text/css)
2008-05-17 07:54 PDT, Bernd
no flags Details
patch updated to tip (80.57 KB, patch)
2008-08-02 11:39 PDT, Bernd
no flags Details | Diff | Splinter Review
revised patch (79.24 KB, patch)
2008-08-16 23:29 PDT, Bernd
no flags Details | Diff | Splinter Review
slightly updated (79.43 KB, patch)
2008-09-12 23:58 PDT, Bernd
no flags Details | Diff | Splinter Review
patch (82.91 KB, patch)
2009-03-07 22:34 PST, Bernd
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Splinter Review
updated to tip (63.31 KB, patch)
2009-11-09 22:42 PST, Bernd
no flags Details | Diff | Splinter Review
revised patch (118.14 KB, patch)
2010-01-03 22:21 PST, Bernd
bzbarsky: review+
Details | Diff | Splinter Review
Interdiff in question (57.21 KB, patch)
2010-01-06 11:35 PST, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review
test cases for the dependent bugs that got fixed (6.62 KB, patch)
2010-01-10 08:25 PST, Bernd
no flags Details | Diff | Splinter Review
bundle for checkin (923.39 KB, application/octet-stream)
2010-01-10 12:49 PST, Bernd
no flags Details

Description fantasai 2000-06-20 11:15:13 PDT
Overview:

 Currently, "rules" is hard-coded into Mozilla (see bug 41411), which
 places restrictions on CSS properties. "rules" should be implemented
 with style sheets or their equivalent and participate in the cascade.

Some problems that can be solved by this:

 Bug 21076 - rules don't display without border attribute
 Bug 41411 - rules attribute overrides CSS
 Bug 25228 - border=0 overrides CSS

Other bugs that will be affected:

 Bug 2068 - HTML4 example table not rendered correctly
 Bug 24113 - rules=groups does not function properly

What this requires:

 - the CSS2 collapsing border model in working order (bug 41262)

 - HTML4 row & column groups with border capabilities (bug 2436, bug 915)

 - Table's "frame" attribute implemented with a style-sheet
   equivalent (already done) and *all non-visible frame borders mapped
   to '0 hidden', not 'none'.* (see "emulating rules with CSS" - bug 41411)
       http://bugzilla.mozilla.org/showattachment.cgi?attach_id=10217
        (view source - comments in stylesheet)
Comment 1 Bernd Mielke 2000-06-22 05:20:45 PDT
Confirming the bug. fantasai@escape.com you should ask for some bugzilla 
permissions.
Comment 2 karnaze (gone) 2001-03-16 16:23:49 PST
Moving to m1.0
Comment 3 Amarendra Hanumanula 2001-03-22 13:20:50 PST
QA contact update
Comment 4 karnaze (gone) 2002-01-15 09:20:13 PST
->m099
Comment 5 Kevin McCluskey (gone) 2002-02-19 16:13:24 PST
Marking nsbeta1+
Comment 6 karnaze (gone) 2002-02-19 19:06:45 PST
Fixed by meta bug 41262.
Comment 7 Amarendra Hanumanula 2002-03-13 18:28:15 PST
 Rules with style sheets are fixed and checked in. 

Tested with Build ID: 2002031303

Comment 8 fantasai 2004-02-08 22:44:24 PST
Reopening, because it really never did get fixed.
Comment 9 Bernd 2004-02-08 22:46:30 PST
if one could make the colgroup case in
http://bugzilla.mozilla.org/attachment.cgi?id=10217&action=view make working I
think thats definetely a thing we want.
Comment 10 Boris Zbarsky [:bz] (TPAC) 2004-02-09 00:01:07 PST
Created attachment 140924 [details]
Attempt to make rules=groups work
Comment 11 Boris Zbarsky [:bz] (TPAC) 2004-02-09 00:45:16 PST
Created attachment 140929 [details]
Slightly updated to handle interaction of rules and border better.
Comment 12 Boris Zbarsky [:bz] (TPAC) 2004-02-09 00:53:46 PST
Created attachment 140930 [details]
no outer borders on rules=all
Comment 13 Bernd 2004-02-09 11:29:01 PST
there is a quite complete testcase at
http://bugzilla.mozilla.org/attachment.cgi?id=41726&action=view
Comment 14 Boris Zbarsky [:bz] (TPAC) 2004-02-09 12:26:43 PST
Created attachment 140979 [details]
Fix CSS error caught by that testcase

With these styles (well, I renamed "frame" to" myframe" to test), I get
identical rendering on that exhaustive testcase (I was comparing these style
rules to what mozilla does right now).
Comment 15 Bernd 2004-07-19 11:09:44 PDT
Created attachment 153685 [details] [diff] [review]
first scetch
Comment 16 Boris Zbarsky [:bz] (TPAC) 2004-07-19 11:34:45 PDT
Comment on attachment 153685 [details] [diff] [review]
first scetch 

>Index: layout/html/document/src/Makefile.in
> 	quirk.css \
>+        tablerules.css \
> 	viewsource.css \

Fix indent?

>Index: layout/html/document/src/html.css

Random whitespace changes in here...

>Index: layout/html/document/src/tablerules.css

This needs an @namespace rule.

Can we remove the code that maps "rules" in MapAttributesIntoRule in
nsHTMLTableElement.cpp?  Or is that still used somewhere?
Comment 17 fantasai 2004-07-20 14:31:27 PDT
Just comments on the CSS for now:

- The rules setting border-collapse: collapse; on table should be removed; this
  is covered in html.css already. (Do not move this rule from html.css)

- All rules dealing with td are missing equivalent selectors for th

- The rules setting border: 0 hidden; on the table should be collapsed into just
    table[rules] {
      border-style: hidden;
    }

- Change the table[border] td rule to
    table[border] > tr > td,
    table[border] > tr > th,
    table[border] > * > tr > td,
    table[border] > * > tr > th {
      border: thin inset;
    }
  and shift it up before the [rules] rules.

- Add a rule to handle [border="0"] : 
    table[border="0"],
    table[border="0"] > tr > td,
    table[border="0"] > tr > th,
    table[border="0"] > * > tr > td,
    table[border="0"] > * > tr > th {
      border-style: none;
    }
  The attribute-mapping code should now only be setting border-width and no
  other property.

- The [rules] rules should all be given with the border shorthand.
      border: none solid;
  or  border: solid none;

- You forgot thead in the rules=groups rule.

- Rename tablerules.css to preshint.css. bz, we need to load this into the
  preshint level. How do we do that?
Comment 18 Boris Zbarsky [:bz] (TPAC) 2004-07-20 14:49:07 PDT
>- Rename tablerules.css to preshint.css. bz, we need to load this into the
>  preshint level. How do we do that?

Why is it any more preshint than most of ua.css?

There are no reasonable APIs to load things into the preshint level at the
moment; further, there are asserts that the preshint level contains no CSS rules.

Those could be removed, of course.... and said APIs could be added.
Comment 19 fantasai 2004-07-20 14:57:39 PDT
It's preshint because that's the level it belongs in, and was in (more or less),
and therefore should be in.
And CSS2.1 says so, so no arguing about it! :)

http://www.w3.org/TR/CSS21/cascade.html#q13

> Those could be removed, of course.... and said APIs could be added.

That should be a separate bug. I can file it tomorrow.
Comment 20 fantasai 2004-07-21 14:58:07 PDT
filed bug 252530 for adding preshint API
No dependency, because we can just @import the table rules sheet into ua.css for
now.
Comment 21 Bernd 2004-10-12 03:16:21 PDT
Created attachment 161847 [details] [diff] [review]
first scetch updated to tip
Comment 22 Bernd 2004-11-29 21:42:50 PST
from bug 272341

We propagate table's 'frame' and 'rules' attributes through the style system,
but nobody gets them out the other end.  There's no need for this.

maybee the removal will help for the patch which spoiled up whith the css.
Comment 23 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-11-29 21:51:23 PST
Bug 272341 contains a patch showing more code that could be removed (but skip
the addition of GetAttributeChangeHint that it contains).
Comment 24 Bernd 2006-09-02 08:16:55 PDT
Created attachment 236538 [details] [diff] [review]
first scetch updated to tip again
Comment 25 Bernd 2006-09-02 08:26:34 PDT
I don't get the style rules matching on the td. My guess is that this is a consequence of bug 211636. My knowledge of the style system did not increase over the past years (bug 87663). I simply have no idea what needs to be done here. Giving the bug back.
Comment 26 Bernd 2006-09-03 08:35:09 PDT
Created attachment 236617 [details] [diff] [review]
patch with fix from bug 272341

This patch does include the fix from bug 272341. However the selector do again not work for rows and cell. And I have neither an idea why this is so nor how to debug such a thing.
:-(
Comment 27 Bernd 2006-09-07 13:42:46 PDT
the selector cannot match if the CSS comment closure looks like:
***   /
 Thanks to Boris for spotting it.
Comment 28 fantasai 2006-09-14 02:53:10 PDT
Created attachment 238398 [details]
New tablerules.css (draft)

Sorry for disappearing on you, Bernd. I'll be around more often now. :)

I reorganized the rules in tablerules.css to address some problems with the interaction of 'border' and 'frame'. Note that border-collapsing and cascading are independent. :) Also added rules to match <th> elements and fixed a couple other things iirc. With this style sheet the only attribute mapping that is necessary is mapping the value of 'border' to 'border-width'.

I haven't tested the code yet; I'm going to make some test cases next, then
attach an updated version.

There's a couple open interpretation/compatibility issues:
  1. Border collapsing for rules="" and rules="none"
       - The current code collapses for rules="", but not for rules="none"
            - It also does stupid things like assume rules="" means rules="all"
              when the 'frame' attribute is present.
       - The proposed code collapses for neither rules="" and rules="none"
       - Opera collapses for rules="none" but not for rules=""; it basically
         ignores rules=""
  2. Border style for table
       - The current code uses the outset style for the table borders.
       - The proposed code uses outset for 'border' borders and solid for
         'frame' borders and when both are sest.
       - Opera uses outset whenever 'border' is present, but solid if only
         'frame' is there.

The spec doesn't say what to do here. I suggest we copy what Opera is doing,
because it makes sense and will make at least the two of us compatible. (Safari
doesn't support 'frame' or 'rules', and IE doesn't do border-collapsing for
them.) If that's ok, I'll make those changes in the next revision.
Comment 29 Bernd 2006-09-14 14:15:31 PDT
I usually take https://bugzilla.mozilla.org/attachment.cgi?id=41726&action=view for testing
Comment 30 fantasai 2006-10-12 15:38:01 PDT
Extended test suite:
http://fantasai.inkedblade.net/markup/tests/table-frame-rules/
Comment 31 fantasai 2006-10-16 23:39:33 PDT
Created attachment 242471 [details]
New tablerules.css
Comment 32 fantasai 2006-10-16 23:52:32 PDT
Created attachment 242474 [details] [diff] [review]
updated patch

C++ patch, brought up-to-date. Aside from that, the only thing that's changed is that I removed the MapTableFrameInto function, since it's no longer needed.

This fix introduces a lot of very messy CSS rules to make the quirks mode border coloring work properly. We might want to fix bug 84307 first...
Comment 33 fantasai 2006-10-16 23:53:42 PDT
Oh, the patch also includes diff against quirk.css
Comment 34 fantasai 2006-10-17 00:05:38 PDT
Created attachment 242475 [details] [diff] [review]
updated patch
Comment 35 Bernd 2006-10-28 11:12:29 PDT
>This fix introduces a lot of very messy CSS rules to make the quirks mode
>border coloring work properly.

We got a green to remove the quirk at all:
http://groups.google.com/group/mozilla.dev.tech.layout/browse_frm/thread/a09888064bc1eaf1/0f6cb460af73c8bf#0f6cb460af73c8bf

So I think we should remove the quirk first (bug 84307). Fantasai could you (would you like to) do this?
Comment 36 Bernd 2007-06-10 10:47:31 PDT
Fantasai could you please update the patch now that bug 84307 is fixed.
Comment 37 fantasai 2007-07-18 00:04:10 PDT
Created attachment 272740 [details] [diff] [review]
updated patch

same patch, less bitrot
Comment 38 fantasai 2007-07-18 00:09:07 PDT
Created attachment 272741 [details]
Color Weirdness Testcase

For some reason, even with that patch applied (and tablerules.css turned off), a table with 'rules' has a gray border. The grayness disappears with border-collapse: separate or explicit author CSS border-color. Bernd, any idea why this is happening?
Comment 39 fantasai 2007-07-31 11:30:24 PDT
Found the problem. There's still a lot more C++ to remove.
Comment 40 jag (Peter Annema) 2008-02-16 19:52:41 PST
Any progress here?
Comment 41 Bernd 2008-02-16 23:16:21 PST
I will work on this post 1.9
Comment 42 Bernd 2008-05-04 11:19:02 PDT
Fantasai: post 1.9 is coming ...., could you add some information about comment 39. Do you plan to work on this or should I steal the bug and the patch?
Comment 43 fantasai 2008-05-04 11:37:13 PDT
I don't have any concrete plans to work on it, no. If you want to steal it, though, give me a heads up right before you start working on it. FWIW I'm happy to do the CSS part for you.

Wrt C++ that needs to be removed: in addition to your patch above, there seems to be a lot of code in nsTableFrame.cpp that hard-codes rules attribute handling. That needs to be removed as well.
Comment 44 Bernd 2008-05-09 23:06:51 PDT
I converted https://bugzilla.mozilla.org/attachment.cgi?id=41726&action=view into a set of reftests, I will land them once the 3.0 is out.
Comment 45 Bernd 2008-05-17 04:20:37 PDT
Created attachment 321374 [details] [diff] [review]
next rev.
Comment 46 Bernd 2008-05-17 04:21:44 PDT
Created attachment 321375 [details]
updated table rules
Comment 47 Bernd 2008-05-17 04:26:14 PDT
Created attachment 321380 [details]
reftests archive
Comment 48 Bernd 2008-05-17 07:54:43 PDT
Created attachment 321402 [details]
revised to keep quirks mode gray
Comment 49 Bernd 2008-05-17 07:57:38 PDT
I am happy with how the current patch works. I am very satisfied with the code removal from nsHTMLTableElement.cpp and nsHTMLStyleSheet.cpp
Comment 50 Bernd 2008-05-17 08:06:51 PDT
Comment on attachment 321374 [details] [diff] [review]
next rev.

David could you please review this, its post 1.9 stuff, so no time pressure.
Attachment 321402 [details] is the CSS file that does the work.
I know there is the headache prone border collapse stuff at the end of the patch, but its only a removal of the style marker for rules.
Comment 51 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-07-07 17:45:31 PDT
Have you run any page-loading performance tests on this patch?  It seems like it could regress performance pretty easily.
Comment 52 Bernd 2008-07-07 22:33:54 PDT
No, I haven't. Do you have a recommendation what tests should be executed?
Comment 53 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-07-07 23:32:07 PDT
Probably the Talos pageload performance tests that show up on tinderbox.
Comment 54 fantasai 2008-07-07 23:52:22 PDT
Bernd, for the CSS you should start with the New tablerules.css (draft) version, since we don't need to cascade in the quirks mode rules anymore. There's a typo on line 75, but other than that I think it's mostly correct.
Comment 55 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-07-22 16:12:00 PDT
Note that I think you can get performance numbers using http://wiki.mozilla.org/Build:TryServer
Comment 56 Bernd 2008-08-02 11:39:34 PDT
Created attachment 332062 [details] [diff] [review]
patch updated to tip

fantasai: I don't think we should change the gray shade in quirks mode. Why do you think we should change this.
Comment 57 Bernd 2008-08-02 22:44:06 PDT
I did the test with attachment 332062 [details] [diff] [review] at the TryServer.
The builds are at: 
https://build.mozilla.org/tryserver-builds/2008-08-02_12:19-bmlk@gmx.de-table_rules_css/

The tp timing does not change across platforms within the noise of the data:
        Old      New     Diff
Linux: 461.74    444.02  -17
Mac:   313.04    315.51  +2
Win:   447.52    443.86  -4

I am not sure how good is the reference point. 
Comment 58 fantasai 2008-08-06 18:12:12 PDT
I don't think we should change the gray shade in quirks mode. I'm just saying the selectors in the (draft) version of tablerules.css are much simpler and we should use that as the basis for this patch, not the later version. The later version was trying to cascade in the quirky-colored border *styles* from quirk.css, which is why it has so much negation and overriding mess in it.
Comment 59 Bernd 2008-08-16 23:29:49 PDT
Created attachment 334146 [details] [diff] [review]
revised patch

This patch starts now with the tablerules draft as fantasai requested. Trimming the file with the presence of the reftests was a nobrainer.
Comment 60 Bernd 2008-09-12 23:58:36 PDT
Created attachment 338432 [details] [diff] [review]
slightly updated

while the parser will not allow <table><td> constructs a DOM manipulation can achieve this, So I added two table> *> td rules.
Comment 61 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-09-30 00:22:18 PDT
Here are my comments so far.  I still want to look a bit more, especially at the removed code.


I don't see why tablerules.css should be a separate file.  Please just
put it in html.css.

You should be changing nsHTMLTableElement::IsAttributeMapped to remove
attributes that are no longer mapped attributes (which I think are frame
and rules, although you should double-check that there aren't any
aAttributes->GetAttr() calls for them).  (I'm not sure whether you also
want to modify nsHTMLTableElement::ParseAttribute so that we no longer
longer store them as enumerated... I should probably ask sicking what we
should do here.)


I'm going to make a bunch of comments on the style rules.  I might be
missing things that you were considering when you wrote them (including
compatibility with our old behavior or other browsers), so don't rush to
fix these if there are good reasons for them to be the way they are:

+  /* 'border' before 'frame' so 'frame' overrides
+      A border with a given value should, of course, pass that value
+      as the border-width in pixels -> attr mapping */
+table[border] {
+       border: thin outset;
+}
+  /* 'border="0"' suppresses the border */
+table[border="0"] {
+       border: 0 hidden;
+}

Is there a reason that [border="0"] should not only set border-width to
0 but also border-style to hidden?  Why not just make this a single
rule:

table[border]:not([border="0"]) { border: thin outset; }

+table[frame] {
+       border: thin hidden;
+}
+table[frame="above"] {
+       border-top-style: solid;
+}
+table[frame="below"] {
+       border-bottom-style: solid;
+}
+table[frame="lhs"] {
+       border-left-style: solid;
+}
+table[frame="rhs"] {
+       border-right-style: solid;
+}
+table[frame="hsides"] {
+       border-style: solid hidden;
+}
+table[frame="vsides"] {
+       border-style: hidden solid;
+}
+table[frame="box"],
+table[frame="border"] {
+       border-style: solid;
+}

I'm not crazy about the handling of unknown values of frame here.  Why
not skip the table[frame] rule and specify all four styles for each
known value of frame, i.e.,:
  table[frame="above"] { border-style: solid hidden hidden hidden; }
etc.

+  /* 'border' cell borders first */
+table[border] > tr > td,
+table[border] > * > tr > td,
+table[border] > tr > th,
+table[border] > * > tr > th,
+table[border] > td,
+table[border] > th,
+table[border] > * > td,
+table[border] > * > th {
+  border: thin inset;
+}
+table[border="0"] > tr > td,
+table[border="0"] > * > tr > td,
+table[border="0"] > tr > th,
+table[border="0"] > * > tr > th,
+table[border="0"] > td,
+table[border="0"] > th,
+table[border="0"] > * > td,
+table[border="0"] > * > th {
+  border-style: none;
+}

You can simply this by:

(1) beginning each selector with table[border]:not([border="0"]) and
eliminating the second rule

(2) removing "table > tr > td" etc. given that you have "table > * >
td".

That said, do you really need all these options for tree structure?  Did
we support this for all of these combinations in the old code?



In general, I'm a little concerned about handling of unknown values
being inconsistent.  Have you tested those cases?  Are we changing our
behavior?  Is it becoming more or less compatible with other browsers?
(I doubt specs say anything, but if they do... with specs?)
Comment 62 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-12-15 17:56:58 PST
(In reply to comment #61)
> I don't see why tablerules.css should be a separate file.  Please just
> put it in html.css.

Er, actually, we should really get the preshint.css thing sorted out.

> (I'm not sure whether you also
> want to modify nsHTMLTableElement::ParseAttribute so that we no longer
> longer store them as enumerated... I should probably ask sicking what we
> should do here.)

I think this should stay untouched.
Comment 63 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-12-15 17:57:46 PST
That said, one of the reasons I've been hesitant to put a lot of energy into reviewing this is that I'm still somewhat skeptical that it's not going to make our performance numbers worse...
Comment 64 Boris Zbarsky [:bz] (TPAC) 2008-12-15 18:09:28 PST
Tryserver runs talos.  Can we just bounce this patch on there and see how that goes?
Comment 65 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-12-15 20:18:17 PST
See comment 57, in which the noise seems like about the size of the changes I might expect.
Comment 66 Boris Zbarsky [:bz] (TPAC) 2008-12-15 20:26:21 PST
Hmm...  I guess so, for non-mac...
Comment 67 Bernd 2009-01-06 10:46:06 PST
*** Bug 472327 has been marked as a duplicate of this bug. ***
Comment 68 Bernd 2009-01-11 00:40:09 PST
David, this looks like a dead lock, you are reluctant to review this for reasons that I perfectly understand, I am reluctant to spend more time on this if it does not have a chance to get reviewed.

Mean while we have the nasty code in nsHTMLStyleSheet.cpp which is wrong for years now and blocks the refactoring of the border collapse computing as the rules code currently creates a position dependency for the border collapse computation.

I have no problem to trash the patch entirely and start from scratch as long as the mess from nsHTMLStyleSheet.cpp and nsHTMLTableElement.cpp gets removed.

The reftests that I wrote will be anyway there and will make a revision much more easy for me.

Could you please propose a way that has a chance to get reviewed.
Comment 69 Bernd 2009-03-07 22:34:12 PST
Created attachment 366160 [details] [diff] [review]
patch

this patch addresses the review comments.

I pushed it to the try server:

1. run
             Linux      Mac      Win
after        232.91     -        195.62
this         234.59     -        195.78
before       233.62     -        195.70

* the mac talos did not test this.

2. run

             Linux      Mac      Win
after        234.69     315.86   196.02
this         235.83     306.77   196.01
before       232.91     306.74   195.58

so it looks like this is slightly adding 

Linux: 1 ms  (0.5%)
Mac  : too noisy
win  : < 0.5 ms 

What I need here is a decision will this go into the tree. And it would be cool to get this decision not after months of waiting.

The arguments to take this despite the slight slow down are:
0. it fixes CSS problems that we have when running in combination with rules
1. It removes the obviously wrong code from nsHTMLStyleSheet.cpp
2. It removes a bad parameter that make table border collapse computation more complicated. 

If the decision is that the slow down is not worth it, I would appreciate a clear instruction what needs to be done to solve this, namely to achieve the same what arguments 0 and 1 are about.

Btw, the reftest archive (attachment 321380 [details]) got already pushed.
Comment 70 Jean-Francois Larvoire 2009-03-11 10:51:47 PDT
As a victim of one of the bugs dependant on this one, with only an extreeemely slow workaround based on JavaScript, I certainely vote for checking in the patch. :-)
Comment 71 jag (Peter Annema) 2009-03-11 11:51:02 PDT
The 3ms on linux (but barely any change on Mac / Win) looks out of place. Noise?

Assuming the patch passes review, I think the tiny Tp regressions are worth it.
Comment 72 Ryan VanderMeulen [:RyanVM] 2009-04-18 06:04:23 PDT
dbaron, any chance to look at this yet?
Comment 73 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-04-29 22:52:25 PDT
Comment on attachment 366160 [details] [diff] [review]
patch

Given that I've consistently failed to get to this, reassigning the review to bzbarsky.

bz... if you're not likely to be able to do this, I guess you should probably assign it back, though.
Comment 74 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-04-29 22:54:29 PDT
But what about comment 62 -- sorting out creating a preshint.css that is actually loaded at the preshint level?
Comment 75 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-04-29 23:02:22 PDT
Or maybe not, actually.  Maybe we should just decide that that part of the CSS spec is silly, and we should just start handling all presentational hints at the UA level.

I'd be interested if fantasai has an opinion there.
Comment 76 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-04-29 23:10:03 PDT
(In reply to comment #69)
> If the decision is that the slow down is not worth it, I would appreciate a
> clear instruction what needs to be done to solve this, namely to achieve the
> same what arguments 0 and 1 are about.

I suspect a substantial part of the slowdown could be fixed by implementing a :-moz-in-table() pseudo class that takes a sequence of simple selectors as an argument, so that you could replace things like:

table[border]:not([border="0"])> tr > td,
table[border]:not([border="0"])> * > tr > td,
table[border]:not([border="0"])> tr > th,
table[border]:not([border="0"])> * > tr > th,
table[border]:not([border="0"]) > td,
table[border]:not([border="0"])> th,
table[border]:not([border="0"])> * > td,
table[border]:not([border="0"]) > * > th {

with:

td:-moz-in-table(table[border]:not([border="0"])),
th:-moz-in-table(table[border]:not([border="0"])) {

That said, do we even know whether there is a slowdown?  Is try talos accurate enough?
Comment 77 fantasai 2009-04-29 23:36:05 PDT
fantasai's opinion is that the preshint requirements of 2.1 don't go far enough and should have included things like b { font-weight: bold; }. But it is also her opinion that the loading of tablerules.css at preshint or not at preshint level should not block this bugfix.
Comment 78 Bernd 2009-11-09 22:42:07 PST
Created attachment 411362 [details] [diff] [review]
updated to tip
Comment 79 Boris Zbarsky [:bz] (TPAC) 2009-12-17 01:22:11 PST
Comment on attachment 366160 [details] [diff] [review]
patch

Reviewing the "updated to tip" patch, but marking here since that's where the request is.

>+++ b/layout/reftests/table-bordercollapse/reftest.list

Can you maybe add an == line comparing borderhandling-ref.html to a test that sets borders and maybe border-collapse purely through CSS?

>+++ b/layout/style/html.css

>+  /* 'border' before 'frame' so 'frame' overrides

I'm not quite following this... These two rules, for example:

>+table[border]:not([border="0"]) { border: thin outset; }
>+table[frame="above"]  { border-style: solid hidden hidden hidden;}

Mean that <table border="1" frame="above"> will have outset borders all around, won't it, since the first rule has higher specificity than the second rule.  That doesn't seem to be current behavior.

If we want to keep the current behavior, we probably need to change the frame rules here to something like:

  table[frame][frame="above] { border-style: solid hidden hidden hidden; }

to up the specificity, and add a comment explaining why we're doing that.

Can we add some regression tests for this?

Also, should the |table[frame]| rule also override the [border]-set style?  In that case it should be |table[frame][frame]|.  But I thought dbaron asked forthat to be removed in comment 61.  Is there a reason it's still present?

>+  /* Don't set hidden if there's no frame, no border, and rules=none */
>+table[rules="none"]:not([frame]):not([border]),
>+table[rules="none"]:not([frame])[border="0"] {
>+	border: none;
>+}

That rule could use more comment.  That's effectively overriding the default |table[rules]| styling with "border-style:none" and default border width, instead of border-style hidden, right?  Why do we need this override?

>+table[border]:not([border="0"])> tr > td,
>+table[border]:not([border="0"])> tr > th,
...
>+table[border]:not([border="0"])> * > td,
>+table[border]:not([border="0"]) > * > th {

As dbaron said in comment 61, those last two selectors match in all cases where the first two match, and more besides.  So no need for the first two.

On the other hand, as dbaron also asked, do we actually support all these combinations in the old code?  Do we need to keep supporting them?  Do we actually want to put borders on the <tr> in this XHTML:

  <table border="1">
    <div>
      <td></td>
    </div>
  </table>

or do we not care either way?  It seems a little odd to me to put borders on the <td> in that case, but does make the rules simpler (and fewer)....  
    
>+  /*'rules' overrides 'border' settings */

All the rules in this section have lower specificity than the table[border]:not([border]) rules in the previous section, so don't override them, as far as I can see.  That seems like a regression from addressing some of dbaron's comments, but we should have tests for this.... again, this can be fixed by repeating the [rules] selectors as needed.

>+table[rules] > tr > td,
>+table[rules] > * > tr > td,
>+table[rules] > tr > th,
>+table[rules] > * > tr > th {

This doesn't have the |table > td| and |table > * > td| selectors we have in the [border] section.  I personally think that's ok and we should remove those rules from that section too....

Why not just "border: thin none" for the actual declaration here?  Is there a reason to not set border props other than style/width here?

>+table[rules="cols"] > tr > td,
>+table[rules="cols"] > * > tr > td,
>+table[rules="cols"] > tr > th,
>+table[rules="cols"] > * > tr > th {
>+  border-style: none solid;
>+}

Why do you not need "border-width: thin" here, unlike in the rows case?

The rest of this looks fine, but I'd really like to see the additional tests exercising these selectors, then see the selectors fixed.

Sorry again for the lag here; now that I've sorted through the annoying (and not likely to change, since it's all code removal) parts of this patch, future reviews should be much faster!
Comment 80 Bernd 2010-01-03 22:21:06 PST
Created attachment 419865 [details] [diff] [review]
revised patch

1) I revised the rules
2) There are now two test cases that test frame vs border in 50 different scenarios and rules vs border in 35 scenarios.
3) removed more code from nsHTMLTableElement.cpp as the border mapping to the TD is now done in CSS. The removal of code (including the previously done) opens the path to go for the table part of bug 211636
4) we need the more extensive border rules as otherwise dom manipulated tables will not draw borders around the TD. see http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/363370-1.html and http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/368651-1.html. They rely on pseudo frame creation. In the static case the offending tag would be pushed out, but in the dynamic case it will not.
Comment 81 Boris Zbarsky [:bz] (TPAC) 2010-01-06 11:30:36 PST
Bernd, can you possibly post an interdiff between the two patches?  The bugzilla interdiff seems to be lying to me (and in particular is too small to account for the size difference between the two, I think).
Comment 82 Boris Zbarsky [:bz] (TPAC) 2010-01-06 11:35:45 PST
Created attachment 420381 [details] [diff] [review]
Interdiff in question

Nevermind; I got hg to just cooperate.  Looks like bugzilla was missing a lot of test files.
Comment 83 Bernd 2010-01-06 12:20:25 PST
change notices for the interdiff

nsHTMTableElement.cpp
thats 3) from comment 80

the changes for bordercol.css are intended to leave the color unchanged with respect to out current rendering

the change from &nbsp; to <div/> are to due to linux and macs doing strange things with pixel alignment (tryserver is great) 

then come the new test cases as required

and finally the revised rules.
Comment 84 Bernd 2010-01-06 12:25:26 PST
I had a full green run try server run today at 00:26
Comment 85 Boris Zbarsky [:bz] (TPAC) 2010-01-06 15:14:42 PST
Comment on attachment 419865 [details] [diff] [review]
revised patch

>+++ b/layout/style/html.css
>+  /* Put this first so 'border' and 'frame' rules can override it. */
>+table[rules] {border: thin hidden;}

Please put spaces after the '{' and before the '}' (and similar for the other rules here).  Newlines might be even better for this rule, but I can see how for some of the "frame" rules they aren't desirable.

>+/*increased specifity to compete with [border]:not([border="0"]) rule above*/

"specificity", not "specifity".  And spaces after "/*" and before "*/", please.  Similar for other comments here.

>+/*'rules' overrides 'border' settings  (increased specifity to achieve this)*/
>+table[rules]:not([rules=""]) > tr > td,

It's probably better to do:

  table[rules][rules] > tr > td

and the like.  Easier to read, and a bit faster to match.  Unless you really do want to be checking for nonempty rules="..." values here?  If so, why?  That could use a comment.

This is still missing the "table > td" and "table > th" selectors that the border rules have.  Either put those selectors in both places, or neither, right?  Need that here, for [rules="none"] and for [rules="all"].

With those issues fixed, looks good.
Comment 86 Bernd 2010-01-10 07:33:26 PST
I pushed http://hg.mozilla.org/mozilla-central/rev/ef065d84aaa4

I looked up 8 tp4 datasets on linux before the checkin and 8 after the checkin,

the tp4 times are with std dev.
after:    521.92 +- 2.21
before:   522.26 +- 1.86


on the mac.
after:    643.06 +- 11.01
before:   644.15 +- 18.44

on windows (with only 6 samples)
after:   600,58 +- 29.17
before:  631,46 +- 30.53

So in non of the cases I did observe a performance degradation that is measurable with tp4. One could claim a improvement on win tp4. But he numbers there oscillate so much that I would not rely on them.
Comment 87 Bernd 2010-01-10 08:25:24 PST
Created attachment 420962 [details] [diff] [review]
test cases for the dependent bugs that got fixed

I am pushing this over the try-server currently
Comment 88 Bernd 2010-01-10 12:49:23 PST
Created attachment 420977 [details]
bundle for checkin
Comment 90 Boris Zbarsky [:bz] (TPAC) 2010-01-16 11:48:41 PST
That doesn't look like the right changeset url.
Comment 93 Dão Gottwald [:dao] 2010-01-16 12:11:51 PST
(In reply to comment #92)
> http://hg.mozilla.org/mozilla-central/rev/4bb9868650a8

REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/moz2_slave/mozilla-central-win32-debug-unittest-reftest/build/reftest/tests/layout/reftests/bugs/167496-1.html

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263664485.1263666317.725.gz
Comment 94 Jesse Ruderman 2010-01-16 18:36:16 PST
Bug 540228 has been filed on that fairly-persistent orange.
Comment 95 Daniel Holbert [:dholbert] 2010-03-05 10:48:30 PST
FWIW, I landed a followup to fix a compiler warning for a variable that became unused after this bug's patch landed. (with rs=bernd over IRC)
http://hg.mozilla.org/mozilla-central/rev/0b8ddbe14c06

Note You need to log in before you can comment on or make changes to this bug.