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

VERIFIED FIXED in mozilla1.9beta5

Status

()

P2
normal
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: stevee, Assigned: bernd_mozilla)

Tracking

({regression, testcase})

Trunk
mozilla1.9beta5
regression, testcase
Points:
---
Bug Flags:
blocking1.9 -
wanted1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(7 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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?
(Reporter)

Comment 1

11 years ago
Created attachment 305177 [details]
Expected rendering (white borders around all cells)
(Reporter)

Comment 2

11 years ago
Created attachment 305178 [details]
Regressed rendering (vertical broders are black)
(Reporter)

Updated

11 years ago
Flags: blocking1.9?
(Assignee)

Comment 3

11 years ago
Can you provide a reduced test case? Bugs in the layout component should be filed with one.
(Reporter)

Comment 4

11 years ago
Created attachment 305202 [details]
testcase to use with str
(Reporter)

Updated

11 years ago
Keywords: testcase
(Assignee)

Comment 5

11 years ago
Created attachment 305212 [details]
screenshot inspector
(Assignee)

Comment 6

11 years ago
Created attachment 305214 [details]
screenshot inspector
(Assignee)

Comment 7

11 years ago
please notice on the first screen shot the strange property:
"border-right-color-value" instead of "border-right-color"
(Assignee)

Comment 8

11 years ago
they are not strange but introduced in bug 74880. So this is certainly a regression
(Assignee)

Comment 9

11 years ago
Created attachment 305223 [details]
same testcase with a div
(Assignee)

Comment 10

11 years ago
this is not a table but a general style resolution issue
Component: Layout: Tables → Layout
QA Contact: layout.tables → layout
(Assignee)

Updated

11 years ago
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.
(Assignee)

Comment 12

11 years ago
Created attachment 305227 [details] [diff] [review]
patch to make the mochitest to test all 4 edges
(Assignee)

Comment 13

11 years ago
Created attachment 305228 [details] [diff] [review]
patch to make the mochitest to test all 4 edges
Attachment #305227 - Attachment is obsolete: true
(Assignee)

Comment 16

11 years ago
Created attachment 305230 [details] [diff] [review]
do what David described first

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.
(Assignee)

Comment 20

11 years ago
Created attachment 305371 [details] [diff] [review]
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
(Assignee)

Updated

11 years ago
Attachment #305371 - Attachment is patch: true
(Assignee)

Updated

11 years ago
Attachment #305230 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
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-
(Assignee)

Comment 23

11 years ago
> 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?
(Assignee)

Comment 26

11 years ago
>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
Last Resolved: 11 years ago
Flags: in-testsuite+
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
Flags: tracking1.9+
(Assignee)

Comment 30

11 years ago
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.
(Reporter)

Comment 31

11 years ago
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.