Closed Bug 419167 Opened 15 years ago Closed 15 years ago

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


(Core :: Layout, defect, P2)






(Reporter: stevee, Assigned: bernd_mozilla)




(Keywords: regression, testcase)


(7 files, 3 obsolete files)

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

- Cell borders should be all white

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

Due to bug 74880 perhaps?
Flags: blocking1.9?
Can you provide a reduced test case? Bugs in the layout component should be filed with one.
Keywords: testcase
Attached image screenshot inspector
Attached image screenshot inspector
please notice on the first screen shot the strange property:
"border-right-color-value" instead of "border-right-color"
they are not strange but introduced in bug 74880. So this is certainly a regression
this is not a table but a general style resolution issue
Component: Layout: Tables → Layout
QA Contact: layout.tables → layout
Summary: table borders don't alwys adopt user-defined color (colour) scheme (untick 'Allow pages to choose their own colors') → right/left borders don't adopt user-defined color (colour) scheme (untick 'Allow pages to choose their own colors')
The properties to ignore need updating in nsCSSCompressedDataBlock::MapRuleInfoInto to reflect the changes in bug 74880.
Attachment #305227 - Attachment is obsolete: true
... although I should probably just do what Boris wanted in bug 58048 comment 29 / bug 58048 comment 31.
(Also, looks like this bug was originally filed as bug 417919.)
Attached patch do what David described first (obsolete) — Splinter Review
that indeed fixes the issue
I think you need to check both source and value; otherwise somebody could override with -moz-border-start-color, etc.
(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.)
Though given the increase in length from adding 4 more properties to the code, we probably should convert to using an array now.
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
Attachment #305371 - Attachment is patch: true
Attachment #305230 - Attachment is obsolete: true
Attachment #305228 - Attachment is obsolete: true
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).
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Bernd, hope you don't mind me assigning this to you, since you're working on it.
Assignee: nobody → bernd_mozilla
Flags: wanted1.9+
Flags: blocking1.9-
> hope you don't mind me assigning this to you, since you're working on
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.
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 on attachment 305371 [details] [diff] [review]
patch without the array


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.
Attachment #305371 - Flags: superreview+
Attachment #305371 - Flags: review+
Attachment #305371 - Flags: approval1.9?
>Why don't we just get this one in

what a superb idea, its exactly what I wan't 
Comment on attachment 305371 [details] [diff] [review]
patch without the array

Attachment #305371 - Flags: approval1.9? → approval1.9+
I can land this for you (and will pretty soon) unless you prefer to land it yourself.
I filed bug 421197 as followup for the array.

Checked in to trunk, 2008-03-05 16:03 -0800.  Thanks for the patch.
Closed: 15 years ago
Flags: in-testsuite+
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
Flags: tracking1.9+
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.
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.
You need to log in before you can comment on or make changes to this bug.