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

Status: NEW

(Core :: XUL, enhancement)

Description

[(no longer active)]

Attachment: Patch (v1)

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 403984 [details] [diff] [review]
Patch (v1)

enn is probably a better reviewer for this.

Comment 2

[Neil Deakin]

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.

Comment 3

[Neil Deakin]

Comment on attachment 403984 [details] [diff] [review]
Patch (v1)

minus for now.

Comment 4

[(no longer active)]

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.

Comment 5

[Michael Kohler [:mkohler]]

Attachment: Patch v2

* 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.

Comment 6

[Neil Deakin]

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.

Comment 7

[Michael Kohler [:mkohler]]

(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.

Comment 8

[Michael Kohler [:mkohler]]

(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 9

[Neil Deakin]

Comment on attachment 456170 [details] [diff] [review]
Patch v2

OK, it's possible that the fix to TOOLTIP_KEY might fix the test.

Comment 10

[Michael Kohler [:mkohler]]

Attachment: WIP

(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.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.