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

Fix placeholders for password inputs #9072

Merged
merged 3 commits into from Jan 3, 2016

Conversation

@Manishearth
Copy link
Member

Manishearth commented Dec 26, 2015

currently they show dots instead of the placeholder text

Review on Reviewable

currently they show dots
@eefriedman
Copy link
Contributor

eefriedman commented Dec 26, 2015

I think this messes up get_insertion_point_index_for_layout; it expects to see the value before the mapping to dots.

Can we reftest this? Maybe even just <input type=password placeholder=xxx> == <input type=text placeholder=xxx>.

@eefriedman eefriedman self-assigned this Dec 26, 2015
@Manishearth
Copy link
Member Author

Manishearth commented Dec 27, 2015

The comment in get_insertion_point_index_for_layout mentions that we just need a 1:1 char mapping, which we have.

Yes, I will add a reftest.

@Manishearth
Copy link
Member Author

Manishearth commented Dec 27, 2015

Added a reftest

@Manishearth Manishearth force-pushed the Manishearth:password-placeholder branch from d8211f6 to 728ec11 Dec 27, 2015
@Manishearth
Copy link
Member Author

Manishearth commented Dec 27, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Dec 27, 2015

Trying commit 728ec11 with merge ec60957...

bors-servo added a commit that referenced this pull request Dec 27, 2015
Fix placeholders for password inputs

currently they show dots instead of the placeholder text

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

bors-servo commented Dec 27, 2015

💔 Test failed - mac-rel-wpt

@Manishearth
Copy link
Member Author

Manishearth commented Dec 27, 2015

(failed due to an intermittent)

@eefriedman
Copy link
Contributor

eefriedman commented Dec 27, 2015

get_insertion_point_index_for_layout calls char_indices on the result of get_raw_textinput_value. See #8265.

@Manishearth
Copy link
Member Author

Manishearth commented Dec 28, 2015

Given that layout only gets the dots, shouldn't it be calculating the insertion point based on the dots and not the actual text? In case the text contains non-ascii values this could get messed up.

@Manishearth
Copy link
Member Author

Manishearth commented Dec 28, 2015

Pushed another commit which takes a different approach and moves the placeholder logic out of get_raw_textinput_value (it doesn't really belong there anyway?). It shouldn't affect get_raw_textinput_value since the placeholder is only there on an empty string, so the insertion point has to be zero.

@eefriedman
Copy link
Contributor

eefriedman commented Dec 29, 2015

Did you mean to make some other changes to get_raw_textinput_value, like removing the check for an empty string, or removing the mapping to dots?

@Manishearth Manishearth force-pushed the Manishearth:password-placeholder branch from 63ebbc9 to be6caf6 Dec 31, 2015
@Manishearth
Copy link
Member Author

Manishearth commented Dec 31, 2015

My bad, updated.

@eefriedman
Copy link
Contributor

eefriedman commented Jan 3, 2016

Missing changes to get_raw_textinput_value?

@Manishearth Manishearth force-pushed the Manishearth:password-placeholder branch from be6caf6 to cc5844a Jan 3, 2016
@Manishearth
Copy link
Member Author

Manishearth commented Jan 3, 2016

Done

@eefriedman
Copy link
Contributor

eefriedman commented Jan 3, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2016

📌 Commit cc5844a has been approved by eefriedman

@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2016

Testing commit cc5844a with merge 7f156b8...

bors-servo added a commit that referenced this pull request Jan 3, 2016
Fix placeholders for password inputs

currently they show dots instead of the placeholder text

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

bors-servo commented Jan 3, 2016

@bors-servo bors-servo merged commit cc5844a into servo:master Jan 3, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Manishearth Manishearth deleted the Manishearth:password-placeholder branch Jan 3, 2016
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

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