Bug 167567

Summary: table with BORDER="00" (leading zeros) displays a border
Product: [Applications] konqueror Reporter: Jonathan Marten <jjm>
Component: khtmlAssignee: Konqueror Developers <konq-bugs>
Status: RESOLVED FIXED    
Severity: normal CC: germain
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Compiled Sources   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:
Attachments: HTML page for test case
Screen shot from KDE4 trunk
patch... evil, but compatible.
Same evil, done better(?)

Description Jonathan Marten 2008-07-28 08:57:26 UTC
Version:            (using Devel)
Installed from:    Compiled sources
Compiler:          gcc version 4.2.4 (Gentoo 4.2.4 p1.0)
 
OS:                Linux

There doesn't seem to be any HTML standard forbidding leading zeros in numbers, so presumably

<TABLE BORDER="00">

should be equivalent to

<TABLE BORDER="0">

In KDE3 this was the case and the table in both cases displayed with no border.

In KDE4 from trunk this displays a 1-pixel border (equivalent to BORDER="1").

Screen shot and test case are attached.
Comment 1 Jonathan Marten 2008-07-28 08:58:29 UTC
Created attachment 26451 [details]
HTML page for test case
Comment 2 Jonathan Marten 2008-07-28 08:59:16 UTC
Created attachment 26452 [details]
Screen shot from KDE4 trunk
Comment 3 Maksim Orlovich 2008-07-28 17:27:57 UTC
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). 

Comment 4 Germain Garand 2008-07-29 03:43:53 UTC
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
Comment 5 Germain Garand 2008-07-29 03:49:29 UTC
Created attachment 26471 [details]
patch... evil, but compatible.
Comment 6 Maksim Orlovich 2008-07-29 04:28:01 UTC
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..

Comment 7 Maksim Orlovich 2008-08-01 18:50:54 UTC
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...)
Comment 8 Maksim Orlovich 2008-08-08 00:03:02 UTC
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
Comment 9 Maksim Orlovich 2008-08-08 00:06:49 UTC
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
Comment 10 Maksim Orlovich 2008-08-08 00:11:24 UTC
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
Comment 11 Germain Garand 2008-08-31 15:16:15 UTC
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)
Comment 12 Germain Garand 2008-08-31 17:11:05 UTC
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
Comment 13 Germain Garand 2008-09-05 03:12:37 UTC
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: