Issue 4780225334345728: Element Hiding Helper inspector tool fix (Closed)
|
Created:
April 8, 2014, 5:23 p.m. by saroyanm Modified:
April 12, 2014, 9:26 p.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionThis code review is a fix for current ticket:
https://issues.adblockplus.org/ticket/5
Patch Set 1 #
Total comments: 10
Patch Set 2 : fix after Wladimir comments #
Total comments: 11
Patch Set 3 : accesskey removed from inspector button #
Total comments: 2
MessagesTotal messages: 11
@Wladimir, can you please have a look when you will have time, Thanks in advance.
http://codereview.adblockplus.org/4780225334345728/diff/5629499534213120/lib/... File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/4780225334345728/diff/5629499534213120/lib/... lib/inspectorObserver.js:7: Cu.import("resource:///modules/devtools/gDevTools.jsm"); While you are at it, please don't use a blind import here (we need to change this everywhere else as well, it's just not a priority): let {gDevTools} = Cu.import("resource:///modules/devtools/gDevTools.jsm", null); http://codereview.adblockplus.org/4780225334345728/diff/5629499534213120/lib/... lib/inspectorObserver.js:31: inspectorReady: function(eventName, listener) The second parameter here isn't listener but panel (ToolPanel instance). http://codereview.adblockplus.org/4780225334345728/diff/5629499534213120/lib/... lib/inspectorObserver.js:33: let toolbox = gDevTools.getToolbox(listener.target); I checked why the documentation mentions getToolboxByTarget() method instead - it seems that the API changed with https://bugzilla.mozilla.org/show_bug.cgi?id=813031. That means that the API we are using here was introduced in Firefox 20 - please update metadata.gecko file to make sure we don't support any earlier versions. http://codereview.adblockplus.org/4780225334345728/diff/5629499534213120/lib/... lib/inspectorObserver.js:39: return; So you are going from the panel to the toolbox to get the panel :) http://codereview.adblockplus.org/4780225334345728/diff/5629499534213120/lib/... lib/inspectorObserver.js:59: parent.appendChild(button); Adding the button at the end probably isn't the best solution, it should be placed before the inspector-breadcrumbs element. Also, the button occupies too much space right now. You should use button.style.listStyleImage to use the Adblock Plus icon for it and remove the label. The tooltip should be good enough to explain what it is about.
Uploaded a new patch, hope this will work for us. http://codereview.adblockplus.org/4780225334345728/diff/5629499534213120/lib/... File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/4780225334345728/diff/5629499534213120/lib/... lib/inspectorObserver.js:7: Cu.import("resource:///modules/devtools/gDevTools.jsm"); On 2014/04/11 11:06:08, Wladimir Palant wrote: > While you are at it, please don't use a blind import here (we need to change > this everywhere else as well, it's just not a priority): > > let {gDevTools} = Cu.import("resource:///modules/devtools/gDevTools.jsm", null); Got it. http://codereview.adblockplus.org/4780225334345728/diff/5629499534213120/lib/... lib/inspectorObserver.js:31: inspectorReady: function(eventName, listener) On 2014/04/11 11:06:08, Wladimir Palant wrote: > The second parameter here isn't listener but panel (ToolPanel instance). Good catch, actually I've tested and seams like the parameters are: 1. eventName (string) 2. toolBox (Toolbox) 3. panel (ToolPanel) example: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/tilt/tilt-visu... http://codereview.adblockplus.org/4780225334345728/diff/5629499534213120/lib/... lib/inspectorObserver.js:33: let toolbox = gDevTools.getToolbox(listener.target); On 2014/04/11 11:06:08, Wladimir Palant wrote: > I checked why the documentation mentions getToolboxByTarget() method instead - > it seems that the API changed with > https://bugzilla.mozilla.org/show_bug.cgi?id=813031. That means that the API we > are using here was introduced in Firefox 20 - please update metadata.gecko file > to make sure we don't support any earlier versions. Good point: I've tested with FF19 and there is an error for current line: Cu.import("resource:///modules/devtools/gDevTools.jsm"); Tested with FF20 and worked great, as you told :) http://codereview.adblockplus.org/4780225334345728/diff/5629499534213120/lib/... lib/inspectorObserver.js:39: return; On 2014/04/11 11:06:08, Wladimir Palant wrote: > So you are going from the panel to the toolbox to get the panel :) Lol :) http://codereview.adblockplus.org/4780225334345728/diff/5629499534213120/lib/... lib/inspectorObserver.js:59: parent.appendChild(button); On 2014/04/11 11:06:08, Wladimir Palant wrote: > Adding the button at the end probably isn't the best solution, it should be > placed before the inspector-breadcrumbs element. > > Also, the button occupies too much space right now. You should use > button.style.listStyleImage to use the Adblock Plus icon for it and remove the > label. The tooltip should be good enough to explain what it is about. Good point changed. Thanks for pointing. http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... lib/inspectorObserver.js:41: button.style.listStyleImage = "url('chrome://adblockplus/skin/abp-status-16.png')"; Not sure if I need to copy icon to elemhidehelper/chrome/skin/abp-status-16.png ?
http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/chro... File chrome/locale/en-US/global.properties (right): http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/chro... chrome/locale/en-US/global.properties:34: inspector.button.accesskey=A If you remove the label then you should remove the access key as well. For the future: it is enough to change strings in en-US. Other locales will be changed automatically when translations are imported. http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... lib/inspectorObserver.js:24: let result = [stringBundle.GetStringFromName("inspector.button.accesskey"), stringBundle.GetStringFromName("inspector.button.tooltiptext")]; An access key only makes sense in combination with a label. http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... lib/inspectorObserver.js:41: button.style.listStyleImage = "url('chrome://adblockplus/skin/abp-status-16.png')"; On 2014/04/11 16:09:11, saroyanm wrote: > Not sure if I need to copy icon to elemhidehelper/chrome/skin/abp-status-16.png > ? No, it's fine like that - Element Hiding Helper can rely on Adblock Plus. http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/meta... File metadata.gecko (right): http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/meta... metadata.gecko:25: seamonkey=2.13/2.29 SeaMonkey 2.17 corresponds to Firefox 20.
http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... lib/inspectorObserver.js:24: let result = [stringBundle.GetStringFromName("inspector.button.accesskey"), stringBundle.GetStringFromName("inspector.button.tooltiptext")]; On 2014/04/11 18:39:10, Wladimir Palant wrote: > An access key only makes sense in combination with a label. Wladimir why we need to remove accesskey ? Isn't it useful for quickly access the action ? Not sure if I understand how they are related, can you please drop a comment on this.
http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... lib/inspectorObserver.js:24: let result = [stringBundle.GetStringFromName("inspector.button.accesskey"), stringBundle.GetStringFromName("inspector.button.tooltiptext")]; You probably don't know what access keys are - see https://developer.mozilla.org/en/docs/XUL_Accesskey_FAQ_and_Policies.
http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... lib/inspectorObserver.js:24: let result = [stringBundle.GetStringFromName("inspector.button.accesskey"), stringBundle.GetStringFromName("inspector.button.tooltiptext")]; On 2014/04/12 18:14:11, Wladimir Palant wrote: > You probably don't know what access keys are - see > https://developer.mozilla.org/en/docs/XUL_Accesskey_FAQ_and_Policies. Thanks for the reference Wladimir, actually I feel myself that I still don't understand why we need to use inspector button in this case without accesskey attribute even if we don't have label attribute, Sorry if I understand your point wrong, but let me describe how I understand Accesskeys: Accesskey is used to simulate click on the element and I'm not sure why it's only make sense in combination with label in our case even if we will have no underline hint(A) in label for accesskey. I feel myself that it's quite useful feature from usability point of view. Maybe I'm missing something, sorry for that beforehand. Anyway if that is the point to only have accesskeys with combination of label or underline letter hint, maybe we can use some other combination or use Ctrl+Shift+f3 in case of inspector opened, what you think ?
http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... lib/inspectorObserver.js:24: let result = [stringBundle.GetStringFromName("inspector.button.accesskey"), stringBundle.GetStringFromName("inspector.button.tooltiptext")]; An access key is not the same thing as a shortcut key. An access key is the underlined letter of a button or some other control - e.g. if "a" is underlined then you know that you can press Alt-A to trigger that button. A shortcut key on the other hand can be any key combination that triggers any action, not necessarily related to a particular control. Shortcut keys are usually assigned to menu items, that's also how users are normally supposed to discover them. A shortcut key makes no sense in this context, there is no way for the user to find out about it. Also, buttons normally don't have shortcut keys. As to access keys, an access key requires a label - image-only buttons cannot have one.
Uploaded the new patch, hope that will work this time. http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... lib/inspectorObserver.js:24: let result = [stringBundle.GetStringFromName("inspector.button.accesskey"), stringBundle.GetStringFromName("inspector.button.tooltiptext")]; On 2014/04/12 20:23:20, saroyanm wrote: > On 2014/04/12 18:14:11, Wladimir Palant wrote: > > You probably don't know what access keys are - see > > https://developer.mozilla.org/en/docs/XUL_Accesskey_FAQ_and_Policies. > > Thanks for the reference Wladimir, > actually I feel myself that I still don't understand why we need to use > inspector button in this case without accesskey attribute even if we don't have > label attribute, > Sorry if I understand your point wrong, but let me describe how I understand > Accesskeys: > Accesskey is used to simulate click on the element and I'm not sure why it's > only make sense in combination with label in our case even if we will have no > underline hint(A) in label for accesskey. > I feel myself that it's quite useful feature from usability point of view. > Maybe I'm missing something, sorry for that beforehand. > Anyway if that is the point to only have accesskeys with combination of label or > underline letter hint, maybe we can use some other combination or use > Ctrl+Shift+f3 in case of inspector opened, what you think ? I thought again regarding your notes Wladimir I guess you are right as usual, just forget about upper notes :) It's still make sense to follow the rules for using accesskey in case there will be other extension or feature implemented in FF that uses same accesskey in inspector. The button should be enough. http://codereview.adblockplus.org/4780225334345728/diff/5738600293466112/lib/... File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/4780225334345728/diff/5738600293466112/lib/... lib/inspectorObserver.js:20: get inspectorButton() Not sure if it's make sense to change getter into inspectorButtonTooltip ?
http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/4780225334345728/diff/5724160613416960/lib/... lib/inspectorObserver.js:24: let result = [stringBundle.GetStringFromName("inspector.button.accesskey"), stringBundle.GetStringFromName("inspector.button.tooltiptext")]; On 2014/04/12 20:51:26, Wladimir Palant wrote: > An access key is not the same thing as a shortcut key. An access key is the > underlined letter of a button or some other control - e.g. if "a" is underlined > then you know that you can press Alt-A to trigger that button. > > A shortcut key on the other hand can be any key combination that triggers any > action, not necessarily related to a particular control. Shortcut keys are > usually assigned to menu items, that's also how users are normally supposed to > discover them. > > A shortcut key makes no sense in this context, there is no way for the user to > find out about it. Also, buttons normally don't have shortcut keys. As to access > keys, an access key requires a label - image-only buttons cannot have one. Got it. Thanks.
LGTM if the property name is changed. http://codereview.adblockplus.org/4780225334345728/diff/5738600293466112/lib/... File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/4780225334345728/diff/5738600293466112/lib/... lib/inspectorObserver.js:20: get inspectorButton() On 2014/04/12 20:59:38, saroyanm wrote: > Not sure if it's make sense to change getter into inspectorButtonTooltip ? Yes, I think the name should be changed. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
