Closed Bug 823639 Opened 7 years ago Closed 7 years ago

Remove spellcheck on URL's in the Edit Bookmark dialog

Categories

(Firefox for Android :: Awesomescreen, defect)

20 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 21

People

(Reporter: aaronmt, Assigned: toxophiliterry)

Details

Attachments

(5 files, 1 obsolete file)

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"
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.
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.
Assignee: nobody → toxophiliterry
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.
Status: NEW → ASSIGNED
In need of reviewer and feedback before proceeding to testing and checking in.
Other lines in the context of this code were similarly long, so a long line seems okay or hard to avoid.
Attachment #706913 - Flags: review?(sriram)
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.
Attachment #706913 - Flags: review?(sriram) → review+
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.
I'm hoping Danny is right, because I haven't changed anything yet. Please recommend how to proceed, Sriram.
(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.
Sriram - Can you check this patch on an HTC phone too? See if the XML is enough.
Flags: needinfo?(sriram)
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
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.
Attachment #706913 - Attachment is obsolete: true
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 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.
Attachment #712213 - Flags: review?(sriram)
Flags: needinfo?(sriram)
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.
Attachment #712213 - Flags: review?(sriram) → review+
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.
(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? :)
Keywords: checkin-needed
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?
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 :)
https://hg.mozilla.org/mozilla-central/rev/50153480218c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
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
Status: RESOLVED → VERIFIED
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.
Screenshot of my Google N7 after the fix with a clean install.
I included the original "redundant" Java fix, and it is once again fixed.
You need to log in before you can comment on or make changes to this bug.