Last Comment Bug 583533 - Implement AccessKeyLabel attribute
: Implement AccessKeyLabel attribute
Status: RESOLVED FIXED
[inbound]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: David Zbarsky (:dzbarsky)
:
Mentors:
http://www.whatwg.org/specs/web-apps/...
Depends on: 583514 728132
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-31 19:12 PDT by David Zbarsky (:dzbarsky)
Modified: 2014-07-13 11:48 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (9.39 KB, patch)
2010-08-01 14:00 PDT, David Zbarsky (:dzbarsky)
bugs: review-
Details | Diff | Splinter Review
Patch v2 (9.39 KB, patch)
2010-08-02 18:34 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch v2 (9.72 KB, patch)
2010-08-02 18:38 PDT, David Zbarsky (:dzbarsky)
bugs: review-
Details | Diff | Splinter Review
Patch v2.1 (12.48 KB, patch)
2010-08-04 12:43 PDT, David Zbarsky (:dzbarsky)
bugs: review-
Details | Diff | Splinter Review
Patch v2.2 (26.07 KB, patch)
2010-08-25 21:32 PDT, David Zbarsky (:dzbarsky)
bugs: review+
Details | Diff | Splinter Review
Patch v2.2 r=smaug (23.58 KB, patch)
2011-06-23 08:03 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch v2.2 r=smaug (25.20 KB, patch)
2011-06-23 16:23 PDT, David Zbarsky (:dzbarsky)
faaborg: ui‑review+
Details | Diff | Splinter Review
Patch for checkin r=smaug ui-review=faaborg (24.40 KB, patch)
2011-07-30 22:59 PDT, David Zbarsky (:dzbarsky)
dzbarsky: checkin+
Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2010-07-31 19:12:02 PDT
AccessKeyLabel exposes the key shortcuts that we assign to an AccessKey.
For example: If AccessKey is "g", AccessKeyLabel is probably "Alt + Shift + G" on Windows and "Cmd + G" on Mac, etc.
Comment 2 David Zbarsky (:dzbarsky) 2010-08-01 14:00:57 PDT
Created attachment 461927 [details] [diff] [review]
Patch v1

Feel free to punt the review to someone else if you feel like it.
Comment 3 Olli Pettay [:smaug] 2010-08-02 13:39:14 PDT
Comment on attachment 461927 [details] [diff] [review]
Patch v1


>+NS_IMETHODIMP
>+nsEventStateManager::GetAccessKeyLabelPrefix(nsAString& aPrefix)
>+{
>+  aPrefix.AssignLiteral("");
aPrefix.Truncate()

>+NS_IMETHODIMP
>+nsGenericHTMLElement::GetAccessKeyLabel(nsAString& aLabel)
>+{
>+  nsPresContext *presContext = GetPresContext();
>+
>+  if (presContext) {
>+    nsIEventStateManager *esm = presContext->EventStateManager();
>+    esm->GetAccessKeyLabelPrefix(aLabel);
>+
>+    if (!aLabel.IsEmpty()) {
Could you make GetAccessKeyLabelPrefix to return PRBool and PR_TRUE would mean
that there is a prefix. The you could just write
if (esm->GetAccessKeyLabelPrefix(aLabel)) {
  nsAutoString suffix;
  GetAccessKey(suffix);
  aLabel.Append(suffix);
}


>+      nsString suffix;
nsAutoString


>+      GetAccessKey(suffix);
>+      aLabel.Append(suffix);
>+    }
>+  }
>+
>+  return NS_OK;
>+}
So this would return something like Cmd+A on OSX.
Is that what we want? That is not what is shown natively in OSX applications.
Would be great to get a comment from mstange or josh.
(I think HTML5 draft is too vague in this case.)


>@@ -53,12 +53,13 @@ interface nsIDOMHTMLElement : nsIDOMElem
> {
>            attribute DOMString        id;
>            attribute DOMString        title;
>            attribute DOMString        lang;
>            attribute DOMString        dir;
>            attribute DOMString        className;
> 
>            attribute DOMString        accessKey;
>+  readonly attribute DOMString        accessKeyLabel;
You should update the uuid when changing an interface.
Comment 4 David Zbarsky (:dzbarsky) 2010-08-02 18:34:01 PDT
Created attachment 462291 [details] [diff] [review]
Patch v2

I changed it as you requested.  Do you mean we should have that Mac image instead of "Cmd"?
Comment 5 David Zbarsky (:dzbarsky) 2010-08-02 18:38:13 PDT
Created attachment 462292 [details] [diff] [review]
Patch v2

This time for real
Comment 6 Olli Pettay [:smaug] 2010-08-03 02:21:18 PDT
(In reply to comment #4)
> I changed it as you requested.  Do you mean we should have that Mac image
> instead of "Cmd"?
I'm not sure.
Comment 7 Olli Pettay [:smaug] 2010-08-03 02:23:08 PDT
The HTML5 (draft) API is just a bit strange to me. It doesn't say exatly
what kinds of values should be returned.
Comment 8 Olli Pettay [:smaug] 2010-08-03 03:00:30 PDT
Comment on attachment 462292 [details] [diff] [review]
Patch v2

I was looking at EventStateManager some more, and sContentAccessModifier isn't
what you want. See GetAccessModifierMask. We have different accesskey modifiers
for content and chrome. So I think GetAccessKeyLabelPrefix should check that
in which docshell it is in, and based on that use either sContentAccessModifier or
sChromeAccessModifier.

So something like

aPrefix.Truncate();
if (!mPresContext) {
  return PR_FALSE;
}

nsCOMPtr<nsISupports> container = mPresContext->GetContainer();
PRint32 modifier = GetAccessModifierMask(container);
...
and here all the & checks.


Sorry that I didn't notice this earlier.
Comment 9 Markus Stange [:mstange] 2010-08-03 05:21:42 PDT
(In reply to comment #4)
> Created attachment 462291 [details] [diff] [review]
> Patch v2
> 
> I changed it as you requested.  Do you mean we should have that Mac image
> instead of "Cmd"?

I think we should make it identical to what would be shown next to a menu item in the menu, and on Mac that would mean using the special key symbols: ⌘ for Cmd, ⇧ for Shift, ⌥ for Alt and ⌃ for Ctrl.

There's code in nsMenuFrame.cpp that loads the right representation of these keys from chrome://global-platform/locale/platformKeys.properties:
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuFrame.cpp#260

Can you reuse that? You'll have to deal with localization anyway; for example the Ctrl key is called "Strg" on Windows in German.
Comment 10 David Zbarsky (:dzbarsky) 2010-08-04 12:43:05 PDT
Created attachment 462882 [details] [diff] [review]
Patch v2.1

I did as you requested.  It's a bit of a hack, because nsEventStateManager now includes nsMenuFrame.h - maybe we should split the modifier stuff to another header?
Comment 11 Olli Pettay [:smaug] 2010-08-11 08:45:32 PDT
Comment on attachment 462882 [details] [diff] [review]
Patch v2.1


>+nsGenericHTMLElement::GetAccessKeyLabel(nsAString& aLabel)
>+{
>+  nsPresContext *presContext = GetPresContext();
>+
>+  if (presContext) {
>+    if (presContext->EventStateManager()->GetAccessKeyLabelPrefix(aLabel)) {
Combine these ifs
if (presContext && presContext->EventStateManager()->GetAccessKeyLabelPrefix(aLabel)) {
  ...

>diff --git a/layout/xul/base/src/nsMenuFrame.cpp b/layout/xul/base/src/nsMenuFrame.cpp
>--- a/layout/xul/base/src/nsMenuFrame.cpp
>+++ b/layout/xul/base/src/nsMenuFrame.cpp
>@@ -95,16 +95,17 @@ static PRInt32 gEatMouseMove = PR_FALSE;
> static NS_DEFINE_IID(kLookAndFeelCID, NS_LOOKANDFEEL_CID);
> 
> nsrefcnt nsMenuFrame::gRefCnt = 0;
> nsString *nsMenuFrame::gShiftText = nsnull;
> nsString *nsMenuFrame::gControlText = nsnull;
> nsString *nsMenuFrame::gMetaText = nsnull;
> nsString *nsMenuFrame::gAltText = nsnull;
> nsString *nsMenuFrame::gModifierSeparator = nsnull;
>+
No need for this change.


>+  static void GetShiftText(nsAString& text) { text.Assign(*gShiftText); }
>+  static void GetControlText(nsAString& text) { text.Assign(*gControlText); }
>+  static void GetMetaText(nsAString& text) { text.Assign(*gMetaText); }
>+  static void GetAltText(nsAString& text) { text.Assign(*gAltText); }
>+  static void GetModifierSeparatorText(nsAString& text) { text.Assign(*gModifierSeparator); }
It would be great if you could move these to nsContentUtils.
Especially because nothing guarantees atm that these gFoo variables are initialized before they are used
in EventStateManager.
So move the variables to nsContentUtils, rename them so that they start with 's', not 'g'.
(s is for a static variable, g is for a global variable) and initialize them in
nsContentUtils::Init.

And this all really needs tests.

Sorry for the review delay!
Comment 12 David Zbarsky (:dzbarsky) 2010-08-25 21:32:56 PDT
Created attachment 469338 [details] [diff] [review]
Patch v2.2
Comment 13 Olli Pettay [:smaug] 2010-09-06 07:33:08 PDT
Comment on attachment 469338 [details] [diff] [review]
Patch v2.2


>+NS_IMETHODIMP
>+nsGenericHTMLElement::GetAccessKeyLabel(nsAString& aLabel)
>+{
>+  nsPresContext *presContext = GetPresContext();
>+
>+  if (presContext &&
>+    presContext->EventStateManager()->GetAccessKeyLabelPrefix(aLabel)) {
indentation 

>+++ b/content/html/content/test/test_bug583533.html
>@@ -0,0 +1,76 @@
>+<!DOCTYPE HTML>
Some garbage in the file?
Comment 14 Boris Zbarsky [:bz] 2010-09-09 21:12:32 PDT
Do we want this for Gecko 2.0?  If so, should this be requesting approval or blocking or something?
Comment 15 Mounir Lamouri (:mounir) 2010-09-09 23:48:33 PDT
It sounds to be a standalone feature so I guess we can add it to Gecko 2.0.
I could try to refresh, fix the nits and send it to try tomorrow if I found time. However, I can't decide for the approval, I let jst or sicking (or any driver) give it if they think it's appropriate.
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2010-09-10 09:08:14 PDT
Do we need UI review on this? I suspect using the mac symbols on other platforms is going to be confusing.
Comment 17 Jonas Sicking (:sicking) PTO Until July 5th 2010-09-10 09:08:56 PDT
Also, you can always nominate, that doesn't mean automatic approval
Comment 18 Boris Zbarsky [:bz] 2010-09-10 10:10:02 PDT
Well, if you think it should be considered, you need to request approval, no?
Comment 19 David Zbarsky (:dzbarsky) 2011-06-23 08:03:33 PDT
Created attachment 541381 [details] [diff] [review]
Patch v2.2 r=smaug

Updated to current trunk
Comment 20 Mike Beltzner [:beltzner, not reading bugmail] 2011-06-23 08:21:24 PDT
Comment on attachment 541381 [details] [diff] [review]
Patch v2.2 r=smaug

Review of attachment 541381 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not the right person to review this.
Comment 21 David Zbarsky (:dzbarsky) 2011-06-23 16:23:45 PDT
Created attachment 541536 [details] [diff] [review]
Patch v2.2 r=smaug

Oops, missed a chunk
Comment 22 Alex Faaborg [:faaborg] (Firefox UX) 2011-07-28 17:10:45 PDT
>Do we need UI review on this? I suspect using the mac symbols on other platforms >is going to be confusing.

This is perhaps the first time I've done a UI review over in Core > DOM.  We should try to match the text being shown in native menus on each platform.  So for instance, OS X uses symbols, and Windows uses text, like "Ctrl" "Shift" and "Alt."
Comment 23 Alex Faaborg [:faaborg] (Firefox UX) 2011-07-28 17:12:43 PDT
Comment on attachment 541536 [details] [diff] [review]
Patch v2.2 r=smaug

Need to only show the symbols on OS X (at least I think this patch might be showing them on all platforms, I'm not really sure).
Comment 24 David Zbarsky (:dzbarsky) 2011-07-28 17:15:10 PDT
No, on Windows it uses "Ctrl" "Shift" and "Alt."
Comment 25 Alex Faaborg [:faaborg] (Firefox UX) 2011-07-28 18:07:11 PDT
Comment on attachment 541536 [details] [diff] [review]
Patch v2.2 r=smaug

sorry about that, +
Comment 26 Mounir Lamouri (:mounir) 2011-07-28 21:55:11 PDT
(In reply to comment #24)
> No, on Windows it uses "Ctrl" "Shift" and "Alt."

With GTK too I guess?
Comment 27 David Zbarsky (:dzbarsky) 2011-07-30 22:59:24 PDT
This patch pulls the names from the prefs, so it will look the same as the menu shortcut hints.
Comment 28 David Zbarsky (:dzbarsky) 2011-07-30 22:59:55 PDT
Created attachment 549625 [details] [diff] [review]
Patch for checkin r=smaug ui-review=faaborg
Comment 29 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-01 08:04:00 PDT
http://hg.mozilla.org/mozilla-central/rev/cb4d291f2f90
Comment 30 Asa Dotzler [:asa] 2011-08-05 22:56:04 PDT
should the checkin? flag be removed here?
Comment 31 Eric Shepherd [:sheppy] 2011-10-18 09:53:41 PDT
Documentation updated:

https://developer.mozilla.org/en/HTML/Global_attributes

Also mentioned on Firefox 8 for developers.
Comment 32 Krinkle 2014-07-13 11:35:53 PDT
It seems the property is an empty string for nodes while detached. This makes it hard for UI libraries to use this as they'd have to append it to the document first and update the attribute later (generally appending is saved to last and a completely separate step from constructing the widget).

Test case: http://jsfiddle.net/R3fNY/

Down stream bug report: https://bugzilla.wikimedia.org/show_bug.cgi?id=67946
Comment 33 Krinkle 2014-07-13 11:48:09 PDT
(In reply to Timo Tijhof from comment #32)
> It seems the property is an empty string for nodes while detached. This
> makes it hard for UI libraries to use this as they'd have to append it to
> the document first and update the attribute later (generally appending is
> saved to last and a completely separate step from constructing the widget).
> 
> Test case: http://jsfiddle.net/R3fNY/
> 
> Down stream bug report: https://bugzilla.wikimedia.org/show_bug.cgi?id=67946


Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1037990

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