Last Comment Bug 698917 - Be smarter about removing & from prompt labels
: Be smarter about removing & from prompt labels
Status: VERIFIED FIXED
[inbound]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P4 normal (vote)
: ---
Assigned To: Wesley Johnston (:wesj)
:
Mentors:
: 699677 715001 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-01 15:19 PDT by Wesley Johnston (:wesj)
Modified: 2012-01-31 07:16 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Patch (3.37 KB, patch)
2012-01-03 18:47 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch (3.83 KB, patch)
2012-01-05 13:07 PST, Wesley Johnston (:wesj)
mark.finkle: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Wesley Johnston (:wesj) 2011-11-01 15:19:47 PDT
When I was writing the prompt service I saw some of our common prompts included & in the button labels to designate access keys. I took a rather hasty approach and just yanked all "&" symbols from the string, but our prompt service already had a smarter regex/algroythm. 

We should keep using it to format the labels and just throw out the accesskeys for now.
Comment 1 Wesley Johnston (:wesj) 2011-11-01 18:15:29 PDT
mfinkle pointed out on IRC, also need to catch strings at:

http://hg.mozilla.org/projects/birch/file/f853e2b1e753/mobile/components/PromptService.js#l303
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-03 18:13:18 PDT
*** Bug 699677 has been marked as a duplicate of this bug. ***
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-03 18:40:26 PST
*** Bug 715001 has been marked as a duplicate of this bug. ***
Comment 4 Wesley Johnston (:wesj) 2012-01-03 18:47:26 PST
Created attachment 585627 [details] [diff] [review]
Patch

Still testing this
Comment 5 Wesley Johnston (:wesj) 2012-01-05 13:07:39 PST
Created attachment 586190 [details] [diff] [review]
Patch

This works here. I set http://www.maths.surrey.ac.uk/hosted-sites/R.Knott/Fibonacci/fibCalcX.html to calculate 1000 results. The checkbox label should then have its "&" removed.

I'm not removing these from Titles and Messages. Messages likely come from web content and don't expect this. I don't know that title support it either? Context menu list items might need clean up, but I think we control those fairly tightly, and we use the same code path for Select elements, which would be filled in by sites.
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-05 19:37:42 PST
Comment on attachment 586190 [details] [diff] [review]
Patch

Adding the check to getLocaleString seems safe enough from looking at how it is used. I think we need to add a call to cleanupLabel for user-supplied button strings:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/PromptService.js#303

Since we do it in XUL too:
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/components/PromptService.js#450

r+ with that addition
Comment 8 Wesley Johnston (:wesj) 2012-01-10 12:56:35 PST
https://hg.mozilla.org/mozilla-central/rev/af4f9dd24993
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-10 14:40:00 PST
Comment on attachment 586190 [details] [diff] [review]
Patch

[Approval Request Comment]
Needed to help avoid bad strings in the native UI dialogs. Some internal strings can cause the extra "&" but add-ons can too.
Comment 10 christian 2012-01-11 10:04:40 PST
Comment on attachment 586190 [details] [diff] [review]
Patch

[triage comment]
Approved for aurora. Mobile bug.
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-11 20:58:58 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/a47ce0cc4986
Comment 12 Cristian Nicolae (:xti) 2012-01-31 07:16:23 PST
Verified fixed on:

Mozilla/5.0 (Android;Linux armv7l;rv:11.0a2)Gecko/20120130
Firefox/11.0a2 Fennec/11.0a2
Device: Samsung Galaxy S
OS: Android 2.2

Mozilla/5.0 (Android;Linux armv7l;rv:12.0a1)Gecko/20120130
Firefox/12.0a1 Fennec/12.0a1
Device: Samsung Galaxy S
OS: Android 2.2

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