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

Bug 1417864 - Don't use the URL keyboard in custom search title #3466

Closed

Conversation

alyssais
Copy link

Previously, CustomSearchEngineTextView set the keyboard type of every text view it created to .URL, which prevented spaces being entered using the keyboard.

This commit changes that text field so that it uses the standard keyboard, but with word-based autocapitalization so custom search titles are title-cased.

Pull Request Checklist

  • My patch has gone through review and I have addressed review comments

  • My patch has a standard commit message that looks like Bug 12345678 - This fixes something something

  • I have updated the Unit Tests to cover new or changed functionality

  • I have updated the UI Tests to cover new or changed functionality

  • I have marked the bug with [needsuplift]

  • I have made sure that localizable strings use NSLocalizableString()

@alyssais alyssais changed the title Don't use the URL keyboard in custom search title Bug 1417864 - Don't use the URL keyboard in custom search title Nov 16, 2017
Copy link
Contributor

@justindarc justindarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch! With your patch right now, I believe titleField technically still is a UIKeyboardType.URL. It might be better to just change CustomSearchEngineTextView.init to add an additional argument keyboardType: UIKeyboardType = .default so you can just specify the keyboardType: .URL argument when initializing urlField. Does that make sense? You can even specify autocapitalizationType = .words for the textField in the initializer as long as it is being created with a keyboardType of .default. Let me know if you have any other questions. I can help get your patch landed once you've made these changes :-)

@alyssais
Copy link
Author

With your patch right now, I believe titleField technically still is a UIKeyboardType.URL.

Don't think so?

(lldb) p titleField.textField.keyboardType
(UIKeyboardType) $R0 = default

Still want me to make those changes? I'm not sure it makes sense to automatically set autocapitalizationType based on keyboardType because nothing about a CustomSearchEngineTextView being the default keyboard type implies that words should necessarily be capitalized, as far as I can see…

@justindarc
Copy link
Contributor

Oh, right because you removed textField.keyboardType = .URL, so .URL is no longer the default value. I still think the changes I requested would make this code easier to follow. CustomSearchEngineTextView is not used anywhere else in the app except for these two text inputs, so it makes more sense to me to allow the resulting text input to be completely configurable from the init() directly as opposed to having to reach in to the CustomSearchEngineTextView after initialization and overriding its configuration.

@alyssais
Copy link
Author

Okay, I'll have a look at making that happen.

Previously, `CustomSearchEngineTextView` set the keyboard type of every
text view it created to `.URL`, which prevented spaces being entered
using the keyboard.

This commit changes that text field so that it uses the standard
keyboard, but with word-based autocapitalization so custom search
titles are title-cased.
@alyssais
Copy link
Author

@justindarc how does that look? Sorry for the delay!

@@ -174,6 +175,10 @@ class CustomSearchEngineTextView: Setting, UITextViewDelegate {
fileprivate let TextLabelHeight: CGFloat = 44
fileprivate var TextFieldHeight: CGFloat = 44

fileprivate let _accessibilityIdentifier: String?
override var accessibilityIdentifier: String? { return _accessibilityIdentifier }
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure if there's a better way to do this. I couldn't find one.

@farhanpatel farhanpatel closed this Apr 8, 2019
@alyssais
Copy link
Author

alyssais commented Apr 9, 2019

Why was this closed? The bug is still open. Has it been fixed in another way?

jjSDET pushed a commit to jjSDET/firefox-ios that referenced this pull request Feb 13, 2024
… for Focus/Klar (mozilla-mobile#3654)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
jjSDET pushed a commit to jjSDET/firefox-ios that referenced this pull request Feb 13, 2024
isabelrios pushed a commit to isabelrios/firefox-ios that referenced this pull request Feb 19, 2024
… for Focus/Klar (mozilla-mobile#3654)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
isabelrios pushed a commit to isabelrios/firefox-ios that referenced this pull request Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants