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)
:
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 | Review
Patch V2 style issue (1.00 KB, patch)
2012-11-06 11:33 PST, Alexandre BM (:rednaks)
bugs: review-
Details | Diff | Review
PatchV3 (2.24 KB, patch)
2012-11-06 23:14 PST, Alexandre BM (:rednaks)
bugs: review-
Details | Diff | Review
Patch v4 (1.81 KB, patch)
2012-11-07 06:04 PST, Alexandre BM (:rednaks)
bugs: review+
Details | Diff | Review

Description 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 :Ms2ger 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 Alexandre BM (:rednaks) 2012-11-06 07:13:50 PST
Okey let me try :)
Comment 3 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 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 :Ms2ger 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 Alexandre BM (:rednaks) 2012-11-06 11:31:55 PST
OK, thanks Ms2ger !
Comment 7 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 Olli Pettay [:smaug] (high review load, please consider other reviewers) 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 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 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 Olli Pettay [:smaug] (high review load, please consider other reviewers) 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 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 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 Olli Pettay [:smaug] (high review load, please consider other reviewers) 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 Alexandre BM (:rednaks) 2012-11-07 06:04:38 PST
Created attachment 679125 [details] [diff] [review]
Patch v4
Comment 16 Olli Pettay [:smaug] (high review load, please consider other reviewers) 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 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-11-07 08:19:32 PST
https://tbpl.mozilla.org/?tree=Try&rev=1d5de0f9b30a
Comment 18 Alexandre BM (:rednaks) 2012-11-07 08:46:04 PST
Great :)
Comment 19 Olli Pettay [:smaug] (high review load, please consider other reviewers) 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.