290362 - [FIX]Border-color changes can cause reflow hints

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

See bug 290297.  Should have caught this while fixing bug 275139, really... :(

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

The comments say it all.  The header change is removing an unused (and wrong,
looks to me) method.

Comment 2

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

Comment on attachment 180739 [details] [diff] [review]
Patch

>         return NS_STYLE_HINT_VISUAL;
>       }
>     }
>     if (mBorderRadius != aOther.mBorderRadius) {
>       return NS_STYLE_HINT_VISUAL;
>     }

While we're fixing obvious bugs in this code, how about also adding a PRBool
visual = PR_FALSE; at the beginning of the function and changing the above to:

    visual = PR_TRUE;
  }
}
if (visual || mBorderRadius != aOther.mBorderRadius) {
  return NS_STYLE_HINT_VISUAL;
}

Currently a style change from solid to dotted on the top border combined with a
change from none to dotted on the right border will only cause a repaint. 
(That's a pretty easy change to make with shorthands, never mind batching.)

Comment 3

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

(This also led me to filing bug 290377 when I noticed that this gives a reflow
hint for 'border-width' changes even when 'border-style' is 'none'.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase for the second problem David pointed out

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch fixing both issues

Comment 6

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

Comment on attachment 180769 [details] [diff] [review]
Patch fixing both issues

r+sr=dbaron, although these two could be condensed:

>+        (mBorderColors && !aOther.mBorderColors) ||
>+        (!mBorderColors && aOther.mBorderColors)) {

into

 !mBorderColors != !aOther.mBorderColors

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180769 [details] [diff] [review]
Patch fixing both issues

Requesting 1.8 approval for this pretty simple fix.  This should be quite safe.

Comment 8

[Asa Dotzler [:asa]]

Comment on attachment 180769 [details] [diff] [review]
Patch fixing both issues

a=asa

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed.