This slip my review in bug 1235508. We need to add the pref pref("layout.accessiblecaret.extend_selection_for_phone_number", true); which defaults to false and some comment to all.js so that it could be flip easily on all platforms.
Typo in comment 0. This is what should be added to all.js, which defaults to false. pref("layout.accessiblecaret.extend_selection_for_phone_number", false);
Whiteboard: [good first bug] [lang=js]
hello , i am a new contributor ,how do i start ?
Hi, I would like to work on this.
Created attachment 8746593 [details] [diff] [review] Bug 1265695 - added a call to pref() function I have added the required function call. Could you please tell me a proper comment to add?
Assignee: nobody → vinayakdeepu
Status: NEW → ASSIGNED
If you did a code search for existing uses of that pref , you'd have noticed the commented case where we're turning the pref ON . What we're asking you to provide is a default OFF definition of the pref, which you've realized as your new definition correctly defines if as "false". An appropriate comment (compared to the existing one) for the default OFF case might simply be like: // Smart phone-number selection on long-press is not enabled by default.  http://mxr.mozilla.org/mozilla-central/search?string=pref%28%22layout.accessiblecaret.extend_selection_for_phone_number&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central  http://mxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js?rev=4292da9df16b&mark=906-908#905
A quick step to verify of the correctness of the patch is: 1) Open about:config in the url bar in Firefox desktop browser. It'll show all the preferences in Firefox. 2) Search for the pref name you added. If the pref is added correctly to all.js, you should be able to find it with the default value you set.
Created attachment 8747118 [details] [diff] [review] added a pref and necessary comment tested it by running firefox desktop.
Attachment #8746593 - Attachment is obsolete: true
vinayak, this looks good. Please fix the commit message in the patch, re-attach and ask TYLin for review. I'd approve, and while this is TextSelection related, this isn't close enough to my comfort zone. See  for Checkin-comment rules.  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment
Created attachment 8747426 [details] [diff] [review] Bug 1265695 - added a pref to disable smart phone-number selection on long-press by default. r=TYLin Please review this
Attachment #8747118 - Attachment is obsolete: true
Comment on attachment 8747426 [details] [diff] [review] Bug 1265695 - added a pref to disable smart phone-number selection on long-press by default. r=TYLin Review of attachment 8747426 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch. It looks good to me.
Attachment #8747426 - Flags: review+
BTW, the standard procedure for request a review for a patch is to change the review flag to r?. See  for the steps. And don't forget to set "checkin-needed" to the keywords field. It'll let sheriffs know that this bug has reviewed patches which need to be merged into the tree.  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed
needs rebasing renamed 1265695 -> Bug1265695_v2.diff applying Bug1265695_v2.diff patching file modules/libpref/init/all.js Hunk #1 FAILED at 4986 1 out of 1 hunks FAILED -- saving rejects to file modules/libpref/init/all.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh Bug1265695_v2.diff
I'll help rebase the patch and land it.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.