290377 - [FIXr]computed value of 'border-width' is incorrect

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

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

This is the bug that makes the rule tree a pain to deal with (again).

The computed value of 'border-width' is supposed to be zero when 'border-style'
is 'none' or 'hidden', which means it should be inherited that way.  But we
can't really store that in the rule tree because it would break some cascading
cases.  But we also really don't want to disable the rule tree optimizations (as
we did for the section 9.7 fixups) because this is the default case.

Comment 1

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

Attachment: testcase

Comment 2

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

Attachment: testcase we need to be careful not to break

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Quick hack

I think this actually fully fixes the bug...

That said, I'd still like to expose this better on nsStyleBorder.  As things
stand, we have a lot of weird code in various places because nsStyleBorder
can't answer the simple question of "is there a border on this side, and how
wide should it be?"

I'm also still trying to decide whether the cached border stuff is really worth
it...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

This fixes this bug and bug 273500...  I'm not quite sure whether to add the
side constants to gfxCore or nsMargin; in the latter case I'd have to make
nsStyleConsts include nsMargin (which might be ok).

Note that if we find that the "live" width/style updating is too slow we can go
back to having a RecalcData() function like we used to have.  But I suspect
this won't be an issue.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Note: I should also change the comment at
http://lxr.mozilla.org/seamonkey/source/layout/base/nsStyleConsts.h#233 to
something more like:

  "Possible enumerated specified values of border-*-width, used by nsCSSMargin."

Comment 6

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

Comment on attachment 180963 [details] [diff] [review]
Proposed patch

>Index: gfx/public/nsMargin.h
>+  nscoord& side(PRUint8 aSide) {
>+    NS_PRECONDITION(NS_SIDE_TOP == 0 && NS_SIDE_RIGHT == 1 &&
>+                    NS_SIDE_BOTTOM == 2 && NS_SIDE_LEFT == 3,
>+                    "Unexpected side constants");
>+    NS_PRECONDITION(aSide <= NS_SIDE_LEFT, "Out of range side");
>+    return *(&top + aSide);
>+  }    
>+
>+#if (NS_SIDE_TOP == 0) && (NS_SIDE_RIGHT == 1) && (NS_SIDE_BOTTOM == 2) && (NS_SIDE_LEFT == 3)
>+  nscoord side(PRUint8 aSide) const {
>+    NS_PRECONDITION(aSide <= NS_SIDE_LEFT, "Out of range side");
>+    return *(&top + aSide);
>+  }    
>+#else
>+#error "Somebody changed the side constants."
>+#endif

Just put both inside the #if and skip the first NS_PRECONDITION.

Comment 7

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

Comment on attachment 180963 [details] [diff] [review]
Proposed patch

>Index: layout/style/nsStyleStruct.h
>+  void SetBorderWidth(PRUint8 aSide, nscoord aBorderWidth)
>   {
>+    NS_ASSERTION(aSide <= NS_SIDE_LEFT, "bad side");

>+  nscoord GetBorderWidth(PRUint8 aSide) const
>+  {
>+    NS_ASSERTION(aSide <= NS_SIDE_LEFT, "bad side");

Probably better to drop these assertions, and just assume that nsMargin::side
has them.


Can you eliminate the fourth and fifth parameters to CalcSidesFor and fifth and
sixth of CalcSideFor?

>Index: layout/generic/nsInlineFrame.cpp

You can remove IsBorderZero.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Can you eliminate the fourth and fifth parameters to CalcSidesFor and fifth and
> sixth of CalcSideFor?

Yes.  Will do that and make the other changes you asked for.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

This should be pretty safe and simplifies the border code quite a bit, in
addition to making us inherit border widths correctly...

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 182136 [details] [diff] [review]
Updated to comments

a=asa

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Actually updated to comments....

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed.  Even seemed to help out Tp a tad....  Still waiting on luna's Tdhtml to
settle from what I think is a temporary spike.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Well, looks like luna's spike is not quite temporary.  Trying to figure out
what's going on there.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: The update to comments.....

I don't know what was going on this morning, but I think I got confused between
windows on two different machines.... :(  That'll teach me to check in before
I'm really awake.

This diff, applied on top of the previous one, addresses the comments.	I just
checked it in.

Comment 15

[Boris Zbarsky [:bzbarsky]]

And looks like the times on luna are just being very jumpy... it's been back and
forth from 2030 to 2080 ever since roc's scrollframe changes.  :(