148501 - table border is displayed even if its width is specified as 0

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Koike Kazuhiko]

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.

Comment 1

[Koike Kazuhiko]

Attachment: Testcase

Comment 2

[Koike Kazuhiko]

Attachment: Testcase #2

Comment 3

[Boris Zbarsky [:bzbarsky]]

tables.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Andrew Schultz]

this is a regression from bug 142019
==> glazman

Comment 6

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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

[Andrew Schultz]

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

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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.

Comment 10

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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.

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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.

Comment 12

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Confirming !!! I found a nice big bug in nsCSSDeclaration::SetValueImportant()
handling of shorthand properties. Patch pending.

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.0

Comment 14

[Boris Zbarsky [:bzbarsky]]

So... Why is a Reset() sufficient for non-shorthand properties but shorthand
properties require deletion of the member?

Comment 15

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 17

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 87367 [details] [diff] [review]
patch v1.0

patch is obsolete

Comment 18

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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.

Comment 19

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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 !!!

Comment 20

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v2.0

Comment 21

[Kathleen :Brade]

Comment on attachment 87517 [details] [diff] [review]
patch v2.0

r=brade great work!

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Johnny Stenback (:jst)]

Comment on attachment 87517 [details] [diff] [review]
patch v2.0

sr=jst

Comment 25

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

checked in trunk ; nominating, see comment #19.

Comment 26

[Kevin McCluskey (gone)]

nsbeta1+

Comment 27

[Kathleen :Brade]

does anyone know if Daniel sent e-mail to drivers to check this in on the branch?

Comment 28

[Jaime Rodriguez, Jr.]

chris - can you pls verify this fix on the branch? thanks!

Comment 29

[Jaime Rodriguez, Jr.]

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.

Comment 30

[Chris Petersen]

Verified on Linux Redhat 2002-06-24-08 trunk build. 

Comment 31

[Chris Petersen]

Two questions:

Is this a Linux only issue and has this been checked into latest branch ?

Comment 32

[Andrew Schultz]

patch has not landed on the branch (or received approval)

Comment 33

[Chris Petersen]

Verified on OS X 2002-06-26-08 trunk and Windows Me 2002-06-26-08 trunk.

Comment 34

[Jaime Rodriguez, Jr.]

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!

Comment 35

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 36

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

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.

Comment 37

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

And, for what it's worth, the testcases look fine in my Linux branch 
build.

Comment 38

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

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.