All message bar buttons need mnemonics

RESOLVED FIXED

Status

()

P2
normal
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: aaronlev, Assigned: parente)

Tracking

({access})

unspecified
x86
All
access
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

14 years ago
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
(Reporter)

Updated

14 years ago
Priority: -- → P2
(Reporter)

Updated

14 years ago
Blocks: 291034
(Reporter)

Comment 1

14 years ago
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.
Assignee: doronr → parente
(Assignee)

Comment 2

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

Comment 3

14 years ago
Created attachment 187402 [details] [diff] [review]
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
(Assignee)

Updated

14 years ago
Attachment #187402 - Flags: review?(aaronleventhal)
(Assignee)

Comment 4

14 years ago
Created attachment 187411 [details] [diff] [review]
Fixes some nits pointed out by aaronleventh
Attachment #187402 - Attachment is obsolete: true
Attachment #187411 - Flags: review?(mconnor)
(Assignee)

Updated

14 years ago
Attachment #187402 - Flags: review?(aaronleventhal)
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
Attachment #187411 - Flags: review?(mconnor) → review+
bah, ignore the boolean part, I guess I hallucinated brackets there, but lets
stick to conventional formatting for consistency.
(Assignee)

Comment 7

14 years ago
Created attachment 188538 [details] [diff] [review]
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.
Attachment #187411 - Attachment is obsolete: true
Attachment #188538 - Flags: review?(mconnor)
(Assignee)

Updated

14 years ago
Attachment #188538 - Flags: review?(mconnor)
(Assignee)

Updated

14 years ago
Attachment #188538 - Flags: approval1.8b4?
(Reporter)

Updated

14 years ago
Attachment #188538 - Flags: approval1.8b4? → approval-aviary1.1a2?

Updated

14 years ago
Attachment #188538 - Flags: approval-aviary1.1a2? → approval1.8b4+
(Reporter)

Comment 8

14 years ago
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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
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?
(Assignee)

Comment 10

14 years ago
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?
(Reporter)

Comment 12

14 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

14 years ago
Are optional parameters possible in XBL?

http://www.mozilla.org/projects/xbl/xbl.html#parameter
Created attachment 189488 [details] [diff] [review]
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.
Attachment #189488 - Flags: review?(mconnor)
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 :).
(Assignee)

Comment 18

14 years ago
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 ;)
Created attachment 189519 [details] [diff] [review]
Moves "aButtonAccesskey" parameter at the end of tabbrowser.showMessage() signature 

Changed my mind (again) ;) - see above comment #19
Attachment #189488 - Attachment is obsolete: true

Updated

14 years ago
Attachment #189519 - Flags: review?(mconnor)

Updated

14 years ago
Attachment #189488 - Flags: review?(mconnor)
(Assignee)

Comment 21

14 years ago
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.
Attachment #189519 - Flags: review?(mconnor) → review+
(Reporter)

Updated

14 years ago
Attachment #189519 - Flags: approval1.8b4?

Updated

14 years ago
Flags: blocking1.8b4?

Updated

14 years ago
Attachment #189519 - Flags: approval1.8b4? → approval1.8b4+
please land ASAP, not a blocker.
Flags: blocking1.8b4? → blocking1.8b4-
(Reporter)

Updated

14 years ago
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
(Reporter)

Comment 23

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

Comment 24

13 years ago
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.
(Reporter)

Comment 25

13 years ago
(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.


(Reporter)

Comment 27

13 years ago
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

13 years ago
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.
You need to log in before you can comment on or make changes to this bug.