Closed Bug 407584 Opened 17 years ago Closed 17 years ago

The auto appending accesskey is in wrong position when an ellipsis exists at non-tail of the caption

Categories

(Core :: XUL, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
if |intl.menuitems.alwaysappendaccesskeys| is true, the accesskey of the menu is always append to the captions. But when an ellipsis exists at non-tail of the caption, the accesskey label is inserted to wrong position

if the caption is |Search Google for "..."| and the accesskey is 's':

Actual result: |Search Google for "(S)..."|

Expected result: |Search Google for "..."(S)|

The attached patch is created by Kazuyoshi Kato-san (kzys@8-p.info).
Thank you for your work!
Flags: blocking1.9?
Attachment #292311 - Flags: superreview?(bzbarsky)
Attachment #292311 - Flags: review?(bzbarsky)
Comment on attachment 292311 [details] [diff] [review]
Patch

I'm not really comfortable reviewing this, and as a result not sure when I would be able to get to it.  Perhaps one of the Neils would be a better choice?
Attachment #292311 - Flags: superreview?(neil)
Attachment #292311 - Flags: superreview?(bzbarsky)
Attachment #292311 - Flags: review?(neil)
Attachment #292311 - Flags: review?(bzbarsky)
Comment on attachment 292311 [details] [diff] [review]
Patch

>     const nsDependentString kEllipsis = nsContentUtils::GetLocalizedEllipsis();
>-    PRInt32 offset = mTitle.RFind(kEllipsis);
>-    if (offset == kNotFound) {
>+    PRInt32 offset = kNotFound;
>+    if (StringEndsWith(mTitle, kEllipsis)) {
>+         offset = mTitle.Length() - kEllipsis.Length();
>+    } else {
>         // Try to check with our old ellipsis (for old addons)
>-        if (!kEllipsis.Equals(OLD_ELLIPSIS))
>-            offset = mTitle.RFind(OLD_ELLIPSIS);
>-        if (offset == kNotFound) {
>+        if (StringEndsWith(mTitle, OLD_ELLIPSIS)) {
>+            offset = mTitle.Length() - OLD_ELLIPSIS.Length();
>+        } else {
>             // Try to check with our default ellipsis (for non-localized addons)
>-            nsAutoString defaultEllipsis(PRUnichar(0x2026));
>-            if (!kEllipsis.Equals(defaultEllipsis))
>-                offset = mTitle.RFind(defaultEllipsis);
>+            const nsAutoString kDefaultEllipsis(PRUnichar(0x2026));
>+            if (StringEndsWith(mTitle, kDefaultEllipsis)) {
>+                offset = mTitle.Length() - kDefaultEllipsis.Length();
>+            }
>         }
>     }
>+
>     if (offset == kNotFound) {
>         offset = (PRInt32)mTitle.Length();
>         if (mTitle.Last() == PRUnichar(':'))
>             offset--;
>     }
Just looking at this block shows me that you can improve on the previous test, but in fact you can merge the two blocks because they're both doing the same thing - looking for a trailing substring. Start with offset as the length (and you'll be able to make it a PRUint32 now, because you're not doing a RFind any more), then check each of the four substrings/characters in turn. I'd also suggest using } else if { to cut down on the indentation.
Attachment #292311 - Flags: superreview?(neil)
Attachment #292311 - Flags: superreview-
Attachment #292311 - Flags: review?(neil)
Attached patch Patch #2 (obsolete) — Splinter Review
Kato-san's work.
Attachment #292311 - Attachment is obsolete: true
Attachment #292419 - Flags: superreview?(neil)
Attachment #292419 - Flags: review?(neil)
Comment on attachment 292419 [details] [diff] [review]
Patch #2

(Sorry if these weren't clear in my previous comment.)

>+    PRUint32 offset = mTitle.Length();

>+        offset = mTitle.Length() -
You can use -= as you already have the length.

>-        if (mTitle.Last() == PRUnichar(':'))
>-            offset--;
>+    } else if (StringEndsWith(mTitle, NS_LITERAL_STRING(":"))){
>+        offset = mTitle.Length() - 1;
Last() and -- is better than StringEndsWith() for a single character.
Attachment #292419 - Flags: superreview?(neil)
Attachment #292419 - Flags: superreview-
Attachment #292419 - Flags: review?(neil)
Attached patch Patch #3Splinter Review
Attachment #292419 - Attachment is obsolete: true
Attachment #292730 - Flags: superreview?(neil)
Attachment #292730 - Flags: review?(neil)
Attachment #292730 - Flags: superreview?(neil)
Attachment #292730 - Flags: superreview+
Attachment #292730 - Flags: review?(neil)
Attachment #292730 - Flags: review+
Comment on attachment 292730 [details] [diff] [review]
Patch #3

Thank you Neil. And let's land this to 1.9. This patch is very low risky.
Attachment #292730 - Flags: approval1.9?
Attachment #292730 - Flags: approval1.9? → approval1.9+
checked-in, thank you Kato-san!!
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: blocking1.9?
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Depends on: 564705
You need to log in before you can comment on or make changes to this bug.