Add pref "layout.accessiblecaret.extend_selection_for_phone_number" to all.js

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: TYLin, Assigned: vinayakdeepu, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [good first bug] [lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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);
This is a fairly simple bug for a new contributor (with basic Javascript skills) to learn the Mozilla development process. Estimated completion time should be easily less than a week.

While this bug completes a bit omitted earlier in an Android/mobile patch, it's results will be viewable in a basic desktop build.

You should be able to build Firefox on your desktop. See [0] for instructions if this is new to you, and ask questions on irc channels #introduction, or #mobile.


[0] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
Mentor: markcapella
Whiteboard: [good first bug] [lang=js]

Comment 3

2 years ago
hello , i am a new contributor ,how do i start ?
(Assignee)

Comment 4

2 years ago
Hi, I would like to work on this.
(Assignee)

Comment 5

2 years ago
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?
Flags: needinfo?(markcapella)
Assignee: nobody → vinayakdeepu
Status: NEW → ASSIGNED
If you did a code search for existing uses of that pref [0], you'd have noticed the commented case where we're turning the pref ON [1].

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.

[0] 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
[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js?rev=4292da9df16b&mark=906-908#905
Flags: needinfo?(markcapella)
(Reporter)

Comment 7

2 years ago
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.
(Assignee)

Comment 8

2 years ago
Created attachment 8747118 [details] [diff] [review]
added a pref and necessary comment

tested it by running firefox desktop.
Attachment #8746593 - Attachment is obsolete: true
Flags: needinfo?(markcapella)
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 [0] for Checkin-comment rules.

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment
Flags: needinfo?(markcapella)
(Assignee)

Comment 10

2 years ago
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
Flags: needinfo?(tlin)
(Reporter)

Comment 11

2 years ago
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+
(Reporter)

Comment 12

2 years ago
BTW, the standard procedure for request a review for a patch is to change the review flag to r?. See [1] 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.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed
Flags: needinfo?(tlin)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
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
Flags: needinfo?(vinayakdeepu)
Keywords: checkin-needed
(Reporter)

Comment 14

2 years ago
I'll help rebase the patch and land it.
Flags: needinfo?(vinayakdeepu)

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1dea6c46f236
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.