Last Comment Bug 823639 - Remove spellcheck on URL's in the Edit Bookmark dialog
: Remove spellcheck on URL's in the Edit Bookmark dialog
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Awesomescreen (show other bugs)
: 20 Branch
: ARM Android
: -- normal (vote)
: Firefox 21
Assigned To: Terry
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-20 11:28 PST by Aaron Train [:aaronmt]
Modified: 2013-03-15 21:02 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Here is a first patch that seems to fix the problem (2.68 KB, patch)
2013-01-27 16:23 PST, Terry
sriram.mozilla: review+
Details | Diff | Splinter Review
Screen capture of Auto Review - Only error was line length (45.17 KB, image/png)
2013-01-27 16:25 PST, Terry
no flags Details
Here is a before and after look to show the change in action. (89.17 KB, image/png)
2013-01-27 16:39 PST, Terry
no flags Details
Here is a second patch (removed the redundant Java fix). (1.24 KB, patch)
2013-02-09 16:06 PST, Terry
sriram.mozilla: review+
Details | Diff | Splinter Review
Post-fix, broken again... (101.95 KB, image/png)
2013-03-15 20:59 PDT, Terry
no flags Details
Re-fixed if Java fix included (93.33 KB, image/png)
2013-03-15 21:02 PDT, Terry
no flags Details

Description Aaron Train [:aaronmt] 2012-12-20 11:28:27 PST
Looks like this was overlooked and never filed. Currently the text entry field in the 'Edit Bookmark' dialog has spellcheck enabled. This should probably be disabled.

This bug can probably be mentored as a good first bug with the fix using Java.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBar.java#540

Perhaps using android:inputType="textNoSuggestions"
Comment 1 Terry 2013-01-15 02:22:13 PST
Hi, I am searching for bugs to work on that will double as credit in my Software Engineering course at school. My partner and I decided that Mozilla seems like a good place to start, and this bug seems relatively straight-forward. I was wondering if this issue is still open and whether we can be assigned to work on it. Thank you very much for your time.
Comment 2 Aaron Train [:aaronmt] 2013-01-15 07:08:50 PST
Feel free to take it, it's currently unassigned. I suggest joining #mobile on irc.mozilla.org (IRC) to look for a specific mentor who can assit you if need be.
Comment 3 Terry 2013-01-27 02:41:57 PST
Hello,

Sorry for the delayed reply. It's been very busy.
I think we have the problem fixed, and I am currently looking through many of the guides on how to submit a patch (need to submit our first patch by midnight on 1/28/13).

We have rebuilt Fennec on my device with the changes made and have witnessed the changes in action. We have screenshots and a simple text document showing the changes made to the source code files. It would be great if we could get some assistance in submitting the first patch.

Anyway, that's the update for now. We have another two weeks or so to get it approved, so we will possibly look into related issues and fixes.
Comment 4 Terry 2013-01-27 16:23:06 PST
Created attachment 706913 [details] [diff] [review]
Here is a first patch that seems to fix the problem

In need of reviewer and feedback before proceeding to testing and checking in.
Comment 5 Terry 2013-01-27 16:25:11 PST
Created attachment 706914 [details]
Screen capture of Auto Review - Only error was line length

Other lines in the context of this code were similarly long, so a long line seems okay or hard to avoid.
Comment 6 Terry 2013-01-27 16:39:44 PST
Created attachment 706915 [details]
Here is a before and after look to show the change in action.
Comment 7 Sriram Ramasubramanian [:sriram] 2013-01-28 11:12:08 PST
Comment on attachment 706913 [details] [diff] [review]
Here is a first patch that seems to fix the problem

Review of attachment 706913 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.
r+ after removing the AwesomeBar.java changes.

::: mobile/android/base/AwesomeBar.java
@@ +543,5 @@
>  
>                  final EditText nameText = ((EditText) editView.findViewById(R.id.edit_bookmark_name));
>                  final EditText locationText = ((EditText) editView.findViewById(R.id.edit_bookmark_location));
>                  final EditText keywordText = ((EditText) editView.findViewById(R.id.edit_bookmark_keyword));
> +                locationText.setInputType(InputType.TYPE_TEXT_FLAG_NO_SUGGESTIONS);

This is not needed as you are specifying it in XML. Please remove this.
Comment 8 Daniel Alfaro 2013-01-28 20:44:03 PST
The reason I also specified it in code was that it appears some phones do not obey the flag set in XML.  Specifically HTC phones.  I can remove it if needed though.
Comment 9 Terry 2013-01-28 22:02:37 PST
I'm hoping Danny is right, because I haven't changed anything yet. Please recommend how to proceed, Sriram.
Comment 10 Nick Alexander :nalexander 2013-02-05 17:29:46 PST
(In reply to Daniel Alfaro from comment #8)
> The reason I also specified it in code was that it appears some phones do
> not obey the flag set in XML.  Specifically HTC phones.  I can remove it if
> needed though.

IRC user app0sau5 asked for a second opinion:

17:25 app0sau5: A second opinion on the issue at hand would also help guide us
17:26 app0sau5: the issue being: whether to keep both codes changes in the .java as well as the .xml or not

I agree with Sriram that having this kind of data in both .xml and .java is bad (easy to only change one) and that .xml is to be preferred.  If it is, in fact, not respected on HTC phones (can we get a confirmation of this?) then I could see keeping both and commenting in both places that this is the case.
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2013-02-06 05:02:06 PST
Sriram - Can you check this patch on an HTC phone too? See if the XML is enough.
Comment 12 Terry 2013-02-09 12:12:45 PST
OK, while we still haven't heard back I am going to just revert the change to XML only and resubmit the patch. Until we hear back or somehow confirm the issue with the HTC phones unresponsive to the XML change, we will leave the Java change out.

BTW, we are supposed to have a patch accepted by this upcoming Monday at midnight, so please consider that if you can.

Going to work up the new patch now.

-T
Comment 13 Terry 2013-02-09 16:06:58 PST
Created attachment 712213 [details] [diff] [review]
Here is a second patch (removed the redundant Java fix).
Comment 14 Terry 2013-02-09 16:10:56 PST
I am not sure why it says (First Patch) again next to my name in the patch post.

In any case, it was a simple code change but I am having major issues building for some reason. If someone else could test it on their device to confirm that it is working as expected, then hopefully we can get this approved in time.
Comment 15 Terry 2013-02-09 16:15:45 PST
At what point is this fix actually counted as accepted? I wasn't sure if I was to flag Sriram as a reviewer again since the hover pop-up message said "this patch has already been reviewed" or something to that effect.

Can someone please guide us on how to actually move this bug into a resolved status or is it out of our hands?

Again, acceptance by our Monday night deadline would be nice.

Thank you all for your help.
Comment 16 Aaron Train [:aaronmt] 2013-02-09 17:20:41 PST
Comment on attachment 712213 [details] [diff] [review]
Here is a second patch (removed the redundant Java fix).

You will want your newly attached patch to be reviewed. I have flagged it for review from you.
Comment 17 Sriram Ramasubramanian [:sriram] 2013-02-09 21:52:33 PST
Comment on attachment 712213 [details] [diff] [review]
Here is a second patch (removed the redundant Java fix).

Review of attachment 712213 [details] [diff] [review]:
-----------------------------------------------------------------

Oops. I missed following this mail as I had r+ ed already.
This looks good to me.
Comment 18 Terry 2013-02-09 23:07:48 PST
Thank you for the review!

Now, how do we move this to accepted/resolved? May I take the liberty of changing the status of this bug to resolved?

Thanks again for everyone's help. It's been quite an experience despite the small amount of code changed.
Comment 19 Mark Finkle (:mfinkle) (use needinfo?) 2013-02-10 05:54:46 PST
(In reply to Terry from comment #18)
> Thank you for the review!
> 
> Now, how do we move this to accepted/resolved? May I take the liberty of
> changing the status of this bug to resolved?

We wait until the code has landed before resolving to FIXED
 
> Thanks again for everyone's help. It's been quite an experience despite the
> small amount of code changed.

Thanks for working on this. What can we interest you in working next? :)
Comment 20 Ryan VanderMeulen [:RyanVM] 2013-02-10 10:36:08 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/50153480218c

Thanks for the patch, Terry!
Comment 21 Terry 2013-02-10 11:25:26 PST
Ah, thank you for that link, RyanVM. I believe that should be enough to show that our patch was "accepted" for the purpose of our class project deadline.

One last thing: would it be too much trouble to add an author to the patch file? Would I have to resubmit a patch?
Comment 22 Terry 2013-02-10 11:56:54 PST
Scratch that last bit. The author section of the header should also list my team member Danny Alfaro, but after a quick IRC chat it was determined:

<jfkthame>:given the size of the patch involved, it seems to me like it'd be more trouble than is justified - for future reference, it's best if you ensure the patch headers (author, commit message) are complete when attaching for review and potential landing

It is my mistake as I forgot to make that manual edit after finally getting it to work right, and it was checked-in before I knew it.

Finally, the second bug for our project is Bug #708765
https://bugzilla.mozilla.org/show_bug.cgi?id=708765

That one seems quite a bit more involved with the actual code, and now that we're acquainted with the whole process, it should be fun. If you would like to keep helping us, you will find us actively tackling that bug.

Thanks again, everyone. If we're lucky and finish the second one a bit early we might even take on a third before the quarter is over :)
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-02-11 11:18:28 PST
https://hg.mozilla.org/mozilla-central/rev/50153480218c
Comment 24 Andreea Pod 2013-03-13 01:02:15 PDT
I'm not able to reproduce this on the latest Nightly. Close this bug as verified fixed.
-build: Firefox for Android 22.0a1 (2013-03-12)
-device: Samsung Galaxy Nexus
-OS: Android 4.1.1
Comment 25 Terry 2013-03-15 20:51:02 PDT
Well, through some unfortunate chain of events, this bug reappeared on my Google Nexus 7 (running Android 4.2.2) when using the most recent SwiftKey and SwiftKey Tablet keyboards. Particularly unfortunate, because today was demo day in class :( I was not sure whether to re-patch this bug or create a new one since I don't see any ways to change the status to re-opened.

Anyway, I have attached a couple screen shots of this behavior, and have a new patch ready with the Java code fix. It's technically the same as the original patch that was deemed redundant, but now it seems like we have proof that it may be needed. I am positive the XML fix worked on my tablet at some point, and I am positive that it no longer works unless I include the Java fix.

Please advise on how to proceed.
Comment 26 Terry 2013-03-15 20:59:56 PDT
Created attachment 725708 [details]
Post-fix, broken again...

Screenshot of my Google N7 after the fix with a clean install.
Comment 27 Terry 2013-03-15 21:02:31 PDT
Created attachment 725709 [details]
Re-fixed if Java fix included

I included the original "redundant" Java fix, and it is once again fixed.

Note You need to log in before you can comment on or make changes to this bug.