57234 - a -> font leads to wrong color, if "always use my color"=true

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Ben Bucksch (:BenB)]

If a <font> element (no matter if it has attributes) is a child of a :link and
"always use my colors" pref is sat, the foreground color, not link color, is
used, i.e. layout "forgets" that this is a link.

Reproduce:
1. Set color prefs to unusual values, but observe document colors
2. Open testcase
3. Set "always use my colors" pref to true
4. Check testcase again

Actual result:
After step 2: Everything is as expected.
After step 4: All text but "Music3" has your foreground color (not link color).
("Music3" does have your link color.)

Expected result:
After step 4: Music1,2,3 have your link color, Music4,5 have your foreground
color.

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Reduced testcase

Comment 2

[Marc Attinasi]

I'll check it out.

Comment 3

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

This would be fixed by my suggestion on bug 40340:

> And
> shouldn't the existing rule set the background on the root element to the pref
> and set all the other backgrounds to transparent?

In other words, the rules you want to create should look like:

* { color: <preferred color> ! important; background: transparent ! important; }
:root { background: <preferred background> ! important; }
:link { color: <preferred link color> ! important; }
:visited { color: <preferred visited color> ! important; }

Comment 4

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

Er, ignore my previous comment.  The problem is you need:

:link, :link *
:visited, :visited *

as the selectors.  As painful (for performance) as that may be...

Comment 5

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

No, now that I'm awake...

There's a better way, which is to do what I described in my first comment,
except for color.  Set the default on the root, and use 'inherit'.  Doing the
analogous thing for background (default/'transparent') might be good too, but
I'm not sure how form controls will react.

Comment 6

[Marc Attinasi]

I think I see what you are saying. So if the parts in [] are only for when the 
'ignore document colors' is set:

[:root { color: <pref> !important; background: <pref> !important; }]
[* { color: inherit !important; background: transparent !important; }]
:link { color: <pref> [!important]; }
:visited { color: <pref> [!important]; }
:link, :visited { text-decoration: <pref> !important; }

So the root element gets the colors set on it, and everything else inherits the 
foreground color, and uses a transparent background. This means background 
images are lost too...

Links and visited links get their foreground color set, but still use the 
transparent background color. And of course, links are underlined or not 
according to the pref.

I'm trying it now. Thanks.

Comment 7

[Marc Attinasi]

Yes, those rules make it much better. This bug, along with bug 57316 and bug 
57240 appear fixed by the patch also. I'll attach a patch showing the changes.

Comment 8

[Marc Attinasi]

Attachment: Patch: set pref-colors on :root, make everything else inherit foreground color, transparent background color

Comment 9

[Marc Attinasi]

Setting dependencies on related bugs also fixed by patch: not really dependent 
but related.

Comment 10

[Ben Bucksch (:BenB)]

> So if the parts in [] are only for when the 'ignore document colors' is set:
> [:root { color: <pref> !important; background: <pref> !important; }]

Did you really mean that? If you do that, the document color will be ignored
completely all the time, if "always use my color" is false, not? Don't you mean

:root { color: <pref> [!important]; background: <pref> [!important;] }

?

Comment 11

[Ben Bucksch (:BenB)]

The patch fixes for me the problem originally reported, and the realworld
testcase through which I found the bug. Same for bug 57240 and bug 57316.

The reason why the patch works though no :root color is sat (see last comment)
is that IIRC we set also this pref at another place already. attinasi, what
about removing this other place and just using what I suggested for the trunk?
This would reduce redundancy.

Comment 12

[Marc Attinasi]

The reason the patch works when the :root {...} is not set is that the default 
colors are applied without style rules. When no style rules specifying a color 
are found, the style context gets the initial colors from the pref.

http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsStyleContext.cpp#242

That is where the styleColor structures is initialized and it gets the default 
colors from the presContext and directly sets them into the style context data.

I realy don't want to change this behavior now, since it has been that way for a 
long time, and it works, and I don't know the risks of changing it. It is 
something to think about, though...

Comment 13

[Ben Bucksch (:BenB)]

You are the master... :)

Comment 14

[Marc Attinasi]

Master of EViL, thank you ;)

Comment 15

[Hixie (not reading bugmail)]

Marc: actually, according to Pierre (ccing), that is _not_ where the colour is
actually set. It turns out we do some voodoo that nobody has been able to
explain yet. This would be an even better reason not to touch it for now...

Comment 16

[Marc Attinasi]

Wrong time to be messin' with voodoo, I agree. 

I wonder if it has something to do with the 
HTMLStyleSheet::SetDocumentBackgroundColor and 
HTMLStyleSheet::SetDocumentForegroundColor methods 
(http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsHTMLStyleSheet.
cpp#1199)?

Comment 17

[Pierre Saslawsky]

See my comments from [2000-10-02 16:03] in bug 19329 about the document 
background color.

Comment 18

[Marc Attinasi]

Can I get a review or two on this patch for a landing on the trunk? It makes it
much nicer. Thanks!

Comment 19

[Ben Bucksch (:BenB)]

Marc, I can't review (nlot qualified), but, as I said, the patch works fine for
me, and fixes all the bugs I filed.

Comment 20

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

r=dbaron

Comment 21

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

... assuming you've tested that form controls, frames, and scrollbars are happy.

Comment 22

[Marc Attinasi]

Now that you mention it, David, I did have to modify the rules for form controls
:-) The little down-arrow on the select was getting obliterated by the
'background: transparent' rule, so I changed it to 'background-color:
transparent'. This way, background *images* on elements will be preserved while
the background color will be transparent. I think it is better to leave the
background images alone, do you agree?

Comment 23

[Marc Attinasi]

Attachment: Updated patch: descendants of root element inherit background-color instead of background (to preserve their background images)

Comment 24

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

But that leaves authors' background images lying around -- probably exactly what
people using this pref would want to get rid of.  It could easily make a page
illegible (since you're forcing the color).

I think the real solution is in the select -- perhaps the select's arrow
background-image needs to be !important?  (And perhaps we need to make
UA-stylesheet !important the highest -- to be used sparingly by us only.)

Comment 25

[Marc Attinasi]

I'm not so sure I agree that are better off clearing all of the background images...

Clearly there are some instances where the background-images will make things
look terrible, possibly unusable by some people. But then again, forcing a
user's preferred colors should not eliminate content, and some background images
are used to show content (I'm thinking of cases where a background image is
indicating that something is a draft, or an example, or an uncontrolled
document). I guess authors could make the background-images !important if they
wanted to ensure that they were seen, but how many really will?

I agree that making the select background-image rule important would be
sufficient if we are willing to swallow the possible content-loss concern. I'll
see if Rod S. has any feelings on that - maybe some authors want to change the
background-image on the select's button???

Comment 26

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

Authors shouldn't use background images for content.

Comment 27

[Matthew T (active 1999-2002)]

Presentational style shouldn't be used for content, and that includes background 
images. 4.x doesn't show background images when `always use my colors' is on (and 
nor does it show them when the `Show Images' command is selected). I would 
imagine that a considerable proportion of those people who have `always use my 
colors' choose white on black, for example, and that would be disastrous if a 
white or nearly-white background image was loaded.

If it helps, you could change the rendering of SELECT widgets so that they are
a single button (like they are in Mac OS, and GTK, and Motif, and OS/2, and
MUI ...), rather than having two sections with different appearance but the same 
behavior (like they are in Windows).

Comment 28

[Ben Bucksch (:BenB)]

I agree with dbaron and mpt. CSS is not content, by definition. If a webpage
uses CSS for content, this is a bug in the webpage.

Comment 29

[Marc Attinasi]

Great input, thanks. In addition to the *original* patch (11/19/00), I need to
change html.css to make the arrows on select buttons stick:

select > input[type="button"] {
  position: static ! important;
  white-space: nowrap;
- background-image: url("arrow.gif");
- background-repeat: no-repeat;
- background-position: center;
+ background-image: url("arrow.gif") !important;
+ background-repeat: no-repeat !important;
+ background-position: center !important;
  width: 12px;
  height: 12px;
  -moz-user-focus: none;
}
select > input[type="button"]:active {
- background-image: url("arrowd.gif");
+ background-image: url("arrowd.gif") !important;
}

(note: the second rule for the active button never seems to get applied for some
reason, but I want to make the rule correct anyway in case is does start to apply).

I'll need approval from Rod S. for the html.css changes, and David, your review
would be most welcome too.

Scanning html.css for other background rules, I see that there are several form
controls that set their background-color to special values (ThreeDHighlight,
WindowColor, or -moz-field, for example) and these will be overridden by the
preference colors too. These include input, textarea, select, radio, checkbox,
and buttons. Currently, these all get the background color set in the user's
preference. If this behavior is contentious, we will need to open up another
bug. I don't think we really want to make those all !important, as clearly we
want to allow authors to change the background color of a textarea, for example.

Comment 30

[Hixie (not reading bugmail)]

(dbaron: ua.css ! important overrides everything already IIRC.)

Comment 31

[Matthew T (active 1999-2002)]

Filed bug 58048 about form controls losing their color.

Comment 32

[Marc Attinasi]

Rod's input vis a vis making the select button's image ruls important:

Rod Spears wrote:
I think it should be important, if they mess with it, it is non-standard. 

Comment 33

[Ben Bucksch (:BenB)]

Did the patch go into the branch yet (with the original patch that coused the
regression)? I assume no. 

Comment 34

[buster]

sr=buster

Comment 35

[Marc Attinasi]

This patch has not gone anywhere yet. I see Steve just sr'd it, and dbaron r'd
it and rod OK'd the html.css change for the select button, so I guess it is
ready to rock 'n roll (on the trunk anyway).

Comment 36

[Marc Attinasi]

Fix checked into trunk.
(nsPResShell.cpp, html.css)

Comment 37

[Chris Petersen]

Marking verified in the May 30th build.