1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field on GTK

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement)

Description

[Tim Nguyen :ntim]

On Windows and mac, -moz-cellhighlight and Highlight correspond to different values, which lets the user differentiate between unfocused and focused states of the sidebar. On Linux, they correspond to the same value, which means the difference isn't visible.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

Review commit: https://reviewboard.mozilla.org/r/253992/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253992/

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

https://reviewboard.mozilla.org/r/253992/#review260880

At least GTK's default theme, Adwaita uses the same background color for
selected without focus as for selected with focus.  Adwaita uses a focusring,
much like the current styling.

-moz-cellhighlight should follow the native theme background color.  If you
would like to do something different from what the GTK theme does, then it is
better to use a different background color than to change -moz-cellhighlight.

I guess WindowBackground/Foreground may be an option, but it gets awkward when
using colors for different purposes than intended.  For example, there is no
intention that WindowBackground should be distinct from the tree view background.
It is typically safer to either follow the theme completely, or not use theme
colors at all, or at least not for elements that need to be distinguishable.

Comment 3

[Tim Nguyen :ntim]

Dão, what do you think of comment 2?

Comment 4

[Dão Gottwald [:dao]]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> Comment on attachment 8988821 [details]
> Bug 1472276 - Make -moz-cellhighlight and -moz-cellhighlighttext match
> ButtonFace and ButtonText on GTK.
> 
> https://reviewboard.mozilla.org/r/253992/#review260880
> 
> At least GTK's default theme, Adwaita uses the same background color for
> selected without focus as for selected with focus.  Adwaita uses a focusring,
> much like the current styling.

We would like to use more consistent tree styling across platforms, and the Mac and Windows model seems more nuanced and preferable over the Gtk one. E.g. on my system (Ubuntu 18.04) in Firefox' Bookmarks & History library, the focus ring is barely visible to the naked eye and it's hard to tell what tree is focused, which is quite annoying for keyboard interaction.

> -moz-cellhighlight should follow the native theme background color.  If you
> would like to do something different from what the GTK theme does, then it is
> better to use a different background color than to change -moz-cellhighlight.

This would just make -moz-cellhighlight unused on Linux. How exactly is that preferable? We're the only relevant consumer here, I think.

> I guess WindowBackground/Foreground may be an option, but it gets awkward
> when
> using colors for different purposes than intended.  For example, there is no
> intention that WindowBackground should be distinct from the tree view
> background.
> It is typically safer to either follow the theme completely, or not use theme
> colors at all, or at least not for elements that need to be distinguishable.

We're using the same hack on Windows: https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/widget/windows/nsLookAndFeel.cpp#291-292

In case WindowBackground matches -moz-field, the selected item wouldn't be visible in unfocused trees but still in focused ones.

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to Dão Gottwald [::dao] from comment #4)
> We would like to use more consistent tree styling across platforms, and the
> Mac and Windows model seems more nuanced and preferable over the Gtk one.
> E.g. on my system (Ubuntu 18.04) in Firefox' Bookmarks & History library,
> the focus ring is barely visible to the naked eye and it's hard to tell what
> tree is focused, which is quite annoying for keyboard interaction.

I don't have any objection to using different colors to style chrome.
(If we we're to follow the user's chosen system theme, then -moz-appearance
would be more appropriate than using background colors and Gecko-styled focus
rings, but no suitable -moz-appearance is currently implemented.)

I was, however, warning about the potential problems with using system colors.

> 
> (In reply to Karl Tomlinson (:karlt) from comment #2)
> > -moz-cellhighlight should follow the native theme background color.  If
> > you would like to do something different from what the GTK theme does,
> > then it is better to use a different background color than to change
> > -moz-cellhighlight.
> 
> This would just make -moz-cellhighlight unused on Linux. How exactly is that
> preferable?

I wouldn't see the lack of use of -moz-cellhighlight as a problem.

It is preferable to be explicit and accurate.  If the system's ButtonFace is
the desired color, then it is preferable to use that, rather than using
-moz-cellhighlight to indicate the system's cell highlight color when it is
actually something completely unrelated.

What do you mean by "exactly"?

> We're the only relevant consumer here, I think.

By "we", are you implying that you would like to change the styling of all
current -moz-cellhighlight usages in mozilla-central?

-moz-cellhighlight is exposed to content, and so we don't know what other
consumers may exist.

> > I guess WindowBackground/Foreground may be an option, but it gets awkward
> > when using colors for different purposes than intended.  For example,
> > there is no intention that WindowBackground should be distinct from the
> > tree view background.  It is typically safer to either follow the theme
> > completely, or not use theme colors at all, or at least not for elements
> > that need to be distinguishable.
> 
> We're using the same hack on Windows:
> https://searchfox.org/mozilla-central/rev/
> 6ef785903fee6c0b16a1eab79d722373d940fd78/widget/windows/nsLookAndFeel.
> cpp#291-292
> 
> In case WindowBackground matches -moz-field, the selected item wouldn't be
> visible in unfocused trees but still in focused ones.

Yes.  Would you be happy with that situation, or not?
That doesn't seem good for listbox, at least?

The same is true for ButtonFace, and still true whether the change happens in
CSS or in nsLookAndFeel.cpp.

The eColorID__moz_cellhighlight line in the revision linked, thanks, comes
from the changeset that introduced -moz-cellhighlight.  It's purpose was to
make unfocused selected items visibly distinct from unselected items with GTK.
The change to windows/nsLookAndFeel.cpp was just keeping the same previous
behavior for winnt.  The patch here is proposing to revert to the previous
(indistinct) behavior for GTK (but to use -moz-cellhighlight for that).

https://github.com/mozilla/gecko-dev/commit/07450eb9c04665ce061a706628b04b78a2cea508#diff-b478552bcdae4b190a72b9bd3208cda4R190
https://github.com/mozilla/gecko-dev/commit/07450eb9c04665ce061a706628b04b78a2cea508#diff-ada841ce12738416918d60915b514355

Comment 6

[Dão Gottwald [:dao]]

(In reply to Karl Tomlinson (:karlt) from comment #5)
> > (In reply to Karl Tomlinson (:karlt) from comment #2)
> > > -moz-cellhighlight should follow the native theme background color.  If
> > > you would like to do something different from what the GTK theme does,
> > > then it is better to use a different background color than to change
> > > -moz-cellhighlight.
> > 
> > This would just make -moz-cellhighlight unused on Linux. How exactly is that
> > preferable?
> 
> I wouldn't see the lack of use of -moz-cellhighlight as a problem.
> 
> It is preferable to be explicit and accurate.  If the system's ButtonFace is
> the desired color, then it is preferable to use that, rather than using
> -moz-cellhighlight to indicate the system's cell highlight color when it is
> actually something completely unrelated.
> 
> What do you mean by "exactly"?

You seem to be saying that it's preferable to have -moz-cellhighlight implemented correctly but unused. I don't see the point -- we might as well remove -moz-cellhighlight support from Gtk widget code at that point.

> > We're the only relevant consumer here, I think.
> 
> By "we", are you implying that you would like to change the styling of all
> current -moz-cellhighlight usages in mozilla-central?

Yes, listbox.css and tree.css.

> -moz-cellhighlight is exposed to content, and so we don't know what other
> consumers may exist.

We should probably stop exposing every -moz- prefixed color that was added for use in our desktop apps. This is nonstandard and lacks cross-browser support, so I don't think breaking this is a big deal.

> > > I guess WindowBackground/Foreground may be an option, but it gets awkward
> > > when using colors for different purposes than intended.  For example,
> > > there is no intention that WindowBackground should be distinct from the
> > > tree view background.  It is typically safer to either follow the theme
> > > completely, or not use theme colors at all, or at least not for elements
> > > that need to be distinguishable.
> > 
> > We're using the same hack on Windows:
> > https://searchfox.org/mozilla-central/rev/
> > 6ef785903fee6c0b16a1eab79d722373d940fd78/widget/windows/nsLookAndFeel.
> > cpp#291-292
> > 
> > In case WindowBackground matches -moz-field, the selected item wouldn't be
> > visible in unfocused trees but still in focused ones.
> 
> Yes.  Would you be happy with that situation, or not?
> That doesn't seem good for listbox, at least?

It would be a problem but it's hard to reason about this without knowing how common this is.

Could widget code detect that WindowBackground and -moz-field are the same and fall back to Highlight?

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Dão Gottwald [::dao] from comment #6)
> > > (In reply to Karl Tomlinson (:karlt) from comment #2)

> You seem to be saying that it's preferable to have -moz-cellhighlight
> implemented correctly but unused. I don't see the point -- we might as well
> remove -moz-cellhighlight support from Gtk widget code at that point.

I would be fine with removing -moz-cellhighlight, from the perspective of
widget code, but it sounds like we'd still need to find a replacement solution.

> (In reply to Karl Tomlinson (:karlt) from comment #5)
> > By "we", are you implying that you would like to change the styling of all
> > current -moz-cellhighlight usages in mozilla-central?
> 
> Yes, listbox.css and tree.css.

Those are not the only clients in mozilla-central, but let's assume that all
uses will be audited appropriately.

> > -moz-cellhighlight is exposed to content, and so we don't know what other
> > consumers may exist.
> 
> We should probably stop exposing every -moz- prefixed color that was added
> for use in our desktop apps.

I agree.

> > > In case WindowBackground matches -moz-field, the selected item wouldn't be
> > > visible in unfocused trees but still in focused ones.
> > 
> > Yes.  Would you be happy with that situation, or not?
> > That doesn't seem good for listbox, at least?
> 
> It would be a problem but it's hard to reason about this without knowing how
> common this is.

It is hard to reason, yes.  This is the typical problem with uses of system
colors for unintended purposes.

> Could widget code detect that WindowBackground and -moz-field are the same
> and fall back to Highlight?

Widget code could detect similarity of colors.  I'm not so keen on Highlight,
if styling is no longer going to apply a focus ring, but I assume window
background could be lightened or darkened until it is similar to neither
Highlight nor -moz-field.

This feels like an XP operation, but I guess existing code is not structured
to do this at an XP level, and so platform-specific widget code is the
simplest solution.

I'd be happy with such a solution if -moz-cellhighlight is documented (at its
definition) to clarify that this is not what the system uses for unfocused
selected items, but a color chosen to be distinct from Highlight and
-moz-field.

Some care would be required with the foreground color.
ISTR text selection colors doing something similar in XP code for arbitrary
document colors.

Comment 8

[Tim Nguyen :ntim]

Do you have an example of color similar detection code ?

Also, I'm not sure it makes sense to do this, we don't do this on Windows where the same problem could possibly happen.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #8)
> Do you have an example of color similar detection code ?

I don't... 

> Also, I'm not sure it makes sense to do this, we don't do this on Windows
> where the same problem could possibly happen.

Third-party themes are basically nonexistent on Windows at this point. We're dealing with much more diversity on Linux.

Comment 10

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #9)
> Third-party themes are basically nonexistent on Windows at this point. We're
> dealing with much more diversity on Linux.

High contrast themes with colors configurable by the user exist though.

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Tim Nguyen :ntim from comment #8)
> Do you have an example of color similar detection code ?

I suggest looking at nsTextPaintStyle::EnsureSufficientContrast() and perhaps
its use in GetHighlightColors() for ideas.

https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/layout/generic/nsTextFrame.cpp#3765
https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/layout/generic/nsTextFrame.cpp#3827

> Also, I'm not sure it makes sense to do this, we don't do this on Windows
> where the same problem could possibly happen.

What Dão says about more diversity on Linux is true, even though configuration
is possible on Windows, but also we have a higher standard re introducing
regressions, than re dealing with existing bugs (which people have already
worked around).

(In reply to Karl Tomlinson (:karlt) from comment #7)
> (In reply to Dão Gottwald [::dao] from comment #6)
> > Could widget code detect that WindowBackground and -moz-field are the same
> > and fall back to Highlight?
> 
> Widget code could detect similarity of colors.  I'm not so keen on Highlight,
> if styling is no longer going to apply a focus ring, but I assume window
> background could be lightened or darkened until it is similar to neither
> Highlight nor -moz-field.

> Some care would be required with the foreground color.
> ISTR text selection colors doing something similar in XP code for arbitrary
> document colors.

Swapping foreground and background, like the text selection, would be
effective I expect, and simpler than lightening and darkening.
I'm hoping Highlight will already be distinct from window colors.

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253992/diff/1-2/

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

https://reviewboard.mozilla.org/r/253992/#review266846

::: commit-message-f3b2b:1
(Diff revision 2)
> +Bug 1472276 - Make -moz-cellhighlight/-moz-cellhighlighttext use ButtonFace/ButtonText when possible. r=karlt

Please describe why you are making these changes.

I assume the goal is not to make elements look like buttons, and ButtonFace is
not a good representation of button colors, so the terms ButtonFace and
ButtonText are not helpful.

::: layout/base/moz.build:48
(Diff revision 2)
>      'nsBidiPresUtils.h',
>      'nsCaret.h',
>      'nsChangeHint.h',
>      'nsCompatibility.h',
>      'nsCounterManager.h',
> +    'nsCSSColorUtils.h',

Either add to EXPORTS or add to LOCAL_INCLUDES.  Both is not necessary.

::: widget/gtk/nsLookAndFeel.h:98
(Diff revision 2)
>      bool    mInitialized;
>  
>      void EnsureInit();
> +
> +private:
> +    nsresult GetCellHighlightColors(nscolor& aForeColor, nscolor& aBackColor);

Use pointers, instead of references for function out parameters, even for primitive types.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#COM_and_pointers

But no need to pass parameters at all because this is an object method and the arguments are members.

"InitCellHighlightColors" works when not actually getting anything.

::: widget/gtk/nsLookAndFeel.cpp:223
(Diff revision 2)
>  
> +nsresult
> +nsLookAndFeel::GetCellHighlightColors(nscolor& aForeColor, nscolor& aBackColor) {
> +    NS_ASSERTION(aForeColor, "aForeColor is null");
> +    NS_ASSERTION(aBackColor, "aBackColor is null");
> +    int32_t backLumnosityDifference = NS_LUMINOSITY_DIFFERENCE(mMozWindowBackground, mMozFieldBackground);

Please be consistent with spelling.
Wrap to stay within 80 columns.

::: widget/gtk/nsLookAndFeel.cpp:223
(Diff revision 2)
>  
> +nsresult
> +nsLookAndFeel::GetCellHighlightColors(nscolor& aForeColor, nscolor& aBackColor) {
> +    NS_ASSERTION(aForeColor, "aForeColor is null");
> +    NS_ASSERTION(aBackColor, "aBackColor is null");
> +    int32_t backLumnosityDifference = NS_LUMINOSITY_DIFFERENCE(mMozWindowBackground, mMozFieldBackground);

Please document the new value at
https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/widget/LookAndFeel.h#109
to clarify that this is not what the system uses for unfocused
selected items, but a color chosen to be distinct from Highlight and
-moz-field.

That would provide some hints to what is happening here.

::: widget/gtk/nsLookAndFeel.cpp:224
(Diff revision 2)
> +nsresult
> +nsLookAndFeel::GetCellHighlightColors(nscolor& aForeColor, nscolor& aBackColor) {
> +    NS_ASSERTION(aForeColor, "aForeColor is null");
> +    NS_ASSERTION(aBackColor, "aBackColor is null");
> +    int32_t backLumnosityDifference = NS_LUMINOSITY_DIFFERENCE(mMozWindowBackground, mMozFieldBackground);
> +    int32_t minLumnosityDifference = NS_SUFFICIENT_LUMINOSITY_DIFFERENCE / 5;

Please explain where / 5 comes from.

I wonder whether an absolute value would be more useful given NS_SUFFICIENT_LUMINOSITY_DIFFERENCE is not acceptable.

::: widget/gtk/nsLookAndFeel.cpp:226
(Diff revision 2)
> +        aBackColor = mMozWindowBackground;
> +        aForeColor = mButtonText;

I'd feel more comfortable using mMozWindowText with mMozWindowBackground.

::: widget/gtk/nsLookAndFeel.cpp:236
(Diff revision 2)
> +    // Darken the field color if field color is light
> +    // or lighten it if it is dark.
> +    value = value >= 128 ? value - 30 : value + 30;

I think we need something to ensure aForeColor contrasts well with aBackColor.

If value is close to 128, then it can become quite light or dark.

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #7)
> > (In reply to Karl Tomlinson (:karlt) from comment #5)
> > > By "we", are you implying that you would like to change the styling of all
> > > current -moz-cellhighlight usages in mozilla-central?
> > 
> > Yes, listbox.css and tree.css.
> 
> Those are not the only clients in mozilla-central, but let's assume that all
> uses will be audited appropriately.

What did you find in this audit?

Comment 15

[Dão Gottwald [:dao]]

tree.css and richlistbox.css are the only users:
https://searchfox.org/mozilla-central/search?q=-cellhighlight&case=true&path=linux

Comment 16

[Karl Tomlinson (:karlt)]

Why require linux in the path?

Comment 17

[Dão Gottwald [:dao]]

- browser/themes/shared/syncedtabs/sidebar.inc.css

This just mimics tree.css.

- layout/style/res/forms.css

This works fine on Windows, should work here too with the fallback solution in place. Note that the use case (<select multiple>) is not exactly the same but somewhat similar to tree.css and richlistbox.css.

- toolkit/content/plugins.css

about:plugins, I don't think anyone cares about this.

Comment 18

[Tim Nguyen :ntim]

Attachment: Shades of gray with steps of 10% (25.5)

> I think we need something to ensure aForeColor contrasts well with aBackColor.
> If value is close to 128, then it can become quite light or dark.

Not sure that's a huge issue, I put shades of gray with steps of 10% (~26 on a scale of 255, it's 30 (12%) rounded down for convenience). The values around the center are still contrasted enough with the text.

Seems more like an edge case to me. Even if we should solve it, I'm not sure what we should do, we can't significantly reduce the +/-30 number, because contrast check between -moz-cellhighlight and -moz-field will fail, and inverting the text color would result in using a non-platform color.

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253992/diff/2-3/

Comment 20

[Karl Tomlinson (:karlt)]

(In reply to Tim Nguyen :ntim from comment #18)
> Not sure that's a huge issue, I put shades of gray with steps of 10% (~26 on
> a scale of 255, it's 30 (12%) rounded down for convenience). The values
> around the center are still contrasted enough with the text.

This seems to be assuming the foreground color is black when value >= 128 and
white otherwise, and seems to only go as far as half value.
With the black/white assumption, the extremes would be these:

data:text/html,<div style="background-color:rgb(98,98,98);color:black">Hello
data:text/html,<div style="background-color:rgb(157,157,157);color:white">Hello

On my monitor, the dark colors are weak.
If the foreground color is not black or white, then the situation is worse.

> Seems more like an edge case to me. Even if we should solve it, I'm not sure
> what we should do, we can't significantly reduce the +/-30 number, because
> contrast check between -moz-cellhighlight and -moz-field will fail, and
> inverting the text color would result in using a non-platform color.

I don't know why you are concerned about using a non-platform color, because
that seems to be whole intent of this patch.

Options to consider include changing the decision-making to choose between
darkening or lighten, and/or darkening or lightening the text color.

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

https://reviewboard.mozilla.org/r/253992/#review267676

::: commit-message-f3b2b:3
(Diff revision 3)
> +Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field r=karlt
> +
> +This color needs to be distinct from Highlight, used on selected and focused items

Please explain why it needs to be distinct from Highlight, when it has been fine until now.

::: commit-message-f3b2b:4
(Diff revision 3)
> +Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field r=karlt
> +
> +This color needs to be distinct from Highlight, used on selected and focused items
> +and also distinct from -moz-field, which is used as the box background.

Which box is this?

https://reviewboard.mozilla.org/r/253448/diff/12 uses -moz-appearance: listbox,
which overrides background-color.

If this is used with listbox, then please document that.

Please document in InitCellHighlightColors() the assumption that -moz-appearance:listbox provides a similar background color to mMozFieldBackground.

::: widget/LookAndFeel.h:109
(Diff revision 3)
>      // used to cell text background, selected but not focus
> +    // it is not necessarily a system color, but it is

Please use punctuation to separate sentences.

::: widget/gtk/nsLookAndFeel.cpp:221
(Diff revision 3)
> +    NS_ASSERTION(mMozCellHighlightBackground,
> +        "mMozCellHighlightBackground is null");
> +    NS_ASSERTION(mMozCellHighlightText, "mMozCellHighlightText is null");

These variables are not pointers.  They may be uninitialized at this point.

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

Paulo, are you able to approve this change to richlistbox styling?  The
intention is that only the focused item is shown in the system selected
item color.  Other selected items, in downloads and library windows,
for example, would typically have a grey background, often similar to the
window background.

You can experiment by setting ui.-moz-cellhighlight;grey or similar.

Comment 23

[:Paolo Amadini]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

I'm not sure what you're asking. The idea looks good in principle, but I haven't followed the discussion on this bug so far, so I can't review how the color is calculated, and I don't see any changes in the actual styling in the "toolkit" folder.

Comment 24

[Tim Nguyen :ntim]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253992/diff/3-4/

Comment 25

[Tim Nguyen :ntim]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253992/diff/4-5/

Comment 26

[Tim Nguyen :ntim]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253992/diff/5-6/

Comment 27

[Karl Tomlinson (:karlt)]

(In reply to :Paolo Amadini from comment #23)
> I'm not sure what you're asking. The idea looks good in principle, but I
> haven't followed the discussion on this bug so far, so I can't review how
> the color is calculated, and I don't see any changes in the actual styling
> in the "toolkit" folder.

I'm asking whether you think it is appropriate that, if a selected item in a
richlistbox (or tree) is not focused, then it should not look like selected
items in other apps on the system?  Are you happy that it should instead look
greyed out, for example, even if that is not consistent with other apps?

This is not a change that I would recommend, but Dão doesn't like the way
selection/focus is done on typical GTK systems, and so would like to do
something different.  Because this only affects toolkit styling, I'm looking
for an appropriate person willing to own these effects on toolkit styling.
i.e. regressions would not be considered Widget bugs but Theme bugs, because
the choices are made for the sake of toolkit styling.

There are no changes to the files in the toolkit folder because the toolkit
folder uses the -moz-cellhighlight color.  The proposal is to change to a
different color but reuse the same name (because the old name would otherwise
be obsolete).  The styling would be changed, even if the files look the same.

If you are not able to read the discussion, or you are otherwise not the
appropriate person to ask, then are you able to redirect, please?

Comment 28

[Tim Nguyen :ntim]

(In reply to Karl Tomlinson (:karlt) from comment #27)
> (In reply to :Paolo Amadini from comment #23)
> > I'm not sure what you're asking. The idea looks good in principle, but I
> > haven't followed the discussion on this bug so far, so I can't review how
> > the color is calculated, and I don't see any changes in the actual styling
> > in the "toolkit" folder.
> 
> I'm asking whether you think it is appropriate that, if a selected item in a
> richlistbox (or tree) is not focused, then it should not look like selected
> items in other apps on the system?  Are you happy that it should instead look
> greyed out, for example, even if that is not consistent with other apps?

I just want to point out that this is consistent with what's done on Windows and MacOS.

Comment 29

[Tim Nguyen :ntim]

This is the UX spec for Linux: https://firefoxux.github.io/people/shorlander/photon/Mockups/linux.html

You can toggle the "Dark theme" checkbox to see how it's supposed to look like with dark themes.

Comment 30

[Tim Nguyen :ntim]

You also need to toggle the "Sidebar" checkbox to see the tree.

Comment 31

[Dão Gottwald [:dao]]

(In reply to Karl Tomlinson (:karlt) from comment #27)
> I'm asking whether you think it is appropriate that, if a selected item in a
> richlistbox (or tree) is not focused, then it should not look like selected
> items in other apps on the system?  Are you happy that it should instead look
> greyed out, for example, even if that is not consistent with other apps?
> 
> This is not a change that I would recommend, but Dão doesn't like the way
> selection/focus is done on typical GTK systems, and so would like to do
> something different.  Because this only affects toolkit styling, I'm looking
> for an appropriate person willing to own these effects on toolkit styling.
> i.e. regressions would not be considered Widget bugs but Theme bugs, because
> the choices are made for the sake of toolkit styling.

I'm the person you're looking for.

Comment 32

[:Paolo Amadini]

Karl, thanks for clarifying, that was helpful. I think you asked two different questions, so let me phrase my take on them separately.

As far as code ownership goes, yes, it is my understanding that Dão, as a Firefox and Toolkit peer, can be the person responsible for the Toolkit styling aspect of this change, and you would be responsible for maintaining the theming code specific to Linux, which happens to be the only code touched here.

Regarding whether the change in appearance in Firefox should be made or not, on which there is a disagreement here, I believe the final say would always be with the user experience team. The three of us can be consultants, and provide feedback or ask for changes. In this case we have a UX specification (thanks Tim for posting the link to it) and while it isn't at all perfect, I think the intention is to have a separate color for rows that are selected but not focused on Linux. You can reach out to Stephen if there is any edge case that needs discussing.

Finally, I'd like to add a thought, before the question comes up. The Linux theming code is still imported as third-party code by applications that are not Firefox, together with other code in mozilla-central. The Firefox user experience team is not responsible for the appearance of these applications, but, as code module owners and peers, we're still asked to consider the entirety of mozilla-central as strictly functional only to Firefox and other Mozilla experimental products.

In this specific case I don't see a conflict with other applications, because if they don't want to change their appearance, they can easily override the Toolkit styling to use "Highlight" instead of "-moz-cellhighlight". However, if there was a technical difficulty in doing that, we would only need to write the code to solve the Firefox use case. It would be the job of other application developers to keep the appearance they need.

The clear implication here is that the entirety of the modules in mozilla-central that deal with user-facing visuals, including "Widget - GTK", are developed following input from the Firefox user experience team. This expanded scope of the team has already been in place for quite some time now, but is worth spelling out. Obviously, as code module owners and peers we will still discuss the specifications and ask for sensible changes where we think it's appropriate.

Hope this helps, and that this matches your understanding as well. If you think anything is incorrect, it's worth reaching out to someone more authoritative than me, like Dave Townsend for Firefox or the Module Ownership System peers for more general questions.

Comment 33

[Karl Tomlinson (:karlt)]

Thank you for the explanations.  I'm happy to delegate to the user experience
team as this only affects Firefox UI (except for caveats discussed by Paolo).
Thank you, Dão for owning this.

Thank you, Tim, for the link to the UX spec.  That's clear enough for trees.
I don't see any richlistboxes, and I don't know much about how they are used.

In Mozilla, we have a system where we typically require two people to agree
that a change is beneficial.  Dão asked Tim to implement something like this.
Tim is not really an independent second person in this case.  The original
suggestion required some refinement to be workable, and so the two-person
system was working, even if it felt strained.  I don't know enough about
richlistboxes to continue to be the second person.  I've given Paolo the
opportunity to be a second person.  My understanding is that he is saying he
is qualified to be a second person but handing this back to Dão to decide on
his own.

Comment 34

[Karl Tomlinson (:karlt)]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

https://reviewboard.mozilla.org/r/253992/#review268658

This seems close enough now that Dão can take over reviews.  Best that a user experience person reviews this to determine whether it is the behavior wanted by that team.

::: commit-message-f3b2b:3
(Diff revision 6)
> +Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field r=karlt
> +
> +Since unfocused+selected cells will no longer get an outline, we will start using a different background for them.

I assume you mean focused.

::: commit-message-f3b2b:3
(Diff revision 6)
> +Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field r=karlt
> +
> +Since unfocused+selected cells will no longer get an outline, we will start using a different background for them.

Will richlistbox items no longer get an outline?

::: widget/LookAndFeel.h:111
(Diff revision 6)
>      // used to highlight valid regions to drop something onto
>      eColorID__moz_dragtargetzone,
>  
> -    // used to cell text background, selected but not focus
> +    // used to cell text background, selected but not focus.
> +    // It is not necessarily a system color, but it is
> +    // distinct from -moz-field and Highlight

"intended to be distinct from -moz-appearance:outline"

::: widget/gtk/nsLookAndFeel.cpp:220
(Diff revision 6)
>      *aDarkColor = GDK_RGBA_TO_NS_RGBA(darkColor);
>      return ret;
>  }
>  
> +// Finds ideal cell highlight colors used for unfocused+selected cells distinct
> +// from both Highlight, used as focused+selected background, and the listbox 

Trailing whitespace.

::: widget/gtk/nsLookAndFeel.cpp:221
(Diff revision 6)
>      return ret;
>  }
>  
> +// Finds ideal cell highlight colors used for unfocused+selected cells distinct
> +// from both Highlight, used as focused+selected background, and the listbox 
> +// background which is assumed to be -moz-field

"similar to -moz-field".

::: widget/gtk/nsLookAndFeel.cpp:252
(Diff revision 6)
> +    // Otherwise, try both and see what works best with the text color
> +    else {
> +        int32_t attemptedLuminance = luminance - step;
> +        nscolor attemptedColor = mMozCellHighlightBackground;
> +        NS_HSV2RGB(attemptedColor, hue, sat, attemptedLuminance, alpha);
> +        int32_t textLuminosityDifference = NS_LUMINOSITY_DIFFERENCE(
> +            attemptedColor, mMozCellHighlightText);
> +        if (textLuminosityDifference >= NS_SUFFICIENT_LUMINOSITY_DIFFERENCE) {
> +            // The luminance worked well, use it.
> +            luminance = attemptedLuminance;
> +        }
> +        else {
> +            // Do the opposite.
> +            luminance += step;
> +        }
> +    }

The comment says to see what works best, but the code is trying an arbitrary
direction and testing whether it is good enough.  That is favoring one
direction over another.

I'd like a consistent/symmetric choice of direction.

Would you be happy to always move in the direction of increasing contrast?
That decision would be made simply based on whether the foreground or
backround had higher luminance/osity.

If that is not appropriate, is there a contrast that you would like to aim
for, and pick the direction that gives the result closest to that value?

Comment 35

[Tim Nguyen :ntim]

(In reply to Karl Tomlinson (:karlt) from comment #34)
> Comment on attachment 8988821 [details]
> Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field
> 
> https://reviewboard.mozilla.org/r/253992/#review268658
> 
> This seems close enough now that Dão can take over reviews.  Best that a
> user experience person reviews this to determine whether it is the behavior
> wanted by that team.
> 
> ::: commit-message-f3b2b:3
> (Diff revision 6)
> > +Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field r=karlt
> > +
> > +Since unfocused+selected cells will no longer get an outline, we will start using a different background for them.
> 
> I assume you mean focused.

Yes, sorry. I guess I was bit tired when writing this.

> ::: commit-message-f3b2b:3
> (Diff revision 6)
> > +Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field r=karlt
> > +
> > +Since unfocused+selected cells will no longer get an outline, we will start using a different background for them.
> 
> Will richlistbox items no longer get an outline?

Not part of any immediate plans.

> ::: widget/LookAndFeel.h:111
> (Diff revision 6)
> >      // used to highlight valid regions to drop something onto
> >      eColorID__moz_dragtargetzone,
> >  
> > -    // used to cell text background, selected but not focus
> > +    // used to cell text background, selected but not focus.
> > +    // It is not necessarily a system color, but it is
> > +    // distinct from -moz-field and Highlight
> 
> "intended to be distinct from -moz-appearance:outline"

I'm not sure I understand what you meant here.

> ::: widget/gtk/nsLookAndFeel.cpp:252
> (Diff revision 6)
> > +    // Otherwise, try both and see what works best with the text color
> > +    else {
> > +        int32_t attemptedLuminance = luminance - step;
> > +        nscolor attemptedColor = mMozCellHighlightBackground;
> > +        NS_HSV2RGB(attemptedColor, hue, sat, attemptedLuminance, alpha);
> > +        int32_t textLuminosityDifference = NS_LUMINOSITY_DIFFERENCE(
> > +            attemptedColor, mMozCellHighlightText);
> > +        if (textLuminosityDifference >= NS_SUFFICIENT_LUMINOSITY_DIFFERENCE) {
> > +            // The luminance worked well, use it.
> > +            luminance = attemptedLuminance;
> > +        }
> > +        else {
> > +            // Do the opposite.
> > +            luminance += step;
> > +        }
> > +    }
> 
> Would you be happy to always move in the direction of increasing contrast?
> That decision would be made simply based on whether the foreground or
> backround had higher luminance/osity.

That sounds like a nice way to do it.

Comment 36

[Tim Nguyen :ntim]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253992/diff/6-7/

Comment 37

[Karl Tomlinson (:karlt)]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

https://reviewboard.mozilla.org/r/253992/#review268658

> Will richlistbox items no longer get an outline?

Because this is about "cellhighlight", it would be easy to incorrectly
interpret "cells" in this sentence indicated both tree cells and listbox
items because they both use cellhighlight.

I think it is worth being more explicit by saying "tree cells".

> "intended to be distinct from -moz-appearance:outline"

s/distinct from -moz-field and Highlight/intended to be distinct from -moz-appearance:outline and Highlight/

Comment 38

[Karl Tomlinson (:karlt)]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

https://reviewboard.mozilla.org/r/253992/#review268906

::: commit-message-f3b2b:5
(Diff revisions 6 - 7)
>  Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field r=karlt
>  
> -Since unfocused+selected cells will no longer get an outline, we will start using a different background for them.
> +Since focused+selected cells will no longer get an outline, we will start using a different background for the unfocused state.
>  That background needs to be distinct from Highlight, used on selected and focused items
>  and also distinct from -moz-field, which is used as the box background.

The color does not need to be distinct from -moz-field, because -moz-field will not be seen due to the use of -moz-appearance:listbox.

The color needs to be distinct from -moz-appearance:listbox.

::: widget/gtk/nsLookAndFeel.cpp:255
(Diff revisions 6 - 7)
>          luminance -= step;
>      }
> -    // Otherwise, try both and see what works best with the text color
> +    // Otherwise, compute what works best depending on the text luminance.
>      else {
> -        int32_t attemptedLuminance = luminance - step;
> -        nscolor attemptedColor = mMozCellHighlightBackground;
> +        uint16_t textLuminance;
> +        NS_RGB2HSV(mMozCellHighlightText, hue, sat, textLuminance, alpha);

Don't overwrite background HSA here, because they are used below.

Comment 39

[Tim Nguyen :ntim]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253992/diff/7-8/

Comment 40

[Tim Nguyen :ntim]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253992/diff/8-9/

Comment 41

[Karl Tomlinson (:karlt)]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

https://reviewboard.mozilla.org/r/253992/#review268658

> s/distinct from -moz-field and Highlight/intended to be distinct from -moz-appearance:outline and Highlight/

As discussed on irc, "intended to be distinct from -moz-appearance:listbox and Highlight."

Comment 42

[Karl Tomlinson (:karlt)]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

https://reviewboard.mozilla.org/r/253992/#review269040

With that one change, the patch is good for the intentions here, thanks.
I'll pass review to Dão to confirm this is what is wanted.

Comment 43

[Dão Gottwald [:dao]]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

Based on following the bug and skimming the patch, this seems in line with the UX spec and with what I had in mind, but technically this should still be signed off by a widget peer.

Comment 44

[Tim Nguyen :ntim]

Attachment: Conversation with shorlander

:shorlander approved the richlistbox color change on Slack. Putting a screenshot of the conversation here.

Comment 45

[Karl Tomlinson (:karlt)]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

https://reviewboard.mozilla.org/r/253992/#review269190

This is the same revision that I reviewed last time.

I can review when the last issue is resolved.

Comment 46

[Tim Nguyen :ntim]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253992/diff/9-10/

Comment 47

[Karl Tomlinson (:karlt)]

Comment on attachment 8988821 [details]
Bug 1472276 - Make -moz-cellhighlight distinct from Highlight and -moz-field

https://reviewboard.mozilla.org/r/253992/#review269314

Thank you for tidying this up.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a54c28335c4
Make -moz-cellhighlight distinct from Highlight and -moz-field r=karlt

Comment 49

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/3a54c28335c4