12.34 KB, patch
|Details | Diff | Splinter Review|
7.70 KB, patch
|Details | Diff | Splinter Review|
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.
Assignee: doronr → 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.
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
Created attachment 187411 [details] [diff] [review] Fixes some nits pointed out by aaronleventh
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.
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 #188538 - Flags: approval1.8b4? → approval-aviary1.1a2?
Attachment #188538 - Flags: approval-aviary1.1a2? → approval1.8b4+
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: 13 years ago
Resolution: --- → FIXED
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Hack indeed :). Why doesn't just moving it to the end of the list work?
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.
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
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+
Attachment #189519 - Flags: approval1.8b4? → approval1.8b4+
please land ASAP, not a blocker.
Flags: blocking1.8b4? → blocking1.8b4-
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago → 13 years ago
Resolution: --- → FIXED
*** 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.
You need to log in before you can comment on or make changes to this bug.