Closed Bug 419167 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta5

People

(Reporter: stevee, Assigned: bernd_mozilla)

References

()

Details

(Keywords: regression, testcase)

Attachments

(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 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?
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 http://mxr.mozilla.org/mozilla/source/layout/style/nsCSSProps.cpp#1296
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
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.
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

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

a1.9=beltzner
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.
Status: NEW → RESOLVED
Closed: 12 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.
--> VERIFIED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.