808964 - AccessKeyLabel should be empty string for elements without the accesskey attribute

Status: RESOLVED FIXED

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

Description

[Michael Müller]

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 (he/him; ⌚ UTC+1/+2)]

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)]

Okey let me try :)

Comment 3

[Alexandre BM (:rednaks)]

Attachment: 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]]

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 (he/him; ⌚ UTC+1/+2)]

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)]

OK, thanks Ms2ger !

Comment 7

[Alexandre BM (:rednaks)]

Attachment: Patch V2 style issue

fixing style issue

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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)]

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)]

Attachment: 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][[e-mail]]]

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]

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

Oups I didn't noticed that i was connected with the other account !

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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

Attachment: Patch v4

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=1d5de0f9b30a

Comment 18

[Alexandre BM (:rednaks)]

Great :)

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/abac7a34e15c