Closed Bug 255411 Opened 20 years ago Closed 19 years ago

"Use my colors" preference makes popup/hover frames have transparent background

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: dharkness, Assigned: ginnchen+exoracle)

References

(
URL
)

Details

(Keywords: access)

Attachments

(1 file, 2 obsolete files)

patch v3
3.86 KB, patch
bzbarsky
: review+
dbaron
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040803
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040803

Color settings:

* Browser: green-on-black
* Windows XP Appearance: yellow-on-blue

When the "Use my chosen colors" preference is enabled, hover popups
(e.g. email address in Gmail while hovering over the sender's name) and dropdown
lists created using popups (e.g. "Move To..." dropdown on Yahoo Mail) are
created with a *transparent* background -- not the black background color I've
chosen. This makes them barely readable over the content they cover.

Reproducible: Always
Steps to Reproduce:
1. Set background color preference to some non-standard color (e.g. black).
2. Enable "Use my chosen colors" preference on same preference page.
3. View a site with a hover/popup (e.g. popup navigation menus).

Actual Results:  
The popup has a transparent background, making it difficult to read.

Expected Results:  
The popup has the background color selected in Preferences : Appearance :
Colors. No underlying content from the main page should show through.
The "use my colors" preference sets the root to your chosen background and
everything else to transparent.  The other option would be to set backgrounds on
everything in sight and have chunks of text disappear completely (because it's
fairly common for things with transparent background to cover text).

So the basic problem is that the browser has to guess whether something should
have a transparent background or not, and it has nothing to base that guess on.
Would the following work as a fix and workable solution in general? Instead of
setting the root to my color and everything else to transparant, simply replace
the background color/image with mine for all elements that actually specify one?

Given the wording of the preference, that's what I had assumed was happening and
that there was simply a bug when a hover/popup was involved.
*** Bug 250069 has been marked as a duplicate of this bug. ***
See bug 205892 and bug 247805
This has been in bugzilla for just under two years (205892).

>The "use my colors" preference sets the root to your chosen background 
>and everything else to transparent.

What is the normal behaviour?  The set-everything-else-to-transparent is at best
unneccessary, and seems to be very wrong in most cases.  

David's solution seems to be the way to go (at the very least, it's the
implementation suggested by the description in the user interface)
Make that 2 and a half (160099)

Can anybody point me to the relevant source files?
*** Bug 205892 has been marked as a duplicate of this bug. ***
*** Bug 247805 has been marked as a duplicate of this bug. ***
*** Bug 160099 has been marked as a duplicate of this bug. ***
*** Bug 243041 has been marked as a duplicate of this bug. ***
*** Bug 293740 has been marked as a duplicate of this bug. ***
(In reply to comment #1)
> So the basic problem is that the browser has to guess whether something should
> have a transparent background or not, and it has nothing to base that guess on.

I don't think the browser has to guess, a reasonable way is to switch any other bgcolor in the document 
to user chosen bgcolor, but keep transparent background in the document untouched.

For example, if user chooses "yellow"as bgcolor and browse a webpage like this,
<DIV bgcolor="white"> Text ...
<DIV bgcolor="transparent"> Popup ...
Mozilla should displays like this,
<DIV bgcolor="yellow"> Text ...
<DIV bgcolor="transparent"> Popup ...
But now with this bug, it's like,
<HTML><BODY bgcolor="yellow"> ...
<DIV bgcolor="transparent"> Text ...
<DIV bgcolor="transparent"> Popup ...
It's no good.

IE doesn't have the same problem.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: access
OS: Windows XP → All
Hardware: PC → All
cc:bz to let him see the last comment
The point is if we implement this via CSS rules we have to guess.  If we hack
style resolution directly to implement support for this pref, then I agree that
we can do it "right".
Any plan to do a hack in style resolution or do a better guess?
(In reply to comment #6)
> Make that 2 and a half (160099)
> 
> Can anybody point me to the relevant source files?

PrefShell::SetPrefColorRules()
> Any plan to do a hack in style resolution or do a better guess?

It seems ok to me if it can be done fast (possibly in SetColor and using a
cached pref on the prescontext?)
Er, ignore what I said about SetColor.  Just using a fast accessor on
prescontext and in the style computation for background color, I guess.
Attached patch draft patch (obsolete) — — Splinter Review 
a draft patch
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
Attachment #183896 - Flags: review?(bzbarsky)
(In reply to comment #19)
> Created an attachment (id=183896) [edit]
> draft patch
> 
> a draft patch
a side-effect:
DOM Inspector can't return correct value of the background-color.
Comment on attachment 183896 [details] [diff] [review]
draft patch

This is wrong; think about background images.

I'm also not clear about why XUL is special here and I'm not too happy with
having the extra work on every single paint.
Attachment #183896 - Flags: review?(bzbarsky) → review-
(In reply to comment #21)
> (From update of attachment 183896 [details] [diff] [review] [edit])
> This is wrong; think about background images.
You're right. I should add "background-image: none !important;".

> I'm also not clear about why XUL is special here and I'm not too happy with
> having the extra work on every single paint.
Because the Fx UI colors should not change due to user chosen "Content->Colors".
There're quite a few UI elements are using hardcoded colors in css files.

Can you suggest another place to do this work?
I think the patch runs fast enough.

> Because the Fx UI colors should not change due to user chosen
> "Content->Colors".

What about HTML files in the UI (like the help, say)?

What about XUL that's served remotely and isn't part of the UI?

Please get out of the "UI is only XUL and XUL is only UI" mindset.

>  Can you suggest another place to do this work?

I already did; please read my comments in this bug (eg comment 14).

> I think the patch runs fast enough.

What performance testing have you done?
(In reply to comment #23)
> What about HTML files in the UI (like the help, say)?
> What about XUL that's served remotely and isn't part of the UI?
> Please get out of the "UI is only XUL and XUL is only UI" mindset.
I just want to keep the same behavior with XUL and Help before this patch.
I don't want URL box uses user chosen background color.
And the help file is already using user chosen background color before this patch.
Actually  the CSS lines in PrefShell::SetPrefColorRules() only work with HTML, right?
So this hacking should only works with HTML, too.

> I already did; please read my comments in this bug (eg comment 14).
But I can't find a way to seperate HTML and XUL there.

> What performance testing have you done?
I can make a pageload test after my lunch.

> I just want to keep the same behavior with XUL and Help before this patch.

Even if it's wrong?

> Actually the CSS lines in PrefShell::SetPrefColorRules() only work with HTML,
> right?

As written, yes.

> But I can't find a way to seperate HTML and XUL there.

But you could separate content and chrome....

> I can make a pageload test after my lunch.

Actually, dhtml tests would be more useful there.

In any case, the point is that this isn't something we should be doing every
single time we paint.  Style resolution is the right time to do it.
(In reply to comment #25)
> > Actually the CSS lines in PrefShell::SetPrefColorRules() only work with HTML,
> > right?
> As written, yes.
So the patch's effect need to be narrowed to HTML as same as PrefColorRules.
Otherwise we need an other implementation for both fg color and bg color.
Attached patch draft patch v2 (obsolete) — — Splinter Review 
another draft patch
A small concern, nsPresShell is using namespace to narrow the effects,
nsRuleNode is using IsChrome().
There may be some inconsistence with remote XUL.
Anyway, it's not a big matter to me.
Attachment #183994 - Flags: review?(bzbarsky)
The last comment and the patch is on behalf of me.
I forgot to change the login of this machine. :)
> A small concern, nsPresShell is using namespace to narrow the effects,
> nsRuleNode is using IsChrome().

Actaully, nsPresShell bails out if the docshell is of chrome type and never even
creates the preference stylesheet.  So it looks to me like the narrowing by
namespace for this rule is wrong, as I said....

But anyway, I assume you tested this patch on both chrome and non-chrome, HTML
and not HTML?

Also, please add spaces after the "//" in your comments.
(In reply to comment #29)
> Actaully, nsPresShell bails out if the docshell is of chrome type and never even
> creates the preference stylesheet.  So it looks to me like the narrowing by
> namespace for this rule is wrong, as I said....
I tried to remove the namespace setting in PrefShell::CreatePreferenceStyleSheet().
The result is OK and even better.
The test case is,
http://www.infodraft.com/~faser/mab/content/mab.xul
The test machine is Linux using GTK2.

But I the namespace is using by SetPrefNoScriptRule() and a lot other rules.
I don't know how to deal with them.


> I tried to remove the namespace setting in 
> PrefShell::CreatePreferenceStyleSheet().

Leave the namespace setting; just replace the "*" in the selector you want not
namespaced with "*|*".
Attached patch patch v3 — — Splinter Review 
Yes, *|* works. Thanks, bz.
Attachment #183896 - Attachment is obsolete: true
Attachment #183994 - Attachment is obsolete: true
Attachment #184066 - Flags: review?(bzbarsky)
Attachment #183994 - Flags: review?(bzbarsky)
Attachment #184066 - Flags: superreview?(roc)
bz, do you have further thinking about patch v3?
I haven't had a chance to look at it, and may not for another week or so.
Attachment #184066 - Flags: superreview?(roc) → superreview?(bryner)
Comment on attachment 184066 [details] [diff] [review]
patch v3

Looks reasonable to me... David, is this OK with you?  Or will this cause
issues with dynamic changes to that preference?
Attachment #184066 - Flags: superreview?(dbaron)
Attachment #184066 - Flags: superreview?(bryner)
Attachment #184066 - Flags: review?(bzbarsky)
Attachment #184066 - Flags: review+
*** Bug 300240 has been marked as a duplicate of this bug. ***
After applying patch v3, the background color of check boxes and radio buttons
is now the user-selected background color (instead of white), and, since the
selection glyph is always black, the state of these controls is now very
difficult to determine visually.  I think this is a bug.

One possible solution would be to color the selection glyph with the
user-specified foreground color.  Another possible solution would be to ensure
that the background of these controls remains white (the current behavior
without the patch).
(In reply to comment #37)
> One possible solution would be to color the selection glyph with the
> user-specified foreground color.  Another possible solution would be to ensure
> that the background of these controls remains white (the current behavior
> without the patch).

I think the best solution is to implement native checkbox/radio button support, as we did on Windows.
To color the selection glyph with the user-specified foreground color is acceptable and easier.

Is the issue the question of which of two UA-!important rules wins in the
cascade?  If so, shouldn't the order be fixed?
When will the patch be merged? =]
Comment on attachment 184066 [details] [diff] [review]
patch v3

I suppose we can try this.  The dynamic changes should work given the pref
stylesheet rules will be changing; that's not permanent if we get rid of that
bit of the pref stylesheet entirely, though.  In any case, this will fix some
things and break others, which is why I've been hesitant about it, but I
realized today that the ones it will break are ones that are (and probably
should be) already broken for event handling (bug 102695), so it's a relatively
small set.
Attachment #184066 - Flags: superreview?(dbaron) → superreview+
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.873; previous revision: 3.872
done
Checking in layout/style/nsRuleNode.cpp;
/cvsroot/mozilla/layout/style/nsRuleNode.cpp,v  <--  nsRuleNode.cpp
new revision: 1.162; previous revision: 1.161
done

David Hilvert, if you can reproduce the issue you mentioned in comment #37,
please file a new bug and attach a testcase, I'm very glad to fix it.
But I can't reproduce it with seamonkey trunk now.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
what is the state of this bug? ... in which version of firefox can we expect it to appear?
W.r.t comment #43, status says 'resolved fixed', and the transparency problem is indeed fixed in current versions of Firefox.  I'm still (see comment #37) seeing black (as opposed to custom-color) glyphs against custom backgrounds in checkboxes and radio buttons in the browser version I'm using (Firefox 1.6a1 alpha 2), making it very difficult to distinguish the glyph when the custom background is dark, but I'm too lazy at the moment to check whether this occurs in whatever version is most current.  Perhaps someone else could do this.
The issues that this patch caused should be fixed in the patch I'm going to attach to bug 58048.  (I was going to add it to bug 348637, but then I noticed it was already fixed.)
Additional problems fixed by checkin of bug 58048.
Flags: in-testsuite?
Please reactivate this bug. Although resolved fixed in 2005, the current version of SeaMonkey 1.1.12, on Win32, Linux x86 and x86_64, is still having this problem. All DHTML menus on the web, such as the "more" dropdown on the Google main page, still show up without a background if "only my colors" option is checked. Since the menu text is thus superimposed over the underlying page text, the menus are totally unusable in some circumstances.
Mike, this bug wasn't fixed until September 23 2005.  The version of Seamonkey you're using uses a Gecko from early August 2005, with security fixes added.

Once there is a shipping Seamonkey based on Gecko 1.9 you will get this fix in Seamonkey.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: