Closed Bug 40340 Opened 24 years ago Closed 24 years ago

color / font preferences not being displayed / honored

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: bugzilla, Assigned: attinasi)

References

Details

(Keywords: access, polish, Whiteboard: [nsbeta3-][rtm++])

Attachments

(6 files)

Patch to fix pref problems with: bgcolor, text color, link color, link underline and preferred fonts
26.83 KB, patch
Details | Diff | Splinter Review
Patch to disable color prefs. for Composer
412 bytes, patch
Details | Diff | Splinter Review
Additional Patch to make preference-change registrations more friendly
5.49 KB, patch
Details | Diff | Splinter Review
Testcase used in testing the patches...
3.54 KB, text/html
Details
Patch to fix the loss of monospace fonts for -moz-fixed when ignoring document fonts
2.85 KB, patch
Details | Diff | Splinter Review
Patch: all current, reviewed and approved changes, prepared against the branch.
39.04 KB, patch
Details | Diff | Splinter Review 
spun off from my 2000-05-23 13:39 comments in bug 21160.

tested using opt comm bits 2000.05.22.08 on linux and winnt. not currently
testable on mac since color prefs aren't being written at all to prefs.js (bug
21160 reopened).

to reproduce:
1. open Preferences dialog, select Colors category.
2. select a different color from the four color pickers: background, text
(foreground_color), links (anchor_color) and visited links (visited_color).
3. select "Use my chosen colors" to insure that the colors will be displayed.
4. click OK to save changes and dismiss the dialog.

expected: the selected colors for background, text, links and visited links
should change to what was selected in the prefs.

results:

a. linux: only background_color and foreground_color are actually displayed (and
for the most part only in the chrome, but that's covered by bug 22953).
visited_color and anchor_color, while also written to prefs.js, is not displayed
at all (even after restarting the browser *and* use_document_colors set to
false).

b. winnt: even though anchor_color, visited_color, background_color and
foreground_color are written to prefs.js, they're never displayed even after
restarting the browser *and* use_document_colors set to false.
Triage duty calls. Reassinging to waterson.
Assignee: clayton → waterson
Status: NEW → ASSIGNED
Target Milestone: --- → M18
Nom. nsbeta3, recc. nsbeta3+. We're failing to support coloring options that are 
part of our own prefs UI.
Keywords: nsbeta3, polish
Blocks: 44122
[NEED INFO]. waterson will investigate & see which is harder: fixing this or
removing those prefs from the UI.
Keywords: helpwanted
Whiteboard: [NEED INFO]
Target Milestone: M18 → Future
Something is weird here. On Winnt build, 20000814, this sort of works. Did
someone fix it and not update the bug? Step to reproduce:In prefs, set
background color to red, foreground color to white. Uncheck "use windows" and
select "use my colors". Now, go to http://www.yahoo.com and the background is
red and the text is white, as it should be. However, notice the status bar (in
Modern Skin) is white but even more weird is if you go back to prefs, notice all
the pref items on the left are also white (on a white background). I don't think
that should be happening. I just tried it with PR2 on Linux and same results as NT. 
peter, sounds like you're seeing bug 22963.
Whiteboard: [NEED INFO] → [nsbeta3-]
David, another one of these for you.
Chris, I am working on the fix for this (under other bug numbers: bugs 55262, 
bug 22963, bug 31816 and bug 20760) so I'll take it - hope you don't mind.
Assignee: waterson → attinasi
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Priority: P3 → P2
Target Milestone: Future → M19
*** Bug 55262 has been marked as a duplicate of this bug. ***
Adding rtm+, CCing don. This bug will cover some other important bugs involving 
preferences.
Keywords: rtm
Whiteboard: [nsbeta3-] → [nsbeta3-][rtm+]
This is a general bug covering the problems in bug 31816, bug 20760 and bug 22963.

I have received reviews and approval: r=karnaze, a=buster
and a verbal OK from Kathy Brade for the one-line Composer change r=brade

I await an PDT approval now.
Blocks: 20760
Blocks: 22963
Blocks: 31816
Minor tweaking of summary since this is now covering the remaining visual pref.
problems (colors, links, underlines, and fonts).
Summary: color preferences not being displayed → color / font preferences not being displayed / honored
PDT, this is THE bug that we need fixed or we'll have to substantially disable
preferences.  Please, please approve.

Please test this out with AIM, land on the trunk and let it cook for a day, and
then given that everything is copacetic, please come back to PDT for final ++.
Marking need info.
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm++]
cc'ing suzanne so that she can check this out on aim (trunk).
I've completed testing with AIM. Basically , it has no affect on AIM. I did find
another problem, not related to my changes, but related to the prefs. If you
select as your preference a background color of black and a text color of white,
then try to send an instant message, you cannot see the text you type. This is
because the IM compose window hard-codes the initial background color to white,
but uses the user's pref for the text color, yielding white on white. I showed
Syd and he agreed that it is bad.

I'm incorporating a small change that Pierre suggested to preference change
registration, then I'll be ready to push to the trunk.

(updating to [rtm need info] as that was the intention of Michael yesterday)
Whiteboard: [nsbeta3-][rtm++] → [nsbeta3-][rtm need info]
AIM update: AIM _is_ affected. When IM'ing, the upper dislog pane is applying
the preferred colors, losing the colors that AIM normally adds to the screen
name and the colors that the users may have set.

On initial consideration, Syd thinks that we might want to disable the pref
stylesheet for AIM liek we do for composer. Syd, Vishy and I are hashing this
out now.

There is an additional, non-AIM problem that I just found.

Some sidebar panels, like the Search panel, have HTML controls in them. Those
HTML controls are getting the users color preferences applied to them. It looks
a little strange in that the vast majority of the contents in the side panel are
NOT colored according to the user's prefs, but the very few HTML controls are,
so it looks inconsistent.

I'm holding off on checking in until the AIM and sidebar issues are resolved or
accepted. Will need to discuss with PDT.

cc'ing shrirang since he tests the sidebar.
I'm curious about a few things in the patch (I read it quickly, so I may have 
misunderstood some things):

In |SetPrefColorRules|, it looks like you do nothing when the user chooses to 
use colors from the document.  Shouldn't you be creating a non-important rule 
setting the colors on the root element (a totally different rule)?  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 |SetPrefLinkRules|, you don't seem to test UseDocumentColors at all.  
Shouldn't it trigger simple "!important" switching there?

And does the "Always use my colors" option work correctly in the presence of 
author-!important rules given that !important in user stylesheets doesn't work 
right (bug 43220)?

Your comment about the order of calling |SetPrefLinkRules| and 
|SetPrefColorRules| seems strange given that they use |InsertRule(...,0,...)|.  
Doesn't that insert at the beginning?

The comments about future work on backstop and override sheets seem weird to me 
-- shouldn't we just fix the cascade and stop messing with this distinction?
Thanks for looking at the patch, David. Answers to your questions:

Q)In |SetPrefColorRules|, it looks like you do nothing when the user chooses to
use colors from the document.  Shouldn't you be creating a non-important rule
setting the colors on the root element (a totally different rule)?  And
shouldn't the existing rule set the background on the root element to the pref
and set all the other backgrounds to transparent?

A) When the user chooses to allow colors from the document there are no rules
necessary, since we are already handling the default colors elsewhere in the
code. In other words, it works fine when we *are* using document colors, and the
change only addresses the need to enfore colors when the user chooses *not* to
use document colors. In an ideal world, we would do as you suggest and use
preference style rules for applying the default colors as well, but currently
that is applied elsewhere and I don't want to change more than I have to at this
time.

Q)In |SetPrefLinkRules|, you don't seem to test UseDocumentColors at all.
Shouldn't it trigger simple "!important" switching there?

A) Setting the link colors and link underlining is not based on the users
setting for UseDocumentColors. They are independent settings. Link colors are
always the default link colors, and are never forced. In other words, there is
no option to 'always use my link colors, ignoring the link colors in the
document' like there is with background color and text color. That is why there
is no check for UseDocumentColors in SetPrefLinkRules.

Q) And does the "Always use my colors" option work correctly in the presence of
author-!important rules given that !important in user stylesheets doesn't work
right (bug 43220)?

A) No. That case is shown to be a problem in the testcase I attached - whoops,
the testcase is not attached here, it went out with an email to the reviewers
only. I'll attach the testcase. Anyway, it is true that author !important rules
WILL override the text and background colors even if the user has chosen to
'always use my colors'.

Q) Your comment about the order of calling |SetPrefLinkRules| and
|SetPrefColorRules| seems strange given that they use |InsertRule(...,0,...)|.
Doesn't that insert at the beginning?

A) Since both rules are prepended to the stylesheet, the one that is prepended
last will come first, and since the rule for the text color will cascade over
the rule for the link colors, the link color rules need to be prepended last.

Q) The comments about future work on backstop and override sheets seem weird to
me -- shouldn't we just fix the cascade and stop messing with this distinction?

A) The issue is that the text and background colors need to be able to override
the document (when the option is selected). The link colors are default values
only, they can always be overriden by the document. Therefore, even if the
cascade was correct, we should put the text and background rules in the override
stylesheet and the link rules in the backstop stylesheet. The link rules
currently are in the backstop stylesheet (html.css) actually, however we need to
be able to alter the color value according to the pref. instead of hard-wiring
'blue' and 'purple' in html.css. That is why I commented that we really need
both an override and a backstop stylesheet. If I am mistaken, and with a fixed
cascade we can implement the invariant text and background colors using
!important in the backstop stylesheet then I should update that, but from my
understanding they would still be overridden by author rules. Ideally the 'force
my colors' rules would be in an override sheet, as important rules, and the 'use
my link colors by default' rules would be in the backstop stylesheet, also as
important rules.

I hope this all makes sense. Let me know if you need more explanation or if you
disagree.

Fix checked into the trunk. Will check into the branch if the trunk is OK:
marking rtm+ to get PDT to look at it and make the call for ++

These changes were reviewed and approved: r=karnaze,pierre a=buster. I believe
the notations are in the blocked bugs (a bit of bug number juggling has taken
place here, but the reviews and approvals were definitely above-board).
Whiteboard: [nsbeta3-][rtm need info] → [nsbeta3-][rtm+]
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm need info]
I see these changes were checked in to the trunk very early this morning.  It
seems they have caused some significant regressions in the 2000-10-11-06 win32
build.  Any page that specifies link colors in CSS or HTML has those link colors
ignored, for example:

http://www.people.fas.harvard.edu/~dbaron/
http://www.operasoftware.com/people/howcome/
http://style.metrius.com/
http://www.microsoft.com/
http://www.sun.com/
http://developer.gnome.org/
http://www.jwz.org/contact.html

In my comments yesterday I explained why this is happening and what should be
done to fix it.
Fixing collision.
Whiteboard: [nsbeta3-][rtm need info] → [nsbeta3-][rtm+]
Yup - checking it out now.

The regression is covered by bug 56098.  I'd rather keep the regression as a
separate bug because it will accumulate lots of dups, and I'd rather not have
them clutter up this bug.
We can't take this on the branch with a regression in it.  Please get the
regression worked out on the trunk and get David Baron to review it and come
back to us for re-approval by tomorrow.
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm need info]
In Build ID: 2000101108 / Linux i686
In a similar vein - I'm not sure if it was related, but thought I'd post it just
in case. I am seeing the opposite case. I have colors selectd but have the
'Always use the colors and background specified by the web page' selected.
Nevertheless, the colors I have selected override the link colors on all the
sites I've visited.
The regression, covered in bug 56098, has been fixed. I checked in the fix,
suggested by David Baron, yesterday afternoon.

If anybody has tested / worked with the feature on the trunk, please record any
comments, positive or negative, that might be important for the PDT decision to
accept or deny this on the branch.
This now appears to be fixed on NT under build 2000101212
--djo
this in from Asa --works fine using 2000.10.13.08 trunk bits on winNT and linux.
PDT - please approve or deny for the branch. The trunk is fine (see latest
comments).
Whiteboard: [nsbeta3-][rtm need info] → [nsbeta3-][rtm+]
Yes, PDT please approve this!!!

PDT waiting on review from erik@netscape.com, which was requested by ftang.
Marking [need info] -- erik please mark [rtm+] after your review.
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm need info]
Eric, the patch I just posted (10/14/00 21:36) is needed to fix a reported bug 
where tags like <PRE> and <TT> were getting variable fonts when the font 
override is enabled. I now check for -moz-fixed and set the 
NS_STYLE_FONT_USE_FIXED even if ignoring document fonts. The patch is against 
the changes checked into the trunk, which are pretty much the ones attached as 
(10/09/00 15:56) with a minor change for Composer to be able to turn font 
overrid off. YOu ca, of course, see the exact change in the trunk, or via LXR 
(nsHTMLFontElement.cpp and nsCSSStyleRule.cpp). Thanks for reviewing this.
I noticed that you have fixes for the unvisited and visited link colors, but not
for CSS2's :active and :hover. I assume that you don't need the latter 2 because
our UI doesn't have prefs for those? I just noticed that you do take care of
:active elsewhere in the code. I guess the prefs are for both active and
inactive links (both visited and unvisited), while there is no pref for :hover.

I believe that we normally recommend NS_SUCCEEDED instead of NS_OK ==. Is there
a special reason for using NS_OK ==?

PresShell::SetPreferenceStyleRules returns PR_TRUE at one point. It is an XPCOM
method, so it should return NS_OK or NS_ERROR_FAILURE or whatever.

Why are you caching the bool prefs in the pres context? Do you have any evidence
that the prefs manager is not caching them?

In one of the changes, you have a comment with a question about how to turn it
off for composer, but you seem to have found a way to do that in the editor, so
maybe you should remove that comment.

Did you resolve the AIM issues?

What about sidebar?
Erik has reviewed the changes and requested a couple of minor changes that I
have made (see his comments). To summarize, I have changed several instances of
checking against NS_OK to using NS_SUCCEEDED, and also removed the :active parts
of the link color rules so that :active links and :active visited links will be
red (due to html.css rule).

From Erik's email:

> Thanks for the catches! Do you want new patches with the above items
> addressed? I'll update the bug with this information once we've resolved it.

No, I don't need to see new patches. You have my r=erik. I do hope
however, that people have *really* been testing the trunk builds (since
I don't think that a code review alone cuts it in this case).

Erik

I'm marking RTM+ to trigger PDT action.
Whiteboard: [nsbeta3-][rtm need info] → [nsbeta3-][rtm+]
Looks like the PDT didn't make a call on this one last night. If this doesn't
get in soon (like today) I'm betting they won't take it at all...

PDT: please approve or deny. 
PDT says- Need info: Please attach latest version of patch, get that reviewed,
and characterize testing done on trunk using that version of the patch. Please
also include patches to fix exposed regressions, such as bug 56098.
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm need info]
PDT has second thoughts: Not critical for rtm, not worth any more effort, rtm-
Whiteboard: [nsbeta3-][rtm need info] → [nsbeta3-][rtm-]
I have attached the final patch. All of the code has been reviewed multiple
times, and approved. I have checked most of it into the trunk, except the fix
that is also attached as a patch to bug 56194. If somebody else wants to do
battle to get this approved, have at it.


Marking this PDT+ again.

PDT folks: Look, if we don't fix this we're gonna have to make substantial
changes to the prefs or risk looking like idiots with non-functioning dialogs.
Do you realize what the side effects of not taking this change are?
Whiteboard: [nsbeta3-][rtm-] → [nsbeta3-][rtm+]
Come to the Pit at 4pm to discuss
Blocks compliance with WAI User Agent Accessibility Guidelines (bug 24413) -- 
checkpoints 4.1, 4.2, 4.3, and 4.4.
Blocks: uaag
> A) Setting the link colors and link underlining is not based on the users
> setting for UseDocumentColors

I disagree. Filed bug 57220 for that discussion.
Ben, that was fixed a while ago. See comments in bug 57220, and note that the 
latest attached patch has the correct style rule for the link colors, including 
when document colors are overridden. That was fixed the day after the initial 
landing to the trunk, actually.
Phil Peterson wrote:
> Come to the Pit at 4pm to discuss

Has someone been there?
Andreas, yes, we've been through this, and the stumbling block is that we're
scared of taking a huge change in core layout at this late date. Some think the
risk is too much, and some really want the feature, and are willing to take the
risk. I think the missing piece is evidence that the patch really works and
doesn't regress anything.
Ok. About the risk: I'm not competent to judge the risk, but if it's really been
in the trunk for a week and there are no bugzilla reports filed after that date
that could be caused by it, then my feeling would be that the risk is tolerable.
(I can't help with looking through the bugzilla bugs because I wouldn't be able
to distinguish between unrelated and possibly related problems, so someone with
more knowledge about layout should do it.)

On the other hand, if chances that this helps are significantly greater than
zero, I could spend up to about two hours now to verify that the patch fixes the
things it is supposed to fix, if it's sufficient to use a self-made build from
the 10/13 trunk sources on linux (or any other available binaries), and someone
points me to descriptions of the things I should test. If you want that, please
contact me now. Marc?

Andreas, I'll email you offline regarding your offer.

I think a little summary is in order here:

Regarding the risk assesment: the problems that have been reported with this
patch only show up when the option to 'always use my colors' or 'always use my
fonts' are selected. And even then, they are minor. To quote Ben Bucksch, who
has tested it on the trunk a bit:

***
But if you ask me: "Would you prefer a product with these known bugs in the
implementation or no implementation at all", I'd say "Give me the buggy
implementation - I can always disable it, if I'm sick of it."

If I were you, I'd tell PDT this reasoning (last paragraph) and the bugs and let
them decide.
***

The bugs Ben opened are bug 57234, bug 57240 and 57316.

The patch has been baking on the trunk for a week now, and aside from the minor
bugs reported when 'use only my colors' and 'use only my fonts' are selected,
there appears to be no significant problems. In the default case, where the user
has not changed any of the prefs, there are no known problems.

Without the patch most of the colors pref panel has to go away (link colors,
link underline, and use document fonts radios).

Finally, this has been well reviewed and approved:
r=karnaze,erik,pierre,dbaron
a=buster

rtm++.  Here's the deal we made: checkin ASAP so tomorrow's build has this.  If 
there are any regressions, it must be backed out immediately so we don't have a 
broken build over the weekend.
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm++]
Files checked into branch:
mozilla/layout/base/public/nsIPresContext.h
mozilla/layout/base/public/nsIPresShell.h
mozilla/layout/base/src/nsPresContext.cpp
mozilla/layout/base/src/nsPresContext.h
mozilla/layout/html/base/src/nsPresShell.cpp
mozilla/layout/html/content/src/nsHTMLFontElement.cpp
mozilla/layout/html/document/src/html.css
mozilla/layout/html/style/src/nsCSSStyleRule.cpp

Marking FIXED
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Still need to checkin nsEditorShell.cpp to trunk...
nsEditorShell.cpp checked into branch now.
Verified in both Mac and Windows Oct 20th branch builds.
Need to check on linux build. Waiting for sweetlou to come up.
Adding vtrunk so this gets verified properly on the trunk.
Keywords: vtrunk
petersen --this works fine using 2000.10.30.09-n6 [opt comm branch bits] on
linux [redhat 6.2]!
Was checkin made into the trunk?

I'm verifying this bug on nov_3  trunk builds and for now noticed the following:

- visited link does not change color on Win NT, while it   does change on Mac
  and Linux ( try http://my.yahoo.com or test case attached to this bug)

-  on some pages, like http://my.yahoo.com ,
   with pref set to "Always use the colors and background specified by the web
   page" (which in my understanding means: do not overwrite any color prefs
   specified by the web page), link colors get overwritten by user's prefs. (all
  platforms)

 It's even worse for the attached test case: when I change the text/bckgrd
colors, set  "use my colors", then go back to "use web page's colors", even
after restarting of N6 I still get "my colors". This is for Win NT and behavior
is the same with oct_16 trunk build.

Am I missing something?
There are problems with visited links not showing up as visited regardless of
the pref. These are related to issues with how the session history stores the
URL's and are covered in bug 12493 and the many dups thereof.

Pages that do not specify their own colors, like www.yahoo.com, will always use
the default colors set in the user's preferences, even if 'Always use the colors
and background specified by the web page' is selected. We are not, in that case,
overriding anything, we are just applying the user's choice of default colors.
In my experience, very few pages leave the colors unspecified, and yahoo is one
of them. The testcase also has no specified background color on the body, so you
should get your preferred colors on the background no matter how you have set
the 'use my colors / use web page colors' option.

verified with 122705 mozilla win32 build on NT and 122710 mozilla build on
RedHat6.2.  Setting status to VERIFIED and removing the vtrunk keyword.

Background, Text, link and visited link colors behaved as expected in both use
page's colors and use my colors.
Status: RESOLVED → VERIFIED
Keywords: vtrunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: