419167 - right/left borders don't adopt user-defined color (colour) scheme (untick 'Allow pages to choose their own colors')

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Steve England [:stevee]]

Steve Horan found this bug and the range - I'm filing a clean report on his behalf.

1. New profile, start firefox
2. Tools > Options > Content > Colors
3. Set Text to White
4. Set Background to Red
5. Untick "Allow pages to choose their own colors, instead of my selections above"
6. OK, OK.
7. Visit http://www.pogdesign.co.uk/cat/

Expected:
- Cell borders should be all white

Actual:
- Cell borders top and bottom are white (expected), but cell sides are black and so not the color we expect them to be.

works: 20070704_1132_firefox-3.0a7pre.en-US.win32
broken: 20070704_1245_firefox-3.0a7pre.en-US.win32

Checkins to module PhoenixTinderbox between 2007-07-04 11:32 and 2007-07-04 12:45 : 
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-04+11%3A32&maxdate=2007-07-04+12%3A45&cvsroot=%2Fcvsroot

Due to bug 74880 perhaps?

Comment 1

[Steve England [:stevee]]

Attachment: Expected rendering (white borders around all cells)

Comment 2

[Steve England [:stevee]]

Attachment: Regressed rendering (vertical broders are black)

Comment 3

[Bernd]

Can you provide a reduced test case? Bugs in the layout component should be filed with one.

Comment 4

[Steve England [:stevee]]

Attachment: testcase to use with str

Comment 5

[Bernd]

Attachment: screenshot inspector

Comment 6

[Bernd]

Attachment: screenshot inspector

Comment 7

[Bernd]

please notice on the first screen shot the strange property:
"border-right-color-value" instead of "border-right-color"

Comment 8

[Bernd]

they are not strange but introduced in bug 74880. So this is certainly a regression

Comment 9

[Bernd]

Attachment: same testcase with a div

Comment 10

[Bernd]

this is not a table but a general style resolution issue

Comment 11

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

The properties to ignore need updating in nsCSSCompressedDataBlock::MapRuleInfoInto to reflect the changes in bug 74880.

Comment 12

[Bernd]

Attachment: patch to make the mochitest to test all 4 edges

Comment 13

[Bernd]

Attachment: patch to make the mochitest to test all 4 edges

Comment 14

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

... although I should probably just do what Boris wanted in bug 58048 comment 29 / bug 58048 comment 31.

Comment 15

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

(Also, looks like this bug was originally filed as bug 417919.)

Comment 16

[Bernd]

Attachment: do what David described first

that indeed fixes the issue

Comment 17

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

I think you need to check both source and value; otherwise somebody could override with -moz-border-start-color, etc.

Comment 18

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

(Note that the refactoring could be a separate bug; but if done here, it's probably best to add a |flags| parameter to the non-SHORTHAND CSS_PROP macro that's extensible to other booleans later, and put an array of the results (and the enum values) in nsCSSProps.)

Comment 19

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

Though given the increase in length from adding 4 more properties to the code, we probably should convert to using an array now.

Comment 20

[Bernd]

Attachment: patch without the array

This checks both source and value. The test verifies now that also the -moz-border-start-color and -moz-border-end-color is blocked.

However the conversion to an array is not done. I am not sure where to place the array. Should that be close to http://mxr.mozilla.org/mozilla/source/layout/style/nsCSSProps.cpp#1296

Comment 21

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

The array should look a lot like nsCSSProps::kTypeTable -- we can add a kFlagsTable just like kTypeTable, and add a flags parameter to the CSS_PROP macro (where the only flag so far is eColorChoice = (1<<0)), and the flag values are defined in nsCSSProps.h as an enum within the nsCSSProps subclass -- since it's a macro the definition doesn't actually need to be visible to the other users of nsCSSPropList.h).

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bernd, hope you don't mind me assigning this to you, since you're working on it.

Comment 23

[Bernd]

> hope you don't mind me assigning this to you, since you're working on
it.
roc: If you could pass also the clue stick to parse comment 21 it would be beneficial. I don't get it. Probably the best thing is to reassign the bug.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

He's just saying add a "flags" parameter to CSS_PROP. The only flag we'll need right now for this bug is eColorChoice. Then you'd define kTypeTable like this:

const nsCSSType nsCSSProps::kFlagsTable[eCSSProperty_COUNT_no_shorthands] = {
#define CSS_PROP(name_, id_, method_, datastruct_, member_, type_, kwtable_, flags_) flags_,
#include "nsCSSPropList.h"
#undef CSS_PROP

Comment 25

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

Comment on attachment 305371 [details] [diff] [review]
patch without the array

r+sr=dbaron.

Why don't we just get this one in and I'll worry about converting this code over later.

Requesting approval, as this is very low risk; it just corrects the list of properties that are ignored by this pref to reflect that two of the properties were split up into pieces.

Comment 26

[Bernd]

>Why don't we just get this one in

what a superb idea, its exactly what I wan't 

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305371 [details] [diff] [review]
patch without the array

a1.9=beltzner

Comment 28

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

I can land this for you (and will pretty soon) unless you prefer to land it yourself.

Comment 29

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

I filed bug 421197 as followup for the array.

Checked in to trunk, 2008-03-05 16:03 -0800.  Thanks for the patch.

Comment 30

[Bernd]

Thank you for checking this in, David you can of course checkin my stuff. This is a blanket permission, the time when cvs blame was important to me is long over and  watching tinderbox is not really entertaining anymore, so I am thankful for every checkin that I do not need to make.

Comment 31

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008030600 Minefield/3.0b5pre ID:2008030600

Following STR the borders are now all white as expected.
--> VERIFIED