Closed
Bug 583533
Opened 14 years ago
Closed 13 years ago
Implement AccessKeyLabel attribute
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: dzbarsky, Assigned: dzbarsky)
References
()
Details
(Keywords: dev-doc-complete, Whiteboard: [inbound])
Attachments
(1 file, 7 obsolete files)
24.40 KB,
patch
|
dzbarsky
:
checkin+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
See http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#dom-accesskeylabel
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → dzbarsky
Assignee | ||
Comment 2•14 years ago
|
||
Feel free to punt the review to someone else if you feel like it.
Attachment #461927 -
Flags: review?(Olli.Pettay)
Comment 3•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
This time for real
Attachment #462291 -
Attachment is obsolete: true
Attachment #462292 -
Flags: review?(Olli.Pettay)
Attachment #462291 -
Flags: review?(Olli.Pettay)
Comment 6•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
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-
Comment 9•14 years ago
|
||
(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•14 years ago
|
||
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 11•14 years ago
|
||
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•14 years ago
|
||
Attachment #462882 -
Attachment is obsolete: true
Attachment #469338 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•14 years ago
|
Comment 13•14 years ago
|
||
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+
Comment 14•14 years ago
|
||
Do we want this for Gecko 2.0? If so, should this be requesting approval or blocking or something?
Comment 15•14 years ago
|
||
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
Comment 18•14 years ago
|
||
Well, if you think it should be considered, you need to request approval, no?
Attachment #469338 -
Flags: ui-review?(beltzner)
Assignee | ||
Comment 19•13 years ago
|
||
Updated to current trunk
Attachment #469338 -
Attachment is obsolete: true
Attachment #541381 -
Flags: ui-review?(mbeltzner)
Attachment #469338 -
Flags: ui-review?(mbeltzner)
Comment 20•13 years ago
|
||
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•13 years ago
|
||
Oops, missed a chunk
Attachment #541381 -
Attachment is obsolete: true
Attachment #541536 -
Flags: ui-review?(faaborg)
Attachment #541381 -
Flags: ui-review?(faaborg)
Comment 22•13 years ago
|
||
>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•13 years ago
|
||
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•13 years ago
|
||
No, on Windows it uses "Ctrl" "Shift" and "Alt."
Comment 25•13 years ago
|
||
Comment on attachment 541536 [details] [diff] [review] Patch v2.2 r=smaug sorry about that, +
Attachment #541536 -
Flags: ui-review- → ui-review+
Comment 26•13 years ago
|
||
(In reply to comment #24) > No, on Windows it uses "Ctrl" "Shift" and "Alt." With GTK too I guess?
Assignee | ||
Comment 27•13 years ago
|
||
This patch pulls the names from the prefs, so it will look the same as the menu shortcut hints.
Assignee | ||
Comment 28•13 years ago
|
||
Attachment #541536 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•13 years ago
|
Attachment #549625 -
Flags: checkin?
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: [inbound]
Comment 29•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/cb4d291f2f90
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Assignee | ||
Updated•13 years ago
|
Keywords: dev-doc-needed
Comment 30•13 years ago
|
||
should the checkin? flag be removed here?
Assignee | ||
Updated•13 years ago
|
Attachment #549625 -
Flags: checkin? → checkin+
Comment 31•13 years ago
|
||
Documentation updated: https://developer.mozilla.org/en/HTML/Global_attributes Also mentioned on Firefox 8 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Comment 32•10 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•10 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
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•