Closed Bug 1024930 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S1 (1aug)

People

(Reporter: Carol, Assigned: mtseng)

References

Details

Attachments

(5 files, 5 obsolete files)

Attached image 2014-05-30-06-21-14.png
No description provided.
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
text_caret_tilt_left@1.5x.png
text_caret_tilt_left@2.25x.png
text_caret_tilt_left@2x.png
text_caret_tilt_right.png
text_caret_tilt_right@1.5x.png
text_caret_tilt_right@2.25x.png
text_caret_tilt_right@2x.png
text_caret.png
text_caret@1.5x.png
text_caret@2.25x.png
text_caret@2x.png

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 :)
Flags: needinfo?(mtseng)
Hi George, can Gaia help on this? Thanks.
Flags: needinfo?(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.
Flags: needinfo?(gduan)
Attached patch bug1024930 (obsolete) — Splinter Review
Update to new image and change selection highlight color.
Assignee: nobody → mtseng
Attachment #8452980 - Flags: review?(21)
Flags: needinfo?(mtseng)
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.
Attachment #8452980 - Flags: review?(21) → review?(ehsan)
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.
Attachment #8452980 - Flags: review?(ehsan) → review-
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.
Since Ting-Yu is update image files in bug 1021527, this bug won't update image files.
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?
Flags: needinfo?(chuang)
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!!
Flags: needinfo?(chuang)
Attached patch bug1024930 v2 (obsolete) — Splinter Review
Update to address comment
Attachment #8452980 - Attachment is obsolete: true
Attachment #8456707 - Flags: review?(ehsan)
Hi Morris,
Once you complete the caret design implement, please give me a ui reivew.
Thank you very much! :-)
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!
Attachment #8456707 - Flags: review?(ehsan) → review-
Yap, but per comment 10 UX decide do not change color even if background and text color is similar.
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
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!
(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!
Attached patch bug1024930 v3 (obsolete) — Splinter Review
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.
Attachment #8456707 - Attachment is obsolete: true
Attachment #8460045 - Flags: review?(ehsan)
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!
Attachment #8460045 - Flags: review?(ehsan) → review?(roc)
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
Attachment #8460045 - Flags: review?(roc) → review+
Attached patch bug1024930 v5 (carry r+: roc) (obsolete) — Splinter Review
Fix compiler error and mochitest error on try run.

new try run:  https://tbpl.mozilla.org/?tree=Try&rev=b93ad8487e09
Attachment #8461270 - Attachment is obsolete: true
Looks good!
Keywords: checkin-needed
(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.
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
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
Take background color change out of patch.

try run: https://tbpl.mozilla.org/?tree=Try&rev=b0680e9403b1
Attachment #8461353 - Attachment is obsolete: true
Blocks: 921965
Hi roc, as your suggestion, I set alpha to 255 before calling NS_GetLuminosity.
Attachment #8464501 - Flags: review?(roc)
Please check-in part2, thanks a lot.
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/75723dfa8d02
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S1 (1aug)
You need to log in before you can comment on or make changes to this bug.