The default bug view has changed. See this FAQ.

Implement AccessKeyLabel attribute

RESOLVED FIXED in mozilla8

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

({dev-doc-complete})

Trunk
mozilla8
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound], URL)

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

7 years ago
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)

Comment 1

7 years ago
See http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#dom-accesskeylabel
(Assignee)

Updated

7 years ago
Assignee: nobody → dzbarsky
(Assignee)

Comment 2

7 years ago
Created attachment 461927 [details] [diff] [review]
Patch v1

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-
(Assignee)

Comment 4

7 years ago
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"?
Attachment #461927 - Attachment is obsolete: true
Attachment #462291 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 5

7 years ago
Created attachment 462292 [details] [diff] [review]
Patch v2

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

Comment 10

7 years ago
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?
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-
(Assignee)

Comment 12

7 years ago
Created attachment 469338 [details] [diff] [review]
Patch v2.2
Attachment #462882 - Attachment is obsolete: true
Attachment #469338 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

7 years ago
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?
Attachment #469338 - Flags: ui-review?(beltzner)
(Assignee)

Comment 19

6 years ago
Created attachment 541381 [details] [diff] [review]
Patch v2.2 r=smaug

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)
(Assignee)

Comment 21

6 years ago
Created attachment 541536 [details] [diff] [review]
Patch v2.2 r=smaug

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-
(Assignee)

Comment 24

6 years ago
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?
(Assignee)

Comment 27

6 years ago
This patch pulls the names from the prefs, so it will look the same as the menu shortcut hints.
(Assignee)

Comment 28

6 years ago
Created attachment 549625 [details] [diff] [review]
Patch for checkin r=smaug ui-review=faaborg
Attachment #541536 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Attachment #549625 - Flags: checkin?

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/cb4d291f2f90
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed

Comment 30

6 years ago
should the checkin? flag be removed here?
(Assignee)

Updated

6 years ago
Attachment #549625 - Flags: checkin? → checkin+
Documentation updated:

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

Also mentioned on Firefox 8 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 728132

Comment 32

3 years ago
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

3 years ago
(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
You need to log in before you can comment on or make changes to this bug.