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

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.

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.

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

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

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

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

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?

Enforced accessibility.

(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?

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.

Are optional parameters possible in XBL? http://www.mozilla.org/projects/xbl/xbl.html#parameter

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.

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

(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.

(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 :).

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.

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

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

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.

please land ASAP, not a blocker.

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

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.

(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?

(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.

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.

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.