Closed Bug 583533 Opened 14 years ago Closed 13 years ago

Implement AccessKeyLabel attribute

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [inbound])

Attachments

(1 file, 7 obsolete files)

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.
Assignee: nobody → dzbarsky
Attached patch Patch v1 (obsolete) — Splinter Review
Feel free to punt the review to someone else if you feel like it.
Attachment #461927 - Flags: review?(Olli.Pettay)
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.
Attachment #461927 - Flags: review?(Olli.Pettay) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
I changed it as you requested.  Do you mean we should have that Mac image instead of "Cmd"?
Attachment #461927 - Attachment is obsolete: true
Attachment #462291 - Flags: review?(Olli.Pettay)
Attached patch Patch v2 (obsolete) — Splinter Review
This time for real
Attachment #462291 - Attachment is obsolete: true
Attachment #462292 - Flags: review?(Olli.Pettay)
Attachment #462291 - Flags: review?(Olli.Pettay)
(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.
The HTML5 (draft) API is just a bit strange to me. It doesn't say exatly
what kinds of values should be returned.
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.
Attachment #462292 - Flags: review?(Olli.Pettay) → review-
(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.
Attached patch Patch v2.1 (obsolete) — Splinter Review
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?
Attachment #462292 - Attachment is obsolete: true
Attachment #462882 - Flags: review?(Olli.Pettay)
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!
Attachment #462882 - Flags: review?(Olli.Pettay) → review-
Attached patch Patch v2.2 (obsolete) — Splinter Review
Attachment #462882 - Attachment is obsolete: true
Attachment #469338 - Flags: review?(Olli.Pettay)
No longer blocks: 583514
Depends on: 583514
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?
Attachment #469338 - Flags: review?(Olli.Pettay) → review+
Do we want this for Gecko 2.0?  If so, should this be requesting approval or blocking or something?
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.
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: x86 → All
Do we need UI review on this? I suspect using the mac symbols on other platforms is going to be confusing.
Also, you can always nominate, that doesn't mean automatic approval
Well, if you think it should be considered, you need to request approval, no?
Attached patch Patch v2.2 r=smaug (obsolete) — Splinter Review
Updated to current trunk
Attachment #469338 - Attachment is obsolete: true
Attachment #541381 - Flags: ui-review?(mbeltzner)
Attachment #469338 - Flags: ui-review?(mbeltzner)
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.
Attachment #541381 - Flags: ui-review?(mbeltzner) → ui-review?(faaborg)
Attached patch Patch v2.2 r=smaug (obsolete) — Splinter Review
Oops, missed a chunk
Attachment #541381 - Attachment is obsolete: true
Attachment #541536 - Flags: ui-review?(faaborg)
Attachment #541381 - Flags: ui-review?(faaborg)
>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 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).
Attachment #541536 - Flags: ui-review?(faaborg) → ui-review-
No, on Windows it uses "Ctrl" "Shift" and "Alt."
Comment on attachment 541536 [details] [diff] [review]
Patch v2.2 r=smaug

sorry about that, +
Attachment #541536 - Flags: ui-review- → ui-review+
(In reply to comment #24)
> No, on Windows it uses "Ctrl" "Shift" and "Alt."

With GTK too I guess?
This patch pulls the names from the prefs, so it will look the same as the menu shortcut hints.
Attachment #541536 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #549625 - Flags: checkin?
Keywords: checkin-needed
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/cb4d291f2f90
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Keywords: dev-doc-needed
should the checkin? flag be removed here?
Attachment #549625 - Flags: checkin? → checkin+
Documentation updated:

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

Also mentioned on Firefox 8 for developers.
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
(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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.