Last Comment Bug 808964 - AccessKeyLabel should be empty string for elements without the accesskey attribute
: AccessKeyLabel should be empty string for elements without the accesskey attr...
Status: RESOLVED FIXED
[good first bug][mentor=Ms2ger][lang=...
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 10 Branch
: x86_64 Windows 7
-- normal (vote)
: mozilla19
Assigned To: Alexandre BM (:rednaks)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-06 00:27 PST by Michael Müller
Modified: 2012-11-07 10:53 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch for accessKeyLabel if not set (940 bytes, patch)
2012-11-06 07:25 PST, Alexandre BM (:rednaks)
Ms2ger: feedback+
Details | Diff | Splinter Review
Patch V2 style issue (1.00 KB, patch)
2012-11-06 11:33 PST, Alexandre BM (:rednaks)
bugs: review-
Details | Diff | Splinter Review
PatchV3 (2.24 KB, patch)
2012-11-06 23:14 PST, Alexandre BM (:rednaks)
bugs: review-
Details | Diff | Splinter Review
Patch v4 (1.81 KB, patch)
2012-11-07 06:04 PST, Alexandre BM (:rednaks)
bugs: review+
Details | Diff | Splinter Review

Description User image Michael Müller 2012-11-06 00:27:22 PST
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0.3) Gecko/20100101 Firefox/10.0.3
Build ID: 20120309135702

Steps to reproduce:

1. Chose an element without an accesskey attribute.
2. Look at its accessKeyLabel property (e.g.
alert(document.getElementsByTagName('body')[0].accessKeyLabel)
)


Actual results:

The accessKeyLabel property is "Alt+Umschalt+" (or whatever language you use).
I tested this both in Firefox 10 and 16.


Expected results:

Instead it should be the empty string.
http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#dom-accesskeylabel says:
"The accessKeyLabel IDL attribute must return a string that represents the element's assigned access key, if any. If the element does not have one, then the IDL attribute must return the empty string."
Comment 1 User image :Ms2ger (⌚ UTC+1/+2) 2012-11-06 00:36:58 PST
To fix this bug, one will need to change nsGenericHTMLElement::GetAccessKeyLabel [1] to only call GetAccessKeyLabelPrefix if GetAccessKey returns a non-empty string.

[1] http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.cpp#449
Comment 2 User image Alexandre BM (:rednaks) 2012-11-06 07:13:50 PST
Okey let me try :)
Comment 3 User image Alexandre BM (:rednaks) 2012-11-06 07:25:08 PST
Created attachment 678745 [details] [diff] [review]
patch for accessKeyLabel if not set

here my patch, it  elt.accessKeyLabel  returns an empty string if it's not set, "Alt+Shift+<key>" else.
Comment 4 User image Josh Matthews [:jdm] 2012-11-06 08:34:56 PST
Comment on attachment 678745 [details] [diff] [review]
patch for accessKeyLabel if not set

For future reference, it is always better to request review from an actual person.
Comment 5 User image :Ms2ger (⌚ UTC+1/+2) 2012-11-06 10:18:52 PST
Comment on attachment 678745 [details] [diff] [review]
patch for accessKeyLabel if not set

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

Just two style issues. Please fix those, and ask for review from :smaug on the updated patch.

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +455,3 @@
>        nsAutoString suffix;
>        GetAccessKey(suffix);
> +      if(!suffix.IsEmpty() && presContext->EventStateManager()->GetAccessKeyLabelPrefix(aLabel))

You should add a space between 'if' and '(', and wrap the condition over two lines, so that the line isn't longer than 80 characters. (&& at the end.)
Comment 6 User image Alexandre BM (:rednaks) 2012-11-06 11:31:55 PST
OK, thanks Ms2ger !
Comment 7 User image Alexandre BM (:rednaks) 2012-11-06 11:33:41 PST
Created attachment 678826 [details] [diff] [review]
Patch V2 style issue

fixing style issue
Comment 8 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-11-06 11:57:39 PST
Comment on attachment 678826 [details] [diff] [review]
Patch V2 style issue

> NS_IMETHODIMP
> nsGenericHTMLElement::GetAccessKeyLabel(nsAString& aLabel)
> {
>   nsPresContext *presContext = GetPresContext();
> 
>-  if (presContext &&
>-    presContext->EventStateManager()->GetAccessKeyLabelPrefix(aLabel)) {
>+  if (presContext) {
>       nsAutoString suffix;
>       GetAccessKey(suffix);
>-      aLabel.Append(suffix);
>+      if (!suffix.IsEmpty() && 
>+	 presContext->EventStateManager()->GetAccessKeyLabelPrefix(aLabel))
You're using tab here. Only use spaces. prescontext should be aligned under !suffix
And always use {} with |if|
  if (expression) {
    stmt;
  }

Could you update the test 
http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_bug583533.html?force=1
to test also this case when accessKeyLabel isn't there.
Comment 9 User image Alexandre BM (:rednaks) 2012-11-06 14:24:22 PST
Sorry for the tabs, now I have setup my vimrc it should be fine.
and about the test, should I add a new element without accessKey and test or ... ?
Comment 10 User image Alexandre BM (:rednaks) 2012-11-06 23:14:36 PST
Created attachment 679068 [details] [diff] [review]
PatchV3

Here my solution, I've fixed tabs with vim to follow mozilla coding style. 
But I'm not sure about what I've wrote in the test file
Comment 11 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-11-07 00:49:23 PST
I would have expected something like 
var div = document.createElement("div");
document.body.appendChild(div);
is(div.accessKeyLabel, "", "accessKeyLabel should be empty string");


And why you're modifying nsGenericHTMLElement::GetAccessKeyLabel, could you please
make it use 2 space indentation.
Comment 12 User image salexandre.bm 2012-11-07 04:32:15 PST
(In reply to Olli Pettay [:smaug] from comment #11)
> I would have expected something like 
> var div = document.createElement("div");
> document.body.appendChild(div);
> is(div.accessKeyLabel, "", "accessKeyLabel should be empty string");
> 
Okey I'll add this and the enf of the test file. should I specify that this is related to this bug in comment ?

> 
> And why you're modifying nsGenericHTMLElement::GetAccessKeyLabel, could you
> please
> make it use 2 space indentation.

Sorry but, It's not clear for me how did I modified nsGenericHTMLElement::GetAccessKeyLabel ? and how it should be ?
Comment 13 User image Alexandre BM (:rednaks) 2012-11-07 04:36:24 PST
Oups I didn't noticed that i was connected with the other account !
Comment 14 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-11-07 05:30:40 PST
(In reply to salexandre.bm from comment #12)
> Okey I'll add this and the enf of the test file. should I specify that this
> is related to this bug in comment ?
You can add some comment
// for bug 808964


> 
> > 
> > And why you're modifying nsGenericHTMLElement::GetAccessKeyLabel, could you
> > please
> > make it use 2 space indentation.
> 
> Sorry but, It's not clear for me how did I modified
> nsGenericHTMLElement::GetAccessKeyLabel ? and how it should be ?
Oops, I wasn't going to write 'why' but 'now that' or something like that.
Anyhow for example
       nsAutoString suffix;
       GetAccessKey(suffix);
have 4 space indentation from the |if|. Should be 2 spaces only.
Same with the |if| you're adding (the one which has aLabel.Append(suffix) in it).
Comment 15 User image Alexandre BM (:rednaks) 2012-11-07 06:04:38 PST
Created attachment 679125 [details] [diff] [review]
Patch v4
Comment 16 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-11-07 08:12:10 PST
Comment on attachment 679125 [details] [diff] [review]
Patch v4

Thanks.

I'll push this to tryserver, and if everything looks good, push to mozilla-central.
Comment 17 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-11-07 08:19:32 PST
https://tbpl.mozilla.org/?tree=Try&rev=1d5de0f9b30a
Comment 18 User image Alexandre BM (:rednaks) 2012-11-07 08:46:04 PST
Great :)
Comment 19 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-11-07 10:53:34 PST
https://hg.mozilla.org/mozilla-central/rev/abac7a34e15c

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