The default bug view has changed. See this FAQ.

AccessKeyLabel should be empty string for elements without the accesskey attribute

RESOLVED FIXED in mozilla19

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Michael Müller, Assigned: rednaks)

Tracking

10 Branch
mozilla19
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=Ms2ger][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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."
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][mentor=Ms2ger][lang=c++]
(Assignee)

Comment 2

4 years ago
Okey let me try :)
(Assignee)

Comment 3

4 years ago
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.
Attachment #678745 - Flags: review?

Updated

4 years ago
Assignee: nobody → s

Comment 4

4 years ago
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.
Attachment #678745 - Flags: review? → review?(Ms2ger)
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.)
Attachment #678745 - Flags: review?(Ms2ger) → feedback+
(Assignee)

Comment 6

4 years ago
OK, thanks Ms2ger !
(Assignee)

Comment 7

4 years ago
Created attachment 678826 [details] [diff] [review]
Patch V2 style issue

fixing style issue
Attachment #678745 - Attachment is obsolete: true
Attachment #678826 - Flags: review?(bugs)
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.
Attachment #678826 - Flags: review?(bugs) → review-
(Assignee)

Comment 9

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

Comment 10

4 years ago
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
Attachment #678826 - Attachment is obsolete: true
Attachment #679068 - Flags: review?(bugs)
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.
Attachment #679068 - Flags: review?(bugs) → review-

Comment 12

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

Comment 13

4 years ago
Oups I didn't noticed that i was connected with the other account !
(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).
(Assignee)

Comment 15

4 years ago
Created attachment 679125 [details] [diff] [review]
Patch v4
Attachment #679068 - Attachment is obsolete: true
Attachment #679125 - Flags: review?(bugs)
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.
Attachment #679125 - Flags: review?(bugs) → review+
https://tbpl.mozilla.org/?tree=Try&rev=1d5de0f9b30a
(Assignee)

Comment 18

4 years ago
Great :)
https://hg.mozilla.org/mozilla-central/rev/abac7a34e15c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.