287739 - All message bar buttons need mnemonics

Status: RESOLVED FIXED

(Firefox :: Keyboard Navigation, defect, P2)

Description

[Aaron Leventhal]

The options button that shows up for popups and extension installation needs an
underlined mnemonic.

It's especially important in the case of alert bars for screen reader users,
because the alert is going to be read aloud, but there will be no other quick
way to even find the button unless a keyboard shortcut for it is simultaneously
announced with the alert.

A FAQ on choosing a good accesskey is here:
http://www.mozilla.org/access/keyboard/accesskey

Comment 1

[Aaron Leventhal]

Pete, I'm assigning this one to you. Sorry if it's a bit mundane, but it's good
to get some chrome work done. This one actually is more important than it sounds
at first.

The browsermessagebinding is the one that contains the button:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/browser.xml#916

I don't recall how the button's text gets set but wherever that is would be
where the accesskey should be set as well.

Comment 2

[parente]

The message bar is displayed by showMessage defined by the tabbrowser XUL 
widget. This method does not currently support an access key as a param. Going 
to extend the method to support an access key param and put a new key in the 
browser.properties file (where the button text for the current locale is stored) 
defining the access key mnemonic.

Comment 3

[parente]

Attachment: Adds access key support to message bar

Updates all uses of showMessage method to include an access key (e.g. popup
blocked, install extension)
browsermessage binding grows a property "buttonAccesskey" for setting the
button access key
Access keys are specified in browser.properties next to button labels

Comment 4

[parente]

Attachment: Fixes some nits pointed out by aaronleventh

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 187411 [details] [diff] [review]
Fixes some nits pointed out by aaronleventh

please use accesskey in place of mnemonic to match the rest of toolkit/browser.

>+        <setter>
>+          <![CDATA[
>+            return this._buttonElement.accesskey = val;

setters are expected to return val, not a boolean.  should be:

  this._buttonElement.accesskey = val;
  return val;

r=me with those addressed

Comment 6

[Mike Connor [:mconnor]]

bah, ignore the boolean part, I guess I hallucinated brackets there, but lets
stick to conventional formatting for consistency.

Comment 7

[parente]

Attachment: Fixes mconnor nits and a bug

Caught a separate bug in browser.xml. Assigning to
this._buttonElement.accesskey does not work. Must use get/setAttribute instead.

Comment 8

[Aaron Leventhal]

Checking in toolkit/content/widgets/browser.xml;
/cvsroot/mozilla/toolkit/content/widgets/browser.xml,v  <--  browser.xml
new revision: 1.66; previous revision: 1.65
done
Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.92; previous revision: 1.91
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.454; previous revision: 1.453
done
Checking in browser/locales/en-US/chrome/browser/browser.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v  <--
 browser.properties
new revision: 1.16; previous revision: 1.15
done

Comment 9

[Giorgio Maone [:ma1]]

I know most likely answer will be *shrug*, but since JavaScript language allows
us to have optional parameters at the end of the list, why brutally changing
tabbrowser API adding an argument in the middle ("aButtonAcceskey"), thus
breaking every extension using the browser message feature (like NoScript, yes
I'm biased ;-) ) with no hope for graceful degrading?

Comment 10

[parente]

Enforced accessibility.

Comment 11

[Giorgio Maone [:ma1]]

(In reply to comment #10)
> Enforced accessibility.

Well, it is not the kind of answer I expected.
What I mean is, since browser message is present in current Aviary branch with
less parameters, why enforce addons developers to maintain *two different
versions* of their code just to accommodate the old API, when they could embrace
the new API right now just adding the accessibility parameter at the end of
their call, making both the current *stable* Firefox 1.0.5 and upcoming Firefox
1.1 happy?

Comment 12

[Aaron Leventhal]

Yeah, it is ideal to enforce accessibility but it is even more important not to
break any extensions. None of us thought about that, but we need to keep the
APIs stable now. That's the new world we have to live in.

Can an optional param be used? Reopening so we fix that.

Comment 13

[parente]

Are optional parameters possible in XBL?

http://www.mozilla.org/projects/xbl/xbl.html#parameter

Comment 14

[Giorgio Maone [:ma1]]

Attachment: Hack to preserve compatibility with the old API preserving the new arguments order

This patch uses a "Java-like polymorphism hack" to take back compatibility with
the old API preserving the new arguments order (wisely) choosen by Pete.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Hack indeed :). Why doesn't just moving it to the end of the list work?

Comment 16

[Giorgio Maone [:ma1]]

(In reply to comment #15)
> Hack indeed :). Why doesn't just moving it to the end of the list work?

It would work, but I did not want to compromise the "beauty"/rationality of the
new API just to save compatibility.
JavaScript flexibility gives us choice, hence better an ugly hack hidden (and
thus removable in future) behind a sane API than an ugly public API "justified"
by compatibility.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #16)
> It would work, but I did not want to compromise the "beauty"/rationality of the
> new API just to save compatibility.
> JavaScript flexibility gives us choice, hence better an ugly hack hidden (and
> thus removable in future) behind a sane API than an ugly public API "justified"
> by compatibility.

Personally I think that a slightly "uglier" public API is much better then using
a hack like this in the implementation. What happens when we want to add some
other arbitrary argument? Add on to the hack? That doesn't sound too good to me.
Is the order of arguments all that important anyways?

In any case, it's not up to me, so let's see what Mike thinks :).

Comment 18

[parente]

Even if you tack the param onto the end, is there a way to make it optional? I 
don't think so since XBL is defined to be language independent and must work 
with languages that don't support optional params.

Is there any point in putting the param at the end of the arg list if it's not 
optional? It still breaks backwards compatibility in that case.

Comment 19

[Giorgio Maone [:ma1]]

(In reply to comment #17)
> (In reply to comment #16)
> > JavaScript flexibility gives us choice, hence better an ugly hack hidden (and
> > thus removable in future) behind a sane API than an ugly public API "justified"
> > by compatibility.
> 
> Personally I think that a slightly "uglier" public API is much better then using
> a hack like this in the implementation. What happens when we want to add some
> other arbitrary argument? 

Talking about hacks, in an object oriented world such a long list of arguments
itself doesn't simply smell
(http://www.awprofessional.com/articles/article.asp?p=102271&seqNum=5&rl=1), it
actually stinks and badly needs refactoring.
I would never advise adding even more parameters to a public API method: as
grandma says, "if it stinks, change it".

BTW, is it actually public? Is there something like "frozen API" in the JS
toolkit realm? With literally thousands of Firefox addons around, not clearly
documenting what is public and frozen and what is private and
deprecatable/removable with no previous notice means making everything public
de-facto! 

>Add on to the hack? That doesn't sound too good to me.

APIs are for ever (ideally) and should be as good as possible, implementation
details are hidden (ideally) and can afford hacks.

> Is the order of arguments all that important anyways?

Today everybody agree that readability and is one of the most important (if not
"the" most important) feature of good code. 
The order of arguments communicates their importance and how they are related. A
well thought arguments list improves mnemonicity of the method signature; of
course, as said before, it should be *short* (more of two arguments smell).

(In reply to comment #18)
> Even if you tack the param onto the end, is there a way to make it optional? I 
> don't think so since XBL is defined to be language independent and must work 
> with languages that don't support optional params.

AFAIK no, there's no XBL formal way to flag a parameter as optional.
This only adds to the evil of a long parameter list - if you use an object as
parameter, you can always add properties even in the more rigid languages (by
inheritance). 
OTOH, the XBL specification draft you pointed, at
http://www.mozilla.org/projects/xbl/xbl.html#bindings ("type" attribute)
suggests (states? mandates? oh, clear documentation!) that only scripting
languages are used. Almost all the scripting languages I know support optional
parameters, not to mention that we both know this particular piece of code will
be only used with JavaScript :)

> Is there any point in putting the param at the end of the arg list if it's not 
> optional? It still breaks backwards compatibility in that case.

Mmmm... provided that my patch actually works, hence the "still breaks" argument
falls, one point I can see in favour of "putting the param at the end of the arg
list" is that it would be a more natural and self-documenting idiom to express
an optional parameter in JavaScript. I'm not sure this is actually a good thing,
if the new API wants to *enforce* accessibility: do we really want to
communicate the concept "access key is optional"?
But, nevertheless, this is a winning argument to me, because now that I can
think about it in the morning daylight with a coffee cup in my hand, I realize
that having the new argument at the end is the only way to let me support the
new API right now, with no impact on my code and with full backward/forward
compatibility, as my comment #11 itself suggested.

Hence, all the above said, I will vote for the Gavin's "uglier" API and provide
the relevant patch ;)

Comment 20

[Giorgio Maone [:ma1]]

Attachment: Moves "aButtonAccesskey" parameter at the end of tabbrowser.showMessage() signature

Changed my mind (again) ;) - see above comment #19

Comment 21

[parente]

The latest patch seems much more reasonable. It might be nice to note somewhere 
that the optional parameter is only optional for backwards compatibility and 
that developers should specify an access key whenever possible for the message 
bar button.

Comment 22

[Mike Connor [:mconnor]]

please land ASAP, not a blocker.

Comment 23

[Aaron Leventhal]

*** Bug 288499 has been marked as a duplicate of this bug. ***

Comment 24

[J]

This seems ridiculous to me. Users would rather intuitive interfaces than forced mnemonics. The loss of functionality in No Script is huge. I had it installed on all my clients, and it was easy to hit the big "feed bar" like button on the bottom to change options. Now, a user must mouse over to a specific corner to hit a tiny button. It’s harder to "sell" to clients. They don't see the benefits of safety anymore, it’s just an annoyance. Most of them are "allowing java script globally" which even warns of the dangers on the button in the extension. They do not care at this point. They just want quick easy browsing. Allowing JS globally defeats the safety of the extension and basically is like they are not using it.
The Firefox + No Script combo was a 1-2 punch against internet malware and malicious java script/java. Now, it's like No Script isn't even installed. Forcing mnemonics makes extensions less intuitive, not more. I'm sure in certain cases, yes, but forcing users to sacrifice security, fast browsing and intuitive interfaces is wrong.

Comment 25

[Aaron Leventhal]

(In reply to comment #24)
> This seems ridiculous to me. Allowing JS globally defeats the safety of the extension and ...

I don't understand what you're saying. How does allowing the user to press a hotkey to activate the buttons in a message bar allow JS globally?

Comment 26

[Giorgio Maone [:ma1]]

(In reply to comment #25)
> I don't understand what you're saying. How does allowing the user to press a
> hotkey to activate the buttons in a message bar allow JS globally?
>

Aaron, I believe the point in what user says is that the Firefox 1.0.x behaviour (activating options menu by clicking on any point of the message bar) was more quick and friendly to users who can take advantage of their mouse than being forced to explicitely target a (relatively tiny) button: an usability improvement for a minority (visually impaired or mouseless people) should not worsen the experience of the majority, when this can be avoided.

The implicit suggestion is that menus should show not only clicking on the button or typing its access key, but _also_ clicking on the message bar itself.

His JS related rant -- somewhat obscure to the non-initiated ;) -- comes from the fact that NoScript users are a category affected (and quite annoyed, apparently) by this change.

Comment 27

[Aaron Leventhal]

So let's keep the button and make the message bar still respond to a click anywhere in it.

The button is good because it makes the fact that there are options accessible to all users. But that doesn't mean we can't respond to clicks anywhere in the bar.

Anyway, this is the wrong bug for this. Whether a button has a keyboard mnemonic or not is a different issue from whether we respond to clicks in the message bar. The separate choice to go with buttons came before the clear choice to make those buttons accessible with mnemonics.

Comment 28

[J]

I apologize for not explaining the process in my earlier post. Thank you Giorgio, for getting my point across to Aaron. I am speaking on behalf of my end-users, who are most-disgruntled with the new, less-functional, NoScript extension.

(In reply to comment #27)
> So let's keep the button and make the message bar still respond to a click
> anywhere in it.

Aaron, does this mean that NoScipt will be able to have the old functionality back? If it does thank you for seeing the light in this matter. I think the idea of both a button and message bar would satisfy all parties.