472410 - URL bar highlight mistakenly switches colours on dark themes

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Michael Ventnor]

Attachment: Patch

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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 3

[Michael Ventnor]

Attachment: Patch 2

This patch takes the new approach we discussed in the office.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 320763cc1e6a

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

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

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

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

great! thank you.