Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make selection blue instead of orange #10231

Merged
merged 2 commits into from Mar 28, 2016
Merged

Conversation

@paulrouget
Copy link
Contributor

paulrouget commented Mar 28, 2016

PR #10176 add a background color for selected text. It uses a builtin color until ::selection is supported. Orange makes sense for Linux. Let's make it blue for windows and mac. See #10231 (comment)


This change is Reviewable

@highfive
Copy link

highfive commented Mar 28, 2016

warning Warning warning

  • These commits modify layout code, but no tests are modified. Please consider adding a test!
@paulrouget
Copy link
Contributor Author

paulrouget commented Mar 28, 2016

(need to update the test)

@paulrouget
Copy link
Contributor Author

paulrouget commented Mar 28, 2016

So, native widget selection colors are:

  • linux, ubuntu theme, white on orange (but really, it depends on the gtk theme, which is usually blue when it's not ubuntu)
  • osx, black on light blue
  • windows, white on dark blue

And for browserhtml, we need black on light blue.

Until we get some support for querying the OS widget, let's go for what makes sense for browserhtml.

@paulrouget paulrouget changed the title Selection is blue on Windows and Mac, orange on Linux Make selection blue instead of orange Mar 28, 2016
@SimonSapin
Copy link
Member

SimonSapin commented Mar 28, 2016

Until we get some support for querying the OS widget

Unless we have designers making sure everything works well in every possible configuration (which even Firefox apparently doesn’t), I’m strongly against querying the OS for colors. I use the "dark" variation of GNOME’s default theme (texts are white on dark grey backgrounds) but Firefox is utterly broken with that theme: many things end up with unreadable black text on dark background, and dark background buttons and input fields look very out of place in websites with otherwise light background.

The 15-years-old bug on this is https://bugzilla.mozilla.org/show_bug.cgi?id=70315 , still relevant with the recent switch to GTK 3.

To make Firefox usable I launch it with a script that sets an environment variable to select the light GTK theme.

@paulrouget
Copy link
Contributor Author

paulrouget commented Mar 28, 2016

Agreed.

@mbrubeck
Copy link
Contributor

mbrubeck commented Mar 28, 2016

@bors-servo r+


Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2016

📌 Commit 2bbf35f has been approved by mbrubeck

@pcwalton
Copy link
Contributor

pcwalton commented Mar 28, 2016

Maybe make this a pref? (I don't object to the change in this PR though.)

@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2016

Testing commit 2bbf35f with merge cf4fa0a...

bors-servo added a commit that referenced this pull request Mar 28, 2016
Make selection blue instead of orange

PR #10176 add a background color for selected text. It uses a builtin color until `::selection` is supported. ~~Orange makes sense for Linux. Let's make it blue for windows and mac.~~ See #10231 (comment)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10231)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2016

@bors-servo bors-servo merged commit 2bbf35f into servo:master Mar 28, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@paulrouget
Copy link
Contributor Author

paulrouget commented Mar 29, 2016

Do we have the ability to access https://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSColor_Class/#//apple_ref/occ/clm/NSColor/selectedTextColor ?

We could add that to glutin, or have any widget code that would give us these colors for the 3 platforms. We will have similar issues for buttons, checkboxes, text input (etc) shapes and colors.

I personally believe that content should not match the OS colors/shapes. Web developers fight against it all the time and browsers have inconsistent behaviors.

@frewsxcv
Copy link
Member

frewsxcv commented Mar 29, 2016

Fair enough, thanks for the explanation

gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
…lrouget:selectionColor); r=mbrubeck

PR servo/servo#10176 add a background color for selected text. It uses a builtin color until `::selection` is supported. ~~Orange makes sense for Linux. Let's make it blue for windows and mac.~~ See servo/servo#10231 (comment)

Source-Repo: https://github.com/servo/servo
Source-Revision: cf4fa0a9f463f9bdfc5daef7f6bf9d6e1d151aa6

UltraBlame original commit: 95ef72f1a6e7ee93eb4d6cc096da8fbf6a52d90a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…lrouget:selectionColor); r=mbrubeck

PR servo/servo#10176 add a background color for selected text. It uses a builtin color until `::selection` is supported. ~~Orange makes sense for Linux. Let's make it blue for windows and mac.~~ See servo/servo#10231 (comment)

Source-Repo: https://github.com/servo/servo
Source-Revision: cf4fa0a9f463f9bdfc5daef7f6bf9d6e1d151aa6

UltraBlame original commit: 95ef72f1a6e7ee93eb4d6cc096da8fbf6a52d90a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…lrouget:selectionColor); r=mbrubeck

PR servo/servo#10176 add a background color for selected text. It uses a builtin color until `::selection` is supported. ~~Orange makes sense for Linux. Let's make it blue for windows and mac.~~ See servo/servo#10231 (comment)

Source-Repo: https://github.com/servo/servo
Source-Revision: cf4fa0a9f463f9bdfc5daef7f6bf9d6e1d151aa6

UltraBlame original commit: 95ef72f1a6e7ee93eb4d6cc096da8fbf6a52d90a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.