Bug 1177597 & 1183210 - Search screen does not show on iOS9 by codestergit · Pull Request #715 · mozilla-mobile/firefox-ios

codestergit · 2015-07-12T14:45:31Z

In iOS9 due to some bug UIKeyInput method insertText is not calling.
This commit resolve the issue.

thebnich · 2015-07-13T17:17:41Z

Nit: Chracters -> Characters

thebnich · 2015-07-13T18:56:15Z

Can you explain these changes, maybe adding some comments? I understand the insertText -> SELtextDidChange if insertText isn't working, but I'm not clear on what all the other changes are doing.

codestergit · 2015-07-14T14:15:06Z

Removes autocompletion before changing any characters.

codestergit · 2015-07-14T14:33:16Z

@thebnich added comments. We are doing same thing as previous. In insertText method before inserting any characters we are removing autocompletion. After inserting characters(i.e call to super.insertText) we are calculating autocompletion suggestion.

Please let me know if any further clarification needed.

thebnich · 2015-07-14T23:41:05Z

This seems to work well on English keyboards, though I found a regression switching to Japanese.

Steps:

Switch to Japanese keyboard.
Click URL bar.
Type "ha".

This should show the Hiragana symbol for "ha" (あ); however, it shows hあ, so it looks like this change is breaking text composition somehow. I filed bug 1183914 to get some automated tests for non-English text entry -- it's very fragile!

codestergit · 2015-07-17T17:57:17Z

@thebnich updated the pull request. Fixed flicker issue and Japanese character issue. Please let me know your thoughts and if any clarification needed.

codestergit · 2015-07-22T16:47:00Z

Hi @thebnich,
updated the pull request to resolve conflicts. Now can be merged.

codestergit · 2015-07-22T17:21:13Z

Hi @thebnich It also resolve Bug-1183210 flicker issue while entering text.

codestergit · 2015-07-22T18:15:25Z

@thebnich removed debounce added for Bug 1173162 . Done changes for hitting db when required i.e if user enter text does not match with saved suggestion.

codestergit · 2015-07-23T15:54:44Z

@thebnich updated the pull request. Resolved conflicts.

codestergit · 2015-07-23T19:43:40Z

@thebnich updated the pull request to address comments. I have made separate commit for that and squashed in to "Hitting history db only when required". I hope that is fine :) .

codestergit · 2015-07-24T05:08:22Z

Hi @thebnich
Due to lag between shouldChangeCharactersInRange and textDidChange event we need to do this. Detailed comment is here. I have already tried to find some alternative but no success. I need your thoughts further. Thanks for all your help :)

codestergit · 2015-07-24T20:18:37Z

@thebnich
Updated the pull request. Thanks for your suggestion. I have simplified the patch.
Thanks

codestergit · 2015-07-26T14:09:49Z

Hi @thebnich
Updated pull request and resolved conflicts.
During testing I have found two issues which is regression of Bug-1166781.
I have done separate pull request for them also.
#807
#808

thebnich · 2015-07-29T17:47:12Z

Do we need this change? I think it's safe to just use self.text here.

@thebnich Here we need enteredText because notifyTextChanged is also called from SELtextDidChange may be after some delay. So self.text may have complete string with auto completion, not the user entered text. So cached query in SearchLoader may save auto- completed string and may give bad results.

OK. I see we are doing this enteredText calculation in lots of places. Maybe it would make sense to just store enteredText directly as a class variable, and remove enteredTextLength? If there are places we still need the entered text length, we could just use count(enteredText). And then we don't need to use substringToIndex everywhere.

@thebnich You always know what is best :).
Changed enteredTextLength to entredText working good.
As we are not calculating enteredTextLength when not in auto-completion mode so if user enters text in between somewhere than it will be calculated in SELtextDidChange so enteredText will work good. Thanks for your suggestion.

thebnich · 2015-07-29T18:07:51Z

Nit: } else if ... { on same line

thebnich · 2015-07-29T18:26:54Z

How about we return early here and remove the if else below? Then, at the very bottom of this function outside of all the if statements, you do removeCompletion? That way, if the setting the suggestion fails for any reason, we always clear the existing completion if there is one.

Thanks great 👍.
Done that

codestergit · 2015-07-29T20:28:33Z

@thebnich Thanks for suggestion. Updated the pull request.
Please let me know if any other changes required.

Bug 1177597 & 1183210 - Search screen does not show on iOS9

Prepare for 420

* Start building add search engine settings screen * add persisting custom search engines * Add search engine editing * add image * a few changes to the custom search engine screens * Add placeholder to UITextView * Fix image display for custom search engines * address ui issues * address ui issues * Add UI Tests * adjust selected bg color and seperator * change selected engine color * remove unfinished tests and switch to leading

thebnich reviewed Jul 13, 2015
View reviewed changes

Comment thread Client/Frontend/Widgets/AutocompleteTextField.swift Outdated

Copy link
Copy Markdown

Contributor

thebnich Jul 13, 2015

Choose a reason for hiding this comment

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

Nit: Chracters -> Characters

codestergit force-pushed the Bug-117759 branch 2 times, most recently from 61b2b21 to 1429b8b Compare July 14, 2015 13:09

codestergit reviewed Jul 14, 2015
View reviewed changes

codestergit force-pushed the Bug-117759 branch 3 times, most recently from 1f75631 to 25f6982 Compare July 22, 2015 16:45

codestergit changed the title ~~Bug 1177597 - Search screen does not show on iOS9~~ Bug 1177597 & 1183210 - Search screen does not show on iOS9 Jul 22, 2015

codestergit force-pushed the Bug-117759 branch 2 times, most recently from 19036fe to 87a2eb4 Compare July 23, 2015 15:52

codestergit force-pushed the Bug-117759 branch 7 times, most recently from 8228108 to 187b23a Compare July 23, 2015 19:41

codestergit force-pushed the Bug-117759 branch from 187b23a to 93ea935 Compare July 24, 2015 20:15

codestergit force-pushed the Bug-117759 branch 2 times, most recently from a414cb4 to e035a98 Compare July 26, 2015 14:02

codestergit force-pushed the Bug-117759 branch from e035a98 to 1028b77 Compare July 29, 2015 17:46

thebnich reviewed Jul 29, 2015
View reviewed changes

Bug 1177597 - Search screen does not show on iOS9

86ff96e

thebnich reviewed Jul 29, 2015
View reviewed changes

Comment thread Client/Frontend/Widgets/AutocompleteTextField.swift Outdated

Copy link
Copy Markdown

Contributor

thebnich Jul 29, 2015

Choose a reason for hiding this comment

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

Nit: } else if ... { on same line

codestergit force-pushed the Bug-117759 branch 2 times, most recently from 7ede6e0 to 7441cdc Compare July 29, 2015 18:25

thebnich reviewed Jul 29, 2015
View reviewed changes

codestergit force-pushed the Bug-117759 branch 2 times, most recently from 4482355 to 82df592 Compare July 29, 2015 20:14

codestergit force-pushed the Bug-117759 branch from 82df592 to e8b3825 Compare July 29, 2015 21:19

codestergit added 2 commits July 30, 2015 02:50

Fix for flicker issue

1fefe9c

Fix for multistage input character issue.

e8b3825

thebnich added a commit that referenced this pull request Jul 29, 2015

Merge pull request #715 from codestergit/Bug-117759

f496e89

Bug 1177597 & 1183210 - Search screen does not show on iOS9

thebnich merged commit f496e89 into mozilla-mobile:master Jul 29, 2015

ecotopian referenced this pull request in ecosia/ios-browser Mar 1, 2022

Merge pull request #715 from danger/v420

26bd9fb

Prepare for 420

Conversation

codestergit commented Jul 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thebnich commented Jul 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codestergit commented Jul 14, 2015

Uh oh!

thebnich commented Jul 14, 2015

Uh oh!

codestergit commented Jul 17, 2015

Uh oh!

codestergit commented Jul 22, 2015

Uh oh!

codestergit commented Jul 22, 2015

Uh oh!

codestergit commented Jul 22, 2015

Uh oh!

codestergit commented Jul 23, 2015

Uh oh!

codestergit commented Jul 23, 2015

Uh oh!

codestergit commented Jul 24, 2015

Uh oh!

codestergit commented Jul 24, 2015

Uh oh!

codestergit commented Jul 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codestergit commented Jul 29, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants