Closed
Bug 472410
Opened 16 years ago
Closed 16 years ago
URL bar highlight mistakenly switches colours on dark themes
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: ventnor.bugzilla, Assigned: ventnor.bugzilla)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
On dark themes I've noticed the colour of the selection background when highlighted is actually reversed. We do this to improve contrast in some situations, but it is not desired in this case. This is due to a CSS rule in autocomplete that sets background-color: transparent. This gets passed down to the textbox where our algorithm doesn't like the "luminous" value of a "transparent" colour. To fix this, we must halt this inheritance. I've added a dummy background-color that is ignored in favour of the -moz-appearance rule in the same clause, but it fixes the bug.
Attachment #355682 -
Flags: review?(rflint)
Updated•16 years ago
|
Attachment #355682 -
Flags: review?(rflint) → review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [needs landing]
Hmm, is this actually the right fix? It sounds like there are two issues with the selection background color code: -- it doesn't make much sense to be checking a background-color that is not actually used because -moz-appearance is overriding it -- it definitely doesn't make sense to be checking the luminance of rgba(0,0,0,0)
I suggest you modify nsCSSRendering::FindNonTransparentBackground so that it stops when it finds a non-transparent background color *or* -moz-appearance. It should return the nsStyleContext, not the nsStyleBackground, because callers will then need to check GetStyleDisplay()->mAppearance and do something based on that (exactly what will depend on the call site). For this call site the right thing to do is probably that if -moz-appearance is set, we should just not adjust any colors --- we don't know what the theme would draw so we just have to assume that chrome is doing things right.
Assignee | ||
Comment 3•16 years ago
|
||
This patch takes the new approach we discussed in the office.
Attachment #355682 -
Attachment is obsolete: true
Attachment #356088 -
Flags: superreview?(roc)
Attachment #356088 -
Flags: review?(roc)
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [needs landing]
Attachment #356088 -
Flags: superreview?(roc)
Attachment #356088 -
Flags: superreview+
Attachment #356088 -
Flags: review?(roc)
Attachment #356088 -
Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Pushed 320763cc1e6a
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Updated•16 years ago
|
Component: Theme → Layout
Product: Firefox → Core
QA Contact: theme → layout
Target Milestone: --- → mozilla1.9.2a1
Comment 5•15 years ago
|
||
I'm working in bug 299603 but I'm troubled by this change. > data:text/html,<input style="background-color: similar-to-selected-text-background"> In this case, seems that bgContext->GetStyleDisplay()->mAppearance is not zero, but the background-color is actually specified. > + if (bgContext->GetStyleDisplay()->mAppearance) { > + // Assume a native widget has sufficient contrast always > + mSufficientContrast = 0; > + mInitCommonColors = PR_TRUE; > + return; > + } So, the condition shouldn't be > bgContext->GetStyleDisplay()->mAppearance && bg->IsTransparent() ?
Comment 6•15 years ago
|
||
(In reply to comment #5) > > + if (bgContext->GetStyleDisplay()->mAppearance) { In general, checking mAppearance to see if something is themed is the wrong thing to do. You should call nsIFrame::IsThemed instead, since that tests whether themed style requested is actually supported (and used).
You need to log in
before you can comment on or make changes to this bug.
Description
•