1024930 - [Text Selection] Text Highlighting color need to change to blue

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[Carol Huang [:Carol]]

Attachment: 2014-05-30-06-21-14.png

Comment 1

[Carol Huang [:Carol]]

Attachment: Text_Selection_Visual Spec.zip

Hi Morris,
In order to make the selection caret easier to use, I revised the caret images.
Please help me update these images below:
text_caret_tilt_left.png
[e-mail]
[e-mail]
[e-mail]
text_caret_tilt_right.png
[e-mail]
[e-mail]
[e-mail]
text_caret.png
[e-mail]
[e-mail]
[e-mail]

One other thing is that the hightlighting color looks different from the spec, please help me change it to #33b5e5, 40%. Thanks!
Let me know when you finish these adjustment so I can have a UI review :)

Comment 2

[howie [:howie]]

Hi George, can Gaia help on this? Thanks.

Comment 3

[(Inactive after June) George Duan [:gduan] [:喬智]]

Hi Howie,
those caret are drawn by gecko currently.

Hi Morris,
will our caret's image changed by device type? Some highly resolution device (like frame) requires to apply @1+ images.

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: bug1024930

Update to new image and change selection highlight color.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8452980 [details] [diff] [review]
bug1024930

Review of attachment 8452980 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a bit scared to change the text selection color to a defined color as I'm not sure how it will looks depending on the background color of the page. IIRC there are some funny things done in Gecko to normally adjust the text selection color based on the text color / background of the page.

Also I can't really review things in editor/composer. Let's defer the review to Ehsan which is likely a better fit for this part of the code.

Comment 6

[(no longer active)]

Comment on attachment 8452980 [details] [diff] [review]
bug1024930

Review of attachment 8452980 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/chrome/content/content.css
@@ +295,5 @@
>  }
>  
> +::-moz-selection {
> +  background: rgba(51, 181, 229, 0.4);
> +}

The correct way to modify this is through the gonk widget code.  See nsLookAndFeel::NativeGetColor in widget/gonk/nsLookAndFeel.cpp.  eColorID_TextSelectForeground denotes the text color for selected text, and eColorID_TextSelectBackground denotes the background color for the selected text.

Note that according to the UX spec, selected text should not change its color.  This is similar to how our Mac widget layer behaves (because that is the native platform convention on Mac.)  The trick we use is we return the value NS_DONT_CHANGE_COLOR from NativeGetColor.  But you need to make sure that the text and its background are never the same exact color (ideally we should also not paint the selected text and background using colors that are too similar to each other too, but that is another bug).  See the nsLookAndFeel.mm implementation because you basically need to copy the same idea.

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8452980 [details] [diff] [review]
bug1024930

Review of attachment 8452980 [details] [diff] [review]:
-----------------------------------------------------------------

Since we are going to enable touch caret and selection carets in 2.1, I would like to change those png images to svg in bug 1021527. It could solve part of the blurry issue when pinch-zooming in, and make the ua.css rule simpler.

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Since Ting-Yu is update image files in bug 1021527, this bug won't update image files.

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Hi Carol,
I have to make sure some details about spec before implementing.

1. Do we change text foreground color when this text is selected?

If yes, next question is
2.1 What is this foreground color?

If no, next question is
2.2 What happen if the text foreground color is same with background color?

Comment 10

[Carol Huang [:Carol]]

Hi Morris,
As our offline discussion, we don't change the text color when the text selected.
The highlighting has transparency so I think it would still work when it's on similar background color.
Please follow the visual spec first and if for most the case aren't working, let's adjust it.
Thank you!!

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: bug1024930 v2

Update to address comment

Comment 12

[Carol Huang [:Carol]]

Hi Morris,
Once you complete the caret design implement, please give me a ui reivew.
Thank you very much! :-)

Comment 13

[(no longer active)]

Comment on attachment 8456707 [details] [diff] [review]
bug1024930 v2

Review of attachment 8456707 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't address the last part of comment 6:

"But you need to make sure that the text and its background are never the same exact color (ideally we should also not paint the selected text and background using colors that are too similar to each other too, but that is another bug).  See the nsLookAndFeel.mm implementation because you basically need to copy the same idea."

But otherwise it looks good!

Comment 14

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Yap, but per comment 10 UX decide do not change color even if background and text color is similar.

Comment 15

[(no longer active)]

(In reply to Morris Tseng [:mtseng] from comment #14)
> Yap, but per comment 10 UX decide do not change color even if background and
> text color is similar.

That is a very strange decision and I would like to push back on it.  The text background and foreground color being the same makes the text completely invisible, and we should definitely not implement something like that.  The discussion prior to comment 10 is not visible on this bug but if you really want to do this, please attach a screenshot from a test case with text color being set to rgb(0x33,0xb5,0xe5).  I am not sure if the transparency of the highlight will take care of things.  If the transparency indeed proves insufficient then we need to revise the UX spec.

Comment 16

[Carol Huang [:Carol]]

Attachment: text_selection situation.jpg

Hi Morris,
For general situation, the text color won't change when it's selected. 
But if the text background and foreground color being the same, please change the color we have now (red) to #4d4d4d.(see attachment) Thank you!

Comment 17

[(no longer active)]

(In reply to Carol Huang [:Carol] from comment #16)
> Created attachment 8459432 [details]
> text_selection situation.jpg
> 
> Hi Morris,
> For general situation, the text color won't change when it's selected. 
> But if the text background and foreground color being the same, please
> change the color we have now (red) to #4d4d4d.(see attachment) Thank you!

Thanks, this sounds good!

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: bug1024930 v3

Ehsan, I cannot simply compare color at nsLookAndFeel because I can't get frame color there. So I slightly modify nsTextFrame.cpp to let it return specific color if foreground and back ground color are the same.

Comment 19

[(no longer active)]

Comment on attachment 8460045 [details] [diff] [review]
bug1024930 v3

Review of attachment 8460045 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me, except maybe with a better name for eColorID_TextSelectForegroundCustom, but I'd like roc to take a look at this as well.  Thanks, Morris!

Comment 20

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

Comment on attachment 8460045 [details] [diff] [review]
bug1024930 v3

Review of attachment 8460045 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/LookAndFeel.h
@@ +588,5 @@
>  // (ie. a colored text keeps its colors  when selected).
>  // Of course if other plaforms work like the Mac, they can use it too.
>  #define NS_DONT_CHANGE_COLOR 	NS_RGB(0x01, 0x01, 0x01)
>  
> +// Similar with NS_DONT_CHNAGE_COLOR, except NS_DONT_CHANGE_COLOR would returns

fix typo

Comment 21

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: bug1024930 v4 (carry r+: roc)

fix typo
try run: https://tbpl.mozilla.org/?tree=Try&rev=156d3f019c8a

Comment 22

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: bug1024930 v5 (carry r+: roc)

Fix compiler error and mochitest error on try run.

new try run:  https://tbpl.mozilla.org/?tree=Try&rev=b93ad8487e09

Comment 23

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Looks good!

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3ee28802c28c

Comment 25

[Ed Morley [:emorley]]

Either this or the other commit in the same push (https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=de34cb8506d9) caused B2G mochitest-debug-14 failures, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=44517496&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=44517920&tree=B2g-Inbound

So I've had to revert the push:
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/73c90712e409
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/ba4d556f908e

Comment 26

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Ed Morley [:edmorley] from comment #25)
> Either this or the other commit in the same push
> (https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=de34cb8506d9) caused B2G
> mochitest-debug-14 failures, eg:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=44517496&tree=B2g-Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=44517920&tree=B2g-Inbound
> 
> So I've had to revert the push:
> remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/73c90712e409
> remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/ba4d556f908e

My patch causes this failed, I'll figure why it failed. Sorry for backout.

Comment 27

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

I found transparent background color would trigger an assertion when getting luminosity [1]. Does it mean we cannot use transparent background color?

[1]: http://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSColorUtils.cpp#114

Comment 28

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

Why are we hitting the assertion? I assume it's NS_RGBA(0x33,0xb5,0xe5,0x66) for eColorID_TextSelectBackground and we're calling NS_LUMINOSITY_DIFFERENCE on it?

I suggest you take that change out of this patch, land the patch, and write a separate patch to change eColorID_TextSelectBackground. We'll need to think about how to handle this, but my guess is that it will be OK to just make NS_LUMINOSITY_DIFFERENCE just set the alpha value to 255 before calling NS_GetLuminosity.

Comment 29

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: bug1024930 v6 (carry r+: roc)

Take background color change out of patch.

try run: https://tbpl.mozilla.org/?tree=Try&rev=b0680e9403b1

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8c1ea12e7faf

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c1ea12e7faf

Comment 32

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part2: Change text hightlighting background color

Hi roc, as your suggestion, I set alpha to 255 before calling NS_GetLuminosity.

Comment 33

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

try run: https://tbpl.mozilla.org/?tree=Try&rev=3463c668842a

Comment 34

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Please check-in part2, thanks a lot.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/75723dfa8d02

Please try to be conscientious about the platforms and test suites you choose to run on your Try pushes. "All" runs like the above consume over 300hr of machine and contribute to long backlogs experienced by all developers. Thanks!
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75723dfa8d02