1360500 - Change the highlight color to align the design spec

Status: VERIFIED FIXED

(Firefox :: Settings UI, enhancement, P1)

Description

[Evan Tseng [:evanxd]]

Change the highlight color to follow the design spec[1], [2].

[1]: https://mozilla.invisionapp.com/share/ZDAGPK3AF#/screens/219340982
[2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1357130#c2

Comment 1

[Evan Tseng [:evanxd]]

Hi Jared,

Change highlight color from #EF0FFF to #FFE900 to align the spec.

It looks like we could have two ways to change the color:
1. Update the `TextHighlightBackground` color in nsXPLookAndFeel.cpp[1].
2. Use the `ui.textHighlightBackground` and set its value as `#FFE900`.

Which way is our prefer? Or any other way?

Thanks.

[1]: http://searchfox.org/mozilla-central/source/widget/nsXPLookAndFeel.cpp#806
[2]: http://searchfox.org/mozilla-central/source/widget/nsXPLookAndFeel.cpp#167

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Hi Evan, we cannot fix the bug using either your (1) or (2) solution, as that will change the highlight of text for other webpages.

You will need to add a new selection type and set the color there. See how SELECTION_URLSECONDARY is implemented for example. The new selection type should be added to http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/dom/base/nsISelectionController.idl#31. Then you will need to update http://searchfox.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#6011 to set the colors for the selection.

See http://searchfox.org/mozilla-central/search?q=eURLSecondary&path= and http://searchfox.org/mozilla-central/search?q=SELECTION_URLSECONDARY&path= for reference. Also, when creating the selection controller in the preferences JS code, you will need to specify the new selection ID.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thanks Evan and Jared. Let me see if I can get this up and running from comment 2.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1360500 - Allow custom colors on find selection type selections.

This patch introduces nsISelectionController.SELECTION_FIND_IN_PREFERENCES
selection type and set the background color to #ffe900. The text color
remain unchanged. We'll invert the background color in high-contrast
mode to make sure the text remain readable.

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

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8866000 [details]
Bug 1360500 - Allow custom colors on find selection type selections.

https://reviewboard.mozilla.org/r/137540/#review140716

This looks right to me but it needs to be reviewed by a dom/layout peer.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8866000 [details]
Bug 1360500 - Allow custom colors on find selection type selections.

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

Comment 7

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

Hmm, I don't like to add new selection type for this purpose because:

1. The selection is really same meaning as FIND.
2. You need to change the selection color of FIND only in specific page.
3. If other UI needs to change the FIND color, e.g., Add-on Manager, we also need to add new selection type.

So, I think that adding new method to nsISelection to specify the colors is better, e.g.,

/**
 * setColors() sets custom colors for the selection.
 * Currently, this is supported only when the selection type is SELECTION_FIND.
 * Otherwise, throws an exception.
 *
 * @param aForeColor       The foreground color of the selection.
 *                         If this is "currentColor", foreground color isn't
 *                         changed by this selection.
 * @param aBackColor       The background color of the selection.
 *                         If this is "transparent", background color is never
 *                         painted.
 * @param aAltForeColor    The alternative foreground color of the selection.
 *                         If aBackColor doesn't have sufficient contrast with
 *                         its around or foreground color if "currentColor" is
 *                         specified, alternative colors are used if it have
 *                         higher contrast.
 * @param aAltBackColor    The alternative background color of the selection.
 */
void setColors(DOMString aForeColor, DOMString aBackColor,
               DOMString aAltForeColor, DOMString aAltBackColor);

/**
 * resetColors() forget the customized colors which were set by setColors().
 */
void resetColors();


Code parsing color is here:
https://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/dom/base/nsAttrValue.cpp#1594

Smaug, Mats, do you have any objection or better ideas?

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Masayuki,

While I agree generic interfaces works better should further use case like this happens, I would need your mentorship to navigate through the selection code to implement that. Specifically, can you tell me to pass the colors set in nsISelection all the way to nsTextPaintStyle or nsTextFrame? I didn't find anything obvious that can get me a reference of nsISelection there. Thanks.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I'm a bit lost what this bug is about.  If just about browser internal find, then I totally agree with Masayuki that adding new selection type is overkill.
One should just be able to control highlight of some particular selection type better.

(I can't now recall where we control the current find highlight colors - not talking about the modal find, but the old style which I have enabled)

Comment 10

[Mats Palmgren (inactive)]

Adding nsISelection methods to customize the colors seems like the right way to fix this.
(I'm not sure if aAltForeColor/aAltBackColor are needed though.  It seems we can just let
EnsureSufficientContrast() do its magic in this case.)

Tim, I think you can rename nsTextPaintStyle::GetHighlightColors to GetSelectionColorsForType
and add a "SelectionType aSelectionType" param to it (first).  Then pass along aSelectionType:
http://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/layout/generic/nsTextFrame.cpp#6010
GetSelectionColorsForType can then get the nsISelection for aSelectionType
and query if it has custom colors and use them if so, otherwise fall back to
what it currently does.  Does that work?

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Mats Palmgren (:mats) from comment #10)
> GetSelectionColorsForType can then get the nsISelection for aSelectionType
> and query if it has custom colors and use them if so, otherwise fall back to
> what it currently does.  Does that work?

How do I get nsISelection from aSelectionType in GetSelectionColorsForType? SelectionType is just a enum.

https://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/dom/base/nsISelectionController.idl#285

Comment 12

[Mats Palmgren (inactive)]

I would guess something like this should work:
const nsFrameSelection* frameSelection = GetConstFrameSelection();
frameSelection->GetSelection(aSelectionType);

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

That won't work unfortunately because nsTextPaintStyle::GetHighlightColors has no access to nsTextFrame::GetConstFrameSelection(). Neither does nsTextFrame::GetSelectionTextColors() because it's static.

My WIP currently stores the colors within a struct that I will try to get from nsISelection from nsTextFrame::PaintTextWithSelectionColors() and pass it all the way to functions mentioned above. I've not yet get it working though.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8866000 [details]
Bug 1360500 - Allow custom colors on find selection type selections.

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

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8866000 [details]
Bug 1360500 - Allow custom colors on find selection type selections.

Hum, I thought MozReview-Commit-ID means something to MozReview. Never mind.

Masayuki, this implements what you asked, although I am pretty sure I am not using pointers correctly. The combinations of "currentColor" and "transparent" probably needs more careful thought, but it's really not the scope of the project I am working on, so I rather we put an NS_ERROR("Not Implemented") on some conditions.

There is also an XXX on sufficient luminosity difference calculations. The problem here is that we don't want to invert the background color under these light-blue link text but we do want to invert it in the high-contrast mode, when the text is white. I wonder if we'll pass that color from |setColors()| also.

Comment 16

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

Comment on attachment 8866000 [details]
Bug 1360500 - Allow custom colors on find selection type selections.

https://reviewboard.mozilla.org/r/137540/#review141454

::: layout/generic/Selection.h:433
(Diff revision 3)
>    nsCOMArray<nsISelectionListener> mSelectionListeners;
>    nsRevocableEventPtr<ScrollSelectionIntoViewEvent> mScrollEvent;
>    CachedOffsetForFrame *mCachedOffsetForFrame;
>    nsDirection mDirection;
>    SelectionType mSelectionType;
> +  SelectionCustomColors *mCustomColors;

nit: put * to the end of type name instead of member name.

::: layout/generic/nsSelection.cpp:6836
(Diff revision 3)
> +NS_IMETHODIMP
> +Selection::SetColors(const nsAString& aForeColor, const nsAString& aBackColor,
> +                     const nsAString& aAltForeColor, const nsAString& aAltBackColor)
> +{
> +  ErrorResult result;
> +  SetColors(aForeColor, aBackColor, aAltForeColor, aAltBackColor);

Isn't this calls itself recursively?

::: layout/generic/nsSelection.cpp:6842
(Diff revision 3)
> +  return result.StealNSResult();
> +}
> +
> +void
> +Selection::SetColors(const nsAString& aForeColor, const nsAString& aBackColor,
> +                     const nsAString& aAltForeColor, const nsAString& aAltBackColor,

nit: over 80 characters. Perhaps, wrapping after every argument is better for here and above method.

::: layout/generic/nsSelection.cpp:6853
(Diff revision 3)
> +  nsString currentColorStr = NS_LITERAL_STRING("currentColor");
> +  nsString transparentStr = NS_LITERAL_STRING("transparent");

Use NS_NAMED_LITERAL_STRING.

::: layout/generic/nsSelection.cpp:6859
(Diff revision 3)
> +  nsString transparentStr = NS_LITERAL_STRING("transparent");
> +
> +  if (!aForeColor.Equals(currentColorStr)) {
> +    nscolor foreColor;
> +    nsAttrValue aForeColorValue;
> +    aForeColorValue.ParseColor(aForeColor);

When this returns false, you need to treat the error. (Perhaps, raising an exception is easy and safe.)

::: layout/generic/nsSelection.cpp:6869
(Diff revision 3)
> +  }
> +
> +  if (!aBackColor.Equals(transparentStr)) {
> +    nscolor backColor;
> +    nsAttrValue aBackColorValue;
> +    aBackColorValue.ParseColor(aBackColor);

When this returns false, you need to treat the error. (Perhaps, raising an exception is easy and safe.)

::: layout/generic/nsSelection.cpp:6879
(Diff revision 3)
> +  }
> +
> +  if (!aAltForeColor.Equals(currentColorStr)) {
> +    nscolor altForeColor;
> +    nsAttrValue aAltForeColorValue;
> +    aAltForeColorValue.ParseColor(aAltForeColor);

When this returns false, you need to treat the error. (Perhaps, raising an exception is easy and safe.)

::: layout/generic/nsSelection.cpp:6889
(Diff revision 3)
> +  }
> +
> +  if (!aAltBackColor.Equals(transparentStr)) {
> +    nscolor altBackColor;
> +    nsAttrValue aAltBackColorValue;
> +    aAltBackColorValue.ParseColor(aAltBackColor);

When this returns false, you need to treat the error. (Perhaps, raising an exception is easy and safe.)

::: layout/generic/nsSelection.cpp:6898
(Diff revision 3)
> +    mCustomColors->mAltBackColor = Nothing();
> +  }
> +}
> +
> +NS_IMETHODIMP
> +Selection::ResetColors() {

nit: put the |{| to the start of next line.

::: layout/generic/nsSelection.cpp:6905
(Diff revision 3)
> +  ResetColors(result);
> +  return result.StealNSResult();
> +}
> +
> +void
> +Selection::ResetColors(ErrorResult& aRv) {

nit: put the |{| to the start of the next line.

::: layout/generic/nsTextFrame.cpp:3849
(Diff revision 3)
> +    EnsureSufficientContrast(&foreColor, &backColor);
> +    if (foreColor != *aCustomColors->mForeColor &&

You can check the result of EnsureSufficientContrast() instead of checking if foreColor is not changed.

::: layout/generic/nsTextFrame.cpp:3853
(Diff revision 3)
> +      foreColor = *aCustomColors->mAltForeColor;
> +      backColor = *aCustomColors->mAltBackColor;

Ideally, we should use colorset which have higher contrast. But it's okay.

::: layout/generic/nsTextFrame.cpp:3858
(Diff revision 3)
> +    *aBackColor = backColor;
> +
> +    return;

nit: I think that this empty line is redundant.

::: layout/generic/nsTextFrame.cpp:3864
(Diff revision 3)
> +
> +    return;
> +  }
> +
> +  if (aCustomColors->mBackColor) {
> +    nscolor foreColor = GetTextColor();

Please explain that !mForeColor means "currentColor".

::: layout/generic/nsTextFrame.cpp:3890
(Diff revision 3)
> +    return;
> +  }
> +
> +  if (aCustomColors->mForeColor) {
> +    nscolor foreColor = *aCustomColors->mForeColor;
> +    nscolor backColor = mFrameBackgroundColor;

Please explain that !mBackColor means "transparent".

::: layout/generic/nsTextFrame.cpp:3894
(Diff revision 3)
> +    nscolor foreColor = *aCustomColors->mForeColor;
> +    nscolor backColor = mFrameBackgroundColor;
> +
> +    if (aCustomColors->mAltForeColor) {
> +      EnsureSufficientContrast(&foreColor, &backColor);
> +      if (foreColor != *aCustomColors->mForeColor) {

You can check the result of EnsureSufficientContrast() instead.

On the other hand, if we can check which is better foreground color, it's the best. But it may not necessary for now.

::: layout/generic/nsTextFrame.cpp:3905
(Diff revision 3)
> +    *aForeColor = foreColor;
> +    *aBackColor = backColor;
> +
> +    return;
> +  }
>  }

If !mForeColor and !mBackColor, that means that the selection shouldn't change the color.

So, perhaps, you should do:

*aForeColor = GetTextColor();
*aBackColor = NS_TRANSPARENT;

::: layout/generic/nsTextFrame.cpp:6366
(Diff revision 3)
> +  const nsFrameSelection* frameSelection = GetConstFrameSelection();
> +  const Selection* selection = 
> +    frameSelection->GetSelection(SelectionType::eFind);
> +  const SelectionCustomColors* customColors;
> +  if (selection) {
> +    customColors = selection->GetCustomColors();
> +  }

I wonder, if you add |const nsFrameSelection*| to the member of nsTextPaintStyle, is it easier to maintain when somebody will add this feature to the other selection types?

::: layout/generic/nsTextFrame.cpp:6367
(Diff revision 3)
>      const nsCharClipDisplayItem::ClipEdges& aClipEdges)
>  {
>    const gfxTextRun::Range& contentRange = aParams.contentRange;
>  
> +  const nsFrameSelection* frameSelection = GetConstFrameSelection();
> +  const Selection* selection = 

nit: remove the trailing whitespace.

::: layout/generic/nsTextFrame.cpp:6369
(Diff revision 3)
>    const gfxTextRun::Range& contentRange = aParams.contentRange;
>  
> +  const nsFrameSelection* frameSelection = GetConstFrameSelection();
> +  const Selection* selection = 
> +    frameSelection->GetSelection(SelectionType::eFind);
> +  const SelectionCustomColors* customColors;

Init it with nullptr.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8866000 [details]
Bug 1360500 - Allow custom colors on find selection type selections.

https://reviewboard.mozilla.org/r/137540/#review141454

> I wonder, if you add |const nsFrameSelection*| to the member of nsTextPaintStyle, is it easier to maintain when somebody will add this feature to the other selection types?

I have move this part to nsTextPaintStyle::GetHighlightColors(). I was indeed wrong at comment 13. I can get to nsTextFrame (and thus nsFrameSelection and Selection) because there is a mFrame property I over looked.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8866000 [details]
Bug 1360500 - Allow custom colors on find selection type selections.

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

Comment 19

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

Comment on attachment 8866000 [details]
Bug 1360500 - Allow custom colors on find selection type selections.

https://reviewboard.mozilla.org/r/137540/#review141536

This includes webidl change. So, needs DOM peer's review. Perhaps, smaug?

::: layout/generic/nsSelection.cpp:6832
(Diff revision 4)
>    
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +Selection::SetColors(const nsAString& aForeColor, 

nit: remove the trailing whitespace.

::: layout/generic/nsSelection.cpp:6843
(Diff revision 4)
> +  SetColors(aForeColor, aBackColor, aAltForeColor, aAltBackColor, result);
> +  return result.StealNSResult();
> +}
> +
> +void
> +Selection::SetColors(const nsAString& aForeColor, 

nit: remove the trailing whitespace.

::: layout/generic/nsSelection.cpp:6845
(Diff revision 4)
> +}
> +
> +void
> +Selection::SetColors(const nsAString& aForeColor, 
> +                     const nsAString& aBackColor,
> +                     const nsAString& aAltForeColor, 

nit: remove the trailing whitespace.

::: layout/generic/nsSelection.cpp:6865
(Diff revision 4)
> +  if (!aForeColor.Equals(currentColorStr)) {
> +    nscolor foreColor;
> +    nsAttrValue aForeColorValue;
> +    aForeColorValue.ParseColor(aForeColor);
> +    if (!aForeColorValue.GetColorValue(foreColor)) {
> +      aRv.Throw(NS_ERROR_FAILURE);

nit: NS_ERROR_INVALID_ARG

::: layout/generic/nsSelection.cpp:6878
(Diff revision 4)
> +  if (!aBackColor.Equals(transparentStr)) {
> +    nscolor backColor;
> +    nsAttrValue aBackColorValue;
> +    aBackColorValue.ParseColor(aBackColor);
> +    if (!aBackColorValue.GetColorValue(backColor)) {
> +      aRv.Throw(NS_ERROR_FAILURE);

nit: NS_ERROR_INVALID_ARG

::: layout/generic/nsSelection.cpp:6891
(Diff revision 4)
> +  if (!aAltForeColor.Equals(currentColorStr)) {
> +    nscolor altForeColor;
> +    nsAttrValue aAltForeColorValue;
> +    aAltForeColorValue.ParseColor(aAltForeColor);
> +    if (!aAltForeColorValue.GetColorValue(altForeColor)) {
> +      aRv.Throw(NS_ERROR_FAILURE);

nit: NS_ERROR_INVALID_ARG

::: layout/generic/nsSelection.cpp:6904
(Diff revision 4)
> +  if (!aAltBackColor.Equals(transparentStr)) {
> +    nscolor altBackColor;
> +    nsAttrValue aAltBackColorValue;
> +    aAltBackColorValue.ParseColor(aAltBackColor);
> +    if (!aAltBackColorValue.GetColorValue(altBackColor)) {
> +      aRv.Throw(NS_ERROR_FAILURE);

nit: NS_ERROR_INVALID_ARG

::: layout/generic/nsTextFrame.cpp:3828
(Diff revision 4)
>  
>  void
>  nsTextPaintStyle::GetHighlightColors(nscolor* aForeColor,
>                                       nscolor* aBackColor)
>  {
> +

nit: unnecessary empty line.

::: layout/generic/nsTextFrame.cpp:3890
(Diff revision 4)
> +      }
> +    }
> +
> +    *aForeColor = foreColor;
> +    *aBackColor = backColor;
> +

nit: looks like redundant empty line.

::: layout/generic/nsTextFrame.cpp:3899
(Diff revision 4)
> +    if (customColors->mAltForeColor) {
> +      if (EnsureSufficientContrast(&foreColor, &backColor)) {

Should be:

if (customColors->mAltForeColor &&
    EnsureSufficientContrast(&foreColor, &backColor)) {

And reduce the indent of following 2 lines.

::: layout/generic/nsTextFrame.cpp:3908
(Diff revision 4)
> +      }
> +    }
> +
> +    *aForeColor = foreColor;
> +    *aBackColor = backColor;
> +

nit: looks like redundant empty line.

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8866000 [details]
Bug 1360500 - Allow custom colors on find selection type selections.

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

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8866000 [details]
Bug 1360500 - Allow custom colors on find selection type selections.

https://reviewboard.mozilla.org/r/137540/#review141970

r+ for the webidl, and random other comments.

::: dom/webidl/Selection.webidl:95
(Diff revision 5)
>  
>    [ChromeOnly,Throws]
>    void scrollIntoView(short aRegion, boolean aIsSynchronous, short aVPercent, short aHPercent);
> +
> +  [ChromeOnly,Throws]
> +  void setColors(DOMString aForeColor, DOMString aBackColor,

aForegroundColor, aBackgroundColor, etc.

::: layout/generic/Selection.h:433
(Diff revision 5)
>    nsCOMArray<nsISelectionListener> mSelectionListeners;
>    nsRevocableEventPtr<ScrollSelectionIntoViewEvent> mScrollEvent;
> -  CachedOffsetForFrame *mCachedOffsetForFrame;
> +  CachedOffsetForFrame* mCachedOffsetForFrame;
>    nsDirection mDirection;
>    SelectionType mSelectionType;
> +  SelectionCustomColors* mCustomColors;

Make this UniquePtr

::: layout/generic/nsFrameSelection.h:51
(Diff revision 5)
>  };
>  
> +struct SelectionCustomColors
> +{
> +#ifdef NS_BUILD_REFCNT_LOGGING
> +  SelectionCustomColors() {

{ goes to its own line

::: layout/generic/nsFrameSelection.h:54
(Diff revision 5)
> +{
> +#ifdef NS_BUILD_REFCNT_LOGGING
> +  SelectionCustomColors() {
> +    MOZ_COUNT_CTOR(SelectionCustomColors);
> +  }
> +  ~SelectionCustomColors() {

ditto

::: layout/generic/nsFrameSelection.h:58
(Diff revision 5)
> +  }
> +  ~SelectionCustomColors() {
> +    MOZ_COUNT_DTOR(SelectionCustomColors);
> +  }
> +#endif
> +  mozilla::Maybe<nscolor> mForeColor;

Can we please use proper names here.
foreground, background etc.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8866000 [details]
Bug 1360500 - Allow custom colors on find selection type selections.

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

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8866000 [details]
Bug 1360500 - Allow custom colors on find selection type selections.

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb95c37e1aed
Allow custom colors on find selection type selections. r=jaws,masayuki,smaug

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fb95c37e1aed

Comment 26

[Hani Yacoub]

Build ID: 20170515030205
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.