Open Bug 519921 Opened 11 years ago Updated 1 year ago

Allow accelerator text hint to be displayed in tooltips (like in menuitems)

Categories

(Core :: XUL, enhancement)

enhancement
Not set

Tracking

()

People

(Reporter: ehsan, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (v1) (obsolete) — Splinter Review
Things like bug 513168 need accelerator text hints for some commands to be shown in control tooltips, instead of in menu items.  The attached patch adds a new attribute to XUL elements (tooltipkey), which is set to the ID of the XUL key element for the command.  Once this attribute is set, the normal tooltip of the control is appended by the accelerator hint text (like |Ctrl+Shift+N| for example).  This is exactly like the key attribute on XUL menuitem elements.

The attached patch adds an API to nsContentUtils for getting the accelerator text based on the content node and the key attribute (|key| or |tooltipkey|), and refactors the code in nsMenuFrame::BuildAcceleratorText into the new API.  It also modifies nsXULTooltipListener::FindTooltip to add the accelerator hint to the tooltiptext if the tooltipkey attribute is specified and valid.  The patch comes also with an automated test.
Attachment #403984 - Flags: superreview?(dbaron)
Attachment #403984 - Flags: review?(bzbarsky)
Comment on attachment 403984 [details] [diff] [review]
Patch (v1)

enn is probably a better reviewer for this.
Attachment #403984 - Flags: review?(bzbarsky) → review?(enndeakin)
Comment on attachment 403984 [details] [diff] [review]
Patch (v1)

>+  /**
>+   * Get the accelerator text for a node in order to display in the UI.
>+   *
>+   * @param aContent [in] the node for which the accelerator text is needed.
>+   * @param aKeyAttribute [in] the attribute specifying the associated key element.

This description took me a moment to understand it. Maybe change it to say 'the attribute specifying the id of a key element'


>+   * @param aAccelText [out] the accelerator text to display in the UI.
>+   *                         might be empty if no such text can be displayed.

might -> Might

>diff --git a/content/base/src/nsContentUtils.cpp b/content/base/src/nsContentUtils.cpp

>-  //we load some display strings from platformKeys.properties only once
>   static nsrefcnt gRefCnt; 
>-  static nsString *gShiftText;
>-  static nsString *gControlText;
>-  static nsString *gMetaText;
>-  static nsString *gAltText;
>-  static nsString *gModifierSeparator;

You can remove gRefCnt as well


>diff --git a/layout/xul/base/src/nsXULTooltipListener.cpp b/layout/xul/base/src/nsXULTooltipListener.cpp
...
>+        nsContentUtils::FormatLocalizedString(nsContentUtils::ePLATFORM_KEYS_PROPERTIES,
>+                                              "MODIFIER_ENCLOSURE",

This would be clearer if instead of being called "MODIFIER_ENCLOSURE" it was just called "TOOLTIP_KEY" 


>diff --git a/toolkit/content/tests/widgets/Makefile.in b/toolkit/content/tests/widgets/Makefile.in
>--- a/toolkit/content/tests/widgets/Makefile.in
>+++ b/toolkit/content/tests/widgets/Makefile.in
>+		test_tooltipkey.xul \

There's no reason for this to be a separate test. Just add the extra steps into test_tooltip.xul


>+      result: function(testname, step, event) {
>+        netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+
>+        var tooltip = event.originalTarget;

test_tooltip.xul just puts the button in gButton, so no need to pass the extra event.


>+        is(tooltip.label, "button b tooltip " + appendix,
>+           "The tooltipkey attribute should correctly affect the tooltip text");

This should have a different description so it is clearer if one of the tests fails.
Attachment #403984 - Flags: review?(enndeakin) → review-
Comment on attachment 403984 [details] [diff] [review]
Patch (v1)

minus for now.
Updating to reality: I don't think I will be able to invest time in addressing the review comments here any time soon.  Anyone who wishes to step up should probably have an easy job to address comment 2, though.
Assignee: ehsan → nobody
Status: ASSIGNED → NEW
Attached patch Patch v2 (obsolete) — Splinter Review
* updated to trunk and addressed comment 2

* pushed to try-server: http://hg.mozilla.org/try/rev/822b21aa0513 but getting the following orange on different boxes: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1278431874.1278432091.5826.gz#err0 . Otherwise it looks pretty good, at least from what I can see on the try-server runs.
Assignee: nobody → michaelkohler
Attachment #403984 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #456170 - Flags: review?(enndeakin)
Comment on attachment 456170 [details] [diff] [review]
Patch v2

>+  delete sShiftText;
>+  sShiftText = nsnull;
>+  delete sControlText;  
>+  sControlText = nsnull;
>+  delete sMetaText;  
>+  sMetaText = nsnull;
>+  delete sAltText;  
>+  sAltText = nsnull;
>+  delete sModifierSeparator;
>+  sModifierSeparator = nsnull;
>+
>+
>   nsAutoGCRoot::Shutdown();

Remove this extra blank line


>+    var appendix = gBundle.formatStringFromName("MODIFIER_ENCLOSURE",
 ["A"], 1);

There are two places in the test where you need to change to TOOLTIP_KEY


>+    var appendix = gBundle.formatStringFromName("MODIFIER_ENCLOSURE",
>+                                                [shift + plus + ctrl + plus + "B"], 1);

Here is the other one.


Does the test fail locally? What error is occurring? Likely, something is going wrong within the 'result' part. If it doesn't fail locally, try adding some debugging info in there and try again.
(In reply to comment #6)
> Comment on attachment 456170 [details] [diff] [review]
> Patch v2
> [...]
> Does the test fail locally? What error is occurring? Likely, something is going
> wrong within the 'result' part. If it doesn't fail locally, try adding some
> debugging info in there and try again.

The window doesn't close after starting the test with |TEST_PATH= make -C ../fx-obj/ mochitest-plain|. So I pushed to try server again and the test fails with the following errors on different boxes: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1278965625.1278965943.10603.gz#err0.

I will try to fix the test and upload a new patch here soon.
(In reply to comment #7)
> The window doesn't close after starting the test with |TEST_PATH= make -C
> ../fx-obj/ mochitest-plain|. So I pushed to try server again and the test fails
> with the following errors on different boxes:
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1278965625.1278965943.10603.gz#err0.

Of course I mean |TEST_PATH=toolkit/content/tests/widgets/test_tooltip.xul make -C ../fx-obj/ mochitest-plain|. Sorry.
Comment on attachment 456170 [details] [diff] [review]
Patch v2

OK, it's possible that the fix to TOOLTIP_KEY might fix the test.
Attachment #456170 - Flags: review?(enndeakin)
Attached patch WIPSplinter Review
(In reply to comment #9)
> Comment on attachment 456170 [details] [diff] [review]
> Patch v2
> 
> OK, it's possible that the fix to TOOLTIP_KEY might fix the test.

Actually it doesn't. When I run the test, the window opens, but doesn't close after that. And honestly, I have no idea why.
Attachment #456170 - Attachment is obsolete: true
Assignee: michaelkohler → nobody
Blocks: 603491
No longer blocks: 603491
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
No assignee, updating the status.
You need to log in before you can comment on or make changes to this bug.