Bug 167567

Created attachment 26451 [details]
HTML page for test case

Created attachment 26452 [details]
Screen shot from KDE4 trunk

This would be why, from presentational.css:
table[border="0"] > tr > td

... which isn't robust enough for this, or even for something like " 0 ", while 
toInt is (though looking at what Qt does, I think DOMString::toInt should avoid Qt). 

I don't think the robustness of matching is a problem. There are other attributes that are using exact matches, and they do not tolerate whitespace changes.
e.g. right align is usually implemented with [align="right"], and you can verify align="   right" does not work in other browsers.

What Gecko and Opera appear to do here, is to actually forcefully change the attribute's value to something that (makes sense | is canonicalized).

e.g. border="foo" will be silently changed to border="1", and border="0000" to border="0"

as can be verified with this testcase:

data:text/html,<style>table[border="aaaa"] { background-color: red; } table[border="0"] { background-color:orange; } table[border="1"] { background-color:green; }</style><table border="aaaa"><tr><td>lorem

Created attachment 26471 [details]
patch... evil, but compatible.

First of all, I think we may have to do this for more attributes... HTML5 has the reflecting attribute concept (though it does doesn't seem say which attributes have these property)... Of course, that also includes the absolutely horrifying style reflection, which can't be done in any clean way..

As for the patch: This wouldn't do anything sensible if the attribute has a full-blown Attr, I am afraid. I think we want a dedicated method... Will put one together, but I am currently debugging some refcounting stuff, which has to be finished first, for obvious reasons..

Created attachment 26552 [details]
Same evil, done better(?)

So here is my take --- just adds a new function that handles the Attr nodes
more gracefully. (Not that anything that has to do with Attr can be
graceful...)

SVN commit 843791 by orlovich:

- Add ability to rewrite attributes on their parsing.
This required making sure that if we have an AttrImpl it still gets passed down 
to parseAttribute, hence the changes in the overloads for that
- Use that ability to fix border=00 on tables, as suggested by Germain.
It seems to not always restyle properly when border attributed changed dynamically, though,
hence keeping this open.
CCBUG:167567


 M  +2 -0      html/html_tableimpl.cpp  
 M  +30 -3     xml/dom_elementimpl.cpp  
 M  +18 -2     xml/dom_elementimpl.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=843791

SVN commit 843794 by orlovich:

Merged revision 843791:
- Add ability to rewrite attributes on their parsing.
This required making sure that if we have an AttrImpl it still gets passed down 
to parseAttribute, hence the changes in the overloads for that
- Use that ability to fix border=00 on tables, as suggested by Germain.
It seems to not always restyle properly when border attributed changed dynamically, though,
hence keeping this open.
CCBUG:167567

 M  +2 -0      html/html_tableimpl.cpp  
 M  +30 -3     xml/dom_elementimpl.cpp  
 M  +18 -2     xml/dom_elementimpl.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=843794

SVN commit 843795 by orlovich:

Regression test for #167567
CCBUG:167567


 A             baseline/tables/border-00.html-dom  
 AM            baseline/tables/border-00.html-dump.png  
 A             baseline/tables/border-00.html-render  
 A             tests/tables/border-00.html  


WebSVN link: http://websvn.kde.org/?view=rev&revision=843795

nice patch!
nitpick: it's missing the compatibility bit about NaN being translated to border=1, so I'll add that (see testcase in #4 : should be green, not just orange :)

> It seems to not always restyle properly when border attributed changed
> dynamically, though, hence keeping this open.

is this really related? I tried to build a testcase with onmouseover, seemed to work +/- OK (oddly, one side of the border would sometime not redraw, never the same one)

SVN commit 855322 by ggarand:

add missing compatibility bit
<table border=NaN> translates to <table border=1>

CCBUG:167567


 M  +4 -1      html_tableimpl.cpp  


--- trunk/KDE/kdelibs/khtml/html/html_tableimpl.cpp #855321:855322
@@ -440,6 +440,7 @@
     case ATTR_BORDER:
     {
         int border;
+        bool ok = true;
         // ### this needs more work, as the border value is not only
         //     the border of the box, but also between the cells
         if(!attr->val())
@@ -447,7 +448,9 @@
         else if(attr->val()->l == 0)
             border = 1;
         else
-            border = attr->val()->toInt();
+            border = attr->val()->toInt(&ok);
+        if (!ok)
+            border = 1;
 #ifdef DEBUG_DRAW_BORDER
         border=1;
 #endif

SVN commit 857211 by ggarand:

ah, now I see what SadEagle meant... *inner* borders.
must dirty all cells when the border attribute is updated.

BUG: 167567


 M  +5 -2      html_tableimpl.cpp  


--- trunk/KDE/kdelibs/khtml/html/html_tableimpl.cpp #857210:857211
@@ -465,8 +465,11 @@
         else
             frame = Box, rules = All;
 
-
-        if (attached()) updateFrame();
+        if (attached()) {
+            updateFrame();
+            if (tFirstBody())
+                setTableCellsChanged(tFirstBody());
+        }
         break;
     }
     case ATTR_BGCOLOR: