Closed
Bug 148501
Opened 22 years ago
Closed 22 years ago
table border is displayed even if its width is specified as 0
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.0.1
People
(Reporter: kazhik, Assigned: glazou)
References
Details
(Keywords: regression, testcase, verifyme, Whiteboard: [ETA 06/28])
Attachments
(2 files, 2 obsolete files)
1.68 KB,
text/html
|
Details | |
8.14 KB,
patch
|
Brade
:
review+
jst
:
superreview+
jud
:
approval+
|
Details | Diff | Splinter Review |
table border is displayed even if its width is specified as 0. Style: <style type="text/css"> .isweb { border:0px ! important; border:none ! important; } </style> Table: <table class="isweb" BORDER="0"> <tr><td>Cell</td><td>Cell</td></tr> <tr><td>Cell</td><td>Cell</td></tr> </table> Table border shouldn't be displayed in this case. This bug appeared recently, between 18 May and 24 May.
Reporter | ||
Comment 1•22 years ago
|
||
Reporter | ||
Comment 2•22 years ago
|
||
Attachment #85899 -
Attachment is obsolete: true
Comment 4•22 years ago
|
||
Actually, this looks like style system (see the second testcase). Looking in Inspector, the combination of the two !important rules produces an odd situation -- the value of border-width is "medium" for all the sides, but the colors and styles are undefined (look like empty strings). I suspect that |border="0"| on the table sets the border-style to something other than "none" and sets the border-width to 0. But we already have an !important border width, so that takes precedence. We do _not_, however, have an !important border-style for some reason....
Component: HTMLTables → Style System
Comment 5•22 years ago
|
||
this is a regression from bug 142019 ==> glazman
Assignee: attinasi → glazman
Assignee | ||
Comment 6•22 years ago
|
||
Andrew, could you please explain how you determined this is a regression caused by fix for bug 142019 ? Not that I don't believe you, but your hints can be helpful.
Comment 7•22 years ago
|
||
I backed it out of current CVS after noticing that trunk build 2002052021 worked and 2002052121 did not work... probably not very help hints. :(
Comment 8•22 years ago
|
||
Hmm... So if you take out just the changes to the two GetValue() functions, does that fix this? The rest of the changes in that bug should have had absolutely no effect on this....
Assignee | ||
Comment 9•22 years ago
|
||
ok, seeing now the bug, accepting and nominating nsbeta1. I still don't understand how this is a regression caused by my check-in but I'll look into it urgently.
Keywords: nsbeta1
Assignee | ||
Comment 10•22 years ago
|
||
confirming root of the problem is my check-in. God only knows why for the moment since I don't see anything able to cause that. Investigating.
Assignee | ||
Comment 11•22 years ago
|
||
I investigated during hours without understanding where the problem comes from. So finally, I added a call to List(stdout, 2); at the beginning of nsCSSDeclaration::ToString() to see what was really in the style engine. Of course, I removed first the code that Andrew said to be the cause of the problem. I opened with Composer the following doc : <table style="border:0px !important; border:none ! important"> <tbody> <tr><td>Cell</td><td>Cell</td></tr> <tr><td>Cell</td><td>Cell</td></tr> </tbody> </table> Here is what I saw on the console, and that's really really weird (just prettyfied for readibility) : { border-color: border-style: } ! important { border-top-width: 1[0x1]enum border-right-width: 1[0x1]enum border-bottom-width: 1[0x1]enum border-left-width: 1[0x1]enum border-color: 2[0x2]enum 2[0x2]enum 2[0x2]enum 2[0x2]enum border-style: none none none none } The weird part is of course the two first lines. I don't understand where this comes from. Continuing investigation but I strongly suspect now the current bug has nothing to do with my code, my code just exposing a deeper bug in the style engine's handling of important shorthands.
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•22 years ago
|
||
Confirming !!! I found a nice big bug in nsCSSDeclaration::SetValueImportant() handling of shorthand properties. Patch pending.
Assignee | ||
Comment 13•22 years ago
|
||
Comment 14•22 years ago
|
||
So... Why is a Reset() sufficient for non-shorthand properties but shorthand properties require deletion of the member?
Assignee | ||
Comment 15•22 years ago
|
||
Boris : the difference does not reside really in shorthand or not shorthand. But in nsCSSValue alone or nsCSSValue in a nsCSSRect. GetValue() does not look at important values if the rect is not null. See nsCSSDeclaration.h
Comment 16•22 years ago
|
||
Um... GetValue() does GetValueIsImportant(), and if that's true gets the value from mImportant.... I see nothing relevant in nsCSSDeclaration.h; where should I be looking?
Assignee | ||
Comment 17•22 years ago
|
||
Comment on attachment 87367 [details] [diff] [review] patch v1.0 patch is obsolete
Attachment #87367 -
Attachment is obsolete: true
Assignee | ||
Comment 18•22 years ago
|
||
grumble... and yay at the same time ! I have understood what's going on here : 1. I have modified nsCSSDeclaration::GetValue() in fix for bug 142019 so it returns mImportant->GetValue() if the value of the property is !important 2. *BUT* CSSParserImpl::AppendValue() does not expect that GetValue() looks for important values because the parser, when encountering an important value, stores it first as a normal value and then asks the nsCSSDeclaration to 'promote' the importance of the value... So let's summarize what happens when the parser sees the declaration block { border: 0px ! important ; border : none ! important } ; this summary is what happens _with_ the bug : a) 'border : 0px' is parsed b) this is expansed to 0px for all border-*-width, none for all border-*-style and -moz-use-text-color for all border-*-color ; nsCSSParserImpl::AppendValue() detects that the previous value of the properties is different from the ones we want to set now, so the new values are stored into nsCSSDeclaration, but *not* as important declarations c) the parser detects the ! important and calls nsCSSDeclaration::SetValueImportant() for the border shorthand d) the value of each of the properties set by the shorthand is promoted to !important e) 'border : none' is parsed d) expansed to medium none -moz-use-text-color nsCSSParserImpl::AppendValue() calls nsCSSDeclaration::GetValue() to check if the existing values for border properties are different from the one we want to set now. ====> BUT BECAUSE GetValue() now calls itself to retrieve !important values too, nsCSSParserImpl::AppendValue() detects that 'none' for style and '-moz-use-text-color' for color are already there ! So it does *not* call nsCSSDeclaration::AppendValue() and the not-important values of border-*-style and border-*-color remain the null value. e) BUT the parser continues its normal life and now detects the !important attached to 'border: none'. And it promotes to !important the not-important values of border properties. Including null for border-*-color and border-*-style !!!! Pfiuuuu. That was tricky. I have discovered very ugly things in the style engine tracking this bug and will file a new bug about style engine's handling of important shorthands. I'll have a new patch for the current bug in a few minutes.
Assignee | ||
Comment 19•22 years ago
|
||
Wooof, Kevin, the fix for the current bug needs to go into the branch : <p style="color: red ! important; color: blue"> foo </p> is erroneously reduced to <p style="color: blue"> foo </p> without it !!!
Assignee | ||
Comment 20•22 years ago
|
||
Comment 21•22 years ago
|
||
Comment on attachment 87517 [details] [diff] [review] patch v2.0 r=brade great work!
Attachment #87517 -
Flags: review+
Comment 22•22 years ago
|
||
Comment on attachment 87517 [details] [diff] [review] patch v2.0 r=bzbarsky as well. This reasoning I buy. ;) cc me on the other bug you plan to file (the footprint issues, I assume?)
Comment 23•22 years ago
|
||
Oh. An you may want to add a comment explaining why we need the GetValueOrImportantValue thing (just a pointer to this bug, basically).
Comment 24•22 years ago
|
||
Comment on attachment 87517 [details] [diff] [review] patch v2.0 sr=jst
Attachment #87517 -
Flags: superreview+
Assignee | ||
Comment 25•22 years ago
|
||
checked in trunk ; nominating, see comment #19.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Keywords: adt1.0.1,
mozilla1.0.1
Resolution: --- → FIXED
Comment 27•22 years ago
|
||
does anyone know if Daniel sent e-mail to drivers to check this in on the branch?
Comment 28•22 years ago
|
||
chris - can you pls verify this fix on the branch? thanks!
Keywords: verifyme
Whiteboard: [adt2 RTM] [ETA 06/21]
Comment 29•22 years ago
|
||
can we get someone in QA to verify this fix? its been baking on the trunk since last week, and we'd like to take it for 1.0.1.
Blocks: 143047
Target Milestone: --- → mozilla1.0.1
Comment 30•22 years ago
|
||
Verified on Linux Redhat 2002-06-24-08 trunk build.
Status: RESOLVED → VERIFIED
Comment 31•22 years ago
|
||
Two questions: Is this a Linux only issue and has this been checked into latest branch ?
Comment 32•22 years ago
|
||
patch has not landed on the branch (or received approval)
Comment 33•22 years ago
|
||
Verified on OS X 2002-06-26-08 trunk and Windows Me 2002-06-26-08 trunk.
Comment 34•22 years ago
|
||
adt1.0.1+ (on the ADT's behalf) approval for checkin to the 1.0 branch. pls check this in asap, then add the "fuxed1.0.1" keyword. thanks!
Updated•22 years ago
|
Attachment #87517 -
Flags: approval+
Comment 35•22 years ago
|
||
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
Kevin says this doesn't apply cleanly to the branch. I think that's because bug 142019 never landed on the branch either. Do we want bug 142019 on the branch? I don't see any reason to land this on the branch without it, and I don't see any nominations on bug 142019.
And, for what it's worth, the testcases look fine in my Linux branch build.
I'm removing the keywords: "adt1.0.1+, approval, mozilla1.0.1+, nsbeta1+" since I believe this bug does not exist (and has never existed) on the branch. If anyone has any reason to think that the bug actually exists on the branch, please readd them and we'll look in to fixing it.
Keywords: adt1.0.1+,
approval,
mozilla1.0.1+,
nsbeta1+
Whiteboard: [adt2 RTM] [ETA 06/28] → [ETA 06/28]
You need to log in
before you can comment on or make changes to this bug.
Description
•