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

Only display text carets in text inputs #7764

Merged
merged 1 commit into from Sep 29, 2015

Conversation

@j3parker
Copy link
Contributor

j3parker commented Sep 27, 2015

For #7756

Review on Reviewable

@highfive
Copy link

highfive commented Sep 27, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!
@pcwalton
Copy link
Contributor

pcwalton commented Sep 27, 2015

Can we reftest this?

@pcwalton
Copy link
Contributor

pcwalton commented Sep 27, 2015

For example, by focusing a submit button with JS and checking to make sure no caret shows up.

@j3parker j3parker force-pushed the j3parker:input-caret-only-for-text branch from 189052a to 9177530 Sep 27, 2015
@j3parker
Copy link
Contributor Author

j3parker commented Sep 27, 2015

I can do that!

I can't think of a good way to reftest that there is no caret in the buttons at all (i.e. that reftest seems like it'd still pass before these changes)

@pcwalton
Copy link
Contributor

pcwalton commented Sep 27, 2015

Would it still pass even if you focus the buttons with JS?

@j3parker j3parker force-pushed the j3parker:input-caret-only-for-text branch from 9177530 to 9cf4387 Sep 27, 2015
@j3parker
Copy link
Contributor Author

j3parker commented Sep 27, 2015

@pcwalton is this what you had in mind? It fails before the change and passes after. I had to add some css to make them match because of servo.css; I'm not sure if that's cool. I tried to do it in the least opinionated way I could.

@j3parker
Copy link
Contributor Author

j3parker commented Sep 27, 2015

(The focusing is unnecessary for the test, though... the caret shows up even unfocused)

@pcwalton
Copy link
Contributor

pcwalton commented Sep 29, 2015

@j3parker That looks great, thank you!

@pcwalton
Copy link
Contributor

pcwalton commented Sep 29, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

📌 Commit 9cf4387 has been approved by pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

Testing commit 9cf4387 with merge b7af011...

bors-servo pushed a commit that referenced this pull request Sep 29, 2015
Only display text carets in text inputs

For #7756

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7764)
<!-- Reviewable:end -->
@metajack
Copy link
Contributor

metajack commented Sep 29, 2015

@bors-servo retry

  • build canceled while addressing infra issues
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

Testing commit 9cf4387 with merge b723bf8...

bors-servo pushed a commit that referenced this pull request Sep 29, 2015
Only display text carets in text inputs

For #7756

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7764)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

💔 Test failed - mac-rel-wpt

@jdm
Copy link
Member

jdm commented Sep 29, 2015

@bors-servo: retry

@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

Testing commit 9cf4387 with merge 0c64e4a...

bors-servo pushed a commit that referenced this pull request Sep 29, 2015
Only display text carets in text inputs

For #7756

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7764)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

@bors-servo bors-servo merged commit 9cf4387 into servo:master Sep 29, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
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.