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 :)

Hi George, can Gaia help on this? Thanks.

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.

Update to new image and change selection highlight color.

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 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 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?

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!!

Update to address comment

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!

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

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

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!

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 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 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

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

Fix compiler error and mochitest error on try run. new try run: https://tbpl.mozilla.org/?tree=Try&rev=b93ad8487e09

Looks good!

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

(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

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.

Take background color change out of patch. try run: https://tbpl.mozilla.org/?tree=Try&rev=b0680e9403b1

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

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

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