Based on the new UI, there will not be a dropdown next to non-default permissions anymore. Instead there will be an 'x' icon that reverts it back to default. See the latest designs here https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/100404627, or the attached mockup for an example.

Aislinn, could you help attach the cross sign icon in svg format?

(In reply to Fred Lin [:gasolin] from comment #1) > Aislinn, could you help attach the cross sign icon in svg format? flagging bryan as he created these

According to bug 1193006 comment 32, we can use the .close-icon class (same X that we use everywhere else (e.g. notification bars and tabs))

Found out UI part source at https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?case=true&from=browser%2Fbase%2Fcontent%2Fbrowser.js#7143-7177 A huge browser.js creates lots of UI elements dynamically. \o/

After user clicks 'x' icon to revert default permission, it will display a "restore" icon as design mockup at https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/100296204. I couldn't find any reusable "restore" icon in toolkit. Could we create a new one for it?

(In reply to (Unavailable until May 23) Brian Grinstead [:bgrins] from comment #0) > Based on the new UI, there will not be a dropdown next to non-default > permissions anymore. Instead there will be an 'x' icon that reverts it back > to default. How about default state is same as current state? IIUC, 'x' button is designed for reverting to default state. However, the current state could be default state that causes showing a 'x' button in this situation is weird.

We're updating this dropbox link to include the X and the reload icon. https://www.dropbox.com/s/qfct8j9wt4idalu/Screenshot%202016-05-20%2014.26.03.png?dl=0

Thanks, I still want to know the answer of comment 6 :)

(In reply to agrigas from comment #7) > We're updating this dropbox link to include the X and the reload icon. > https://www.dropbox.com/s/qfct8j9wt4idalu/Screenshot%202016-05-20%2014.26.03. > png?dl=0 This link was wrong. see https://www.dropbox.com/sh/uqzkxu8ku3hk3lj/AABRq5Z6snQsVvUYD7hl5Qyoa/Parts/Individual%20icons/glyph-reload-16.svg?dl=0 On the other hand, we have to get every single icon with different state such as :hover and :hover:active. My current patch reused .close-icon that already supported :hover and :hover:active state and it seems that a somewhat inconsistent after applying new glyph-reload-16.svg on it. (see attachment)

(In reply to Ricky Chien [:rickychien] from comment #6) > (In reply to (Unavailable until May 23) Brian Grinstead [:bgrins] from > comment #0) > > Based on the new UI, there will not be a dropdown next to non-default > > permissions anymore. Instead there will be an 'x' icon that reverts it back > > to default. > > How about default state is same as current state? > > IIUC, 'x' button is designed for reverting to default state. However, the > current state could be default state that causes showing a 'x' button in > this situation is weird. The permissions shown in the control center are not every possible permission as shows currenty but rather only permissions that are default 'always ask.' Any permission that has a notification that asks a user to make a choice shows in the control panel and the X allows them to change their decision later on. For users that want to change any permissions that are default to always allow or always block, they would have to go into the Page info menu under Tools.

> The permissions shown in the control center are not every possible > permission as shows currenty but rather only permissions that are default > 'always ask.' Any permission that has a notification that asks a user to > make a choice shows in the control panel and the X allows them to change > their decision later on. Does that mean Only items set `Always ask` in site permission tab should show `x` icon? Tap `x` icon will > > For users that want to change any permissions that are default to always > allow or always block, they would have to go into the Page info menu under > Tools. So only items with `Always ask` permission should show in Control Center/Door hanger section? If item with `always allow/block` still need to be shown, what UI should be shown instead of `x` button?

I guess the scenario is: (If I'm wrong please correct me) If someone changes the permission to 'allow' or 'block' in notification panel for those permissions whose default value are 'always ask'. User can click X button on permission panel to revert the current 'allow' or 'block' state to default 'always ask'. I'm still confused about the behavior of 'X' icon that feel like to cancel a current permission state ('allow' / 'block') or convert current permission to a reversed permission (allow to block).

(In reply to Ricky Chien [:rickychien] from comment #13) > I guess the scenario is: (If I'm wrong please correct me) > > If someone changes the permission to 'allow' or 'block' in notification > panel for those permissions whose default value are 'always ask'. User can > click X button on permission panel to revert the current 'allow' or 'block' > state to default 'always ask'. > > I'm still confused about the behavior of 'X' icon that feel like to cancel a > current permission state ('allow' / 'block') or convert current permission > to a reversed permission (allow to block). Yes that is correct. We don't want to show a list of permissions that the user isn't playing an active part in setting. If there are permissions we set for them, most users don't have to think about changing them. If an advanced user wants to, they can go to the Tools menu. The 'X' button is basically provides the user to 'undo' their decision. If they decided over time they don't want to allow a site to use their location because they become more privacy minded, they can clear out that decision and be asked again. They may decide to just allow it temporarily. The X also helps in the case where someone was busy and didn't want to read the dialogue and clicked 'Don't allow' even though they wanted to allow the permission later on. The crossed out icon in the url bar leads them into the control panel where they can click the X and be asked again if they want to allow/block that permission.

Comment on attachment 8755375 [details] reload-icon+close-icon.png As comment 9 mentioned, we need at least four different (normal, hover, active + hover, active) button icons for each permission. As attachment pic, the original style of 'x' icon is inconsistent with reload icon design. Should we update it?

(In reply to agrigas from comment #16) > (In reply to Ricky Chien [:rickychien] from comment #15) > > Comment on attachment 8755375 [details] > > reload-icon+close-icon.png > > > > As comment 9 mentioned, we need at least four different (normal, hover, > > active + hover, active) button icons for each permission. As attachment pic, > > the original style of 'x' icon is inconsistent with reload icon design. > > Should we update it? > > The X and the reload icon can be found here: > https://www.dropbox.com/sh/rffviyl7m4x7p7c/AABoqFM6EqO3opLSds01TnYla?dl=0 > > The hover states for them I am getting specked out from the visual designer > as he didn't include them and will follow up with them here soon. here is a spec for the hover states - its a live code mockup so hover over icons to see state change: http://people.mozilla.org/~shorlander/mockups-interactive/panel-mockups/panel-control-center.html

(In reply to Ricky Chien [:rickychien] from comment #15) > Comment on attachment 8755375 [details] > reload-icon+close-icon.png > > As comment 9 mentioned, we need at least four different (normal, hover, > active + hover, active) button icons for each permission. As attachment pic, > the original style of 'x' icon is inconsistent with reload icon design. > Should we update it? The X and the reload icon can be found here: https://www.dropbox.com/sh/rffviyl7m4x7p7c/AABoqFM6EqO3opLSds01TnYla?dl=0 The hover states for them I am getting specked out from the visual designer as he didn't include them and will follow up with them here soon.

Thank you agrigas. My latest patch has stolen these icons with hover states from the mockup site. A new question is that should we hide 'x' icon for those permission whose current state is same with default state? For example, Install Add-ons's default state is 'block' that will show up on panel with a 'x' button. It would introduce a confused if user clicks 'x' to revert to its default state which is 'block' too.

(In reply to Ricky Chien [:rickychien] from comment #18) > Thank you agrigas. My latest patch has stolen these icons with hover states > from the mockup site. > > A new question is that should we hide 'x' icon for those permission whose > current state is same with default state? > > For example, Install Add-ons's default state is 'block' that will show up on > panel with a 'x' button. It would introduce a confused if user clicks 'x' to > revert to its default state which is 'block' too. Only permissions that the user has had to make a decision with a notification prompt should appear - those are all notifications that have a default state of 'always ask.' When the user selects 'allow' or 'block' from the notification, then the item appears in the control panel with an X. If the user clicks the X it restores the permission to the default of always ask and they are prompted again on reload. Permissions with a default of always allow or always block do not show in the control center.

Review commit: https://reviewboard.mozilla.org/r/55672/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/55672/

Thanks for clarifying that! patch has submitted.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/1-2/

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon https://reviewboard.mozilla.org/r/55672/#review52448 Since we have bug 1206229 for the "reload" behavior, we can probably work on just the button change here, and just refresh the panel when the permission is changed, which will result in the line being removed. We can then decide if we need to land this on a separate branch or if the intermediate state makes sense. I haven't reviewed the theme changes yet. Proably we will add the new icons to those from bug 1274480 once that lands. ::: browser/base/content/browser.js:7160 (Diff revision 2) > - if (state == SitePermissions.UNKNOWN) > - continue; > + if (state != SitePermissions.UNKNOWN && > + SitePermissions.getDefault(permission) == SitePermissions.UNKNOWN) { Can you explain what this logic is checking? ::: browser/base/content/browser.js:7168 (Diff revision 2) > - setPermission: function (aPermission, aState) { > - if (aState == SitePermissions.getDefault(aPermission)) > - SitePermissions.remove(gBrowser.currentURI, aPermission); > - else > + setPermission: function (aPermission, aState, button) { > + let state = SitePermissions.get(gBrowser.currentURI, aPermission); > + > + if (state == SitePermissions.getDefault(aPermission)) { > + button.setAttribute('class', 'cancel'); > SitePermissions.set(gBrowser.currentURI, aPermission, aState); > + } else { > + button.setAttribute('class', 'reload'); > + SitePermissions.remove(gBrowser.currentURI, aPermission); > + } It's unclear to me what this code is trying to accomplish. Do we need this logic? setPermission was used as a callback function for an event that could set a permission to many possible states, it seems it could be simpler if we only need to revert a decision. This was also used by regression tests. Did you run the browser-chrome tests for permissions to see how they are impacted? ::: browser/base/content/browser.js:7186 (Diff revision 2) > - menuitem.setAttribute("label", SitePermissions.getStateLabel(aPermission, state)); > - menupopup.appendChild(menuitem); > - } > - menulist.appendChild(menupopup); > + let stateLabel = document.createElement("label"); > + stateLabel.setAttribute("align", "right"); > + stateLabel.setAttribute("class", "identity-popup-permission-state-label"); > + stateLabel.textContent = SitePermissions.getStateLabel(aPermission, aState); What is the impact of having a state label present in the main area if the permission label or the state label are a long string? Can you post screenshots? To have a good average reference for localization, you can edit the state strings temporarily to be something like 30% longer than the English version. ::: browser/base/content/browser.js:7195 (Diff revision 2) > - menulist.setAttribute("value", aState); > - menulist.setAttribute("oncommand", "gIdentityHandler.setPermission('" + > - aPermission + "', this.value)"); > - menulist.setAttribute("id", "identity-popup-permission:" + aPermission); > - > - let label = document.createElement("label"); > + > + let button = document.createElement("button"); > + button.setAttribute("align", "right"); > + button.setAttribute("class", "cancel"); > + button.addEventListener("click", () => { > + gIdentityHandler.setPermission(aPermission, aState, button) missing semicolon Please use "r?paolo" in the commit message, lowercase. In general you should specify just one word, reviewer nicknames in commit messages don't have spaces.

(In reply to :Paolo Amadini from comment #23) > Comment on attachment 8757123 [details] > MozReview Request: Bug 1203292 - Replace permissions dropdown with 'x' icon > r?Paolo Amadini > > https://reviewboard.mozilla.org/r/55672/#review52448 > > Since we have bug 1206229 for the "reload" behavior, we can probably work on > just the button change here, and just refresh the panel when the permission > is changed, which will result in the line being removed. We can then decide > if we need to land this on a separate branch or if the intermediate state > makes sense. > > I haven't reviewed the theme changes yet. Proably we will add the new icons > to those from bug 1274480 once that lands. > > ::: browser/base/content/browser.js:7160 > (Diff revision 2) > > - if (state == SitePermissions.UNKNOWN) > > - continue; > > + if (state != SitePermissions.UNKNOWN && > > + SitePermissions.getDefault(permission) == SitePermissions.UNKNOWN) { > > Can you explain what this logic is checking? > > ::: browser/base/content/browser.js:7168 > (Diff revision 2) > > - setPermission: function (aPermission, aState) { > > - if (aState == SitePermissions.getDefault(aPermission)) > > - SitePermissions.remove(gBrowser.currentURI, aPermission); > > - else > > + setPermission: function (aPermission, aState, button) { > > + let state = SitePermissions.get(gBrowser.currentURI, aPermission); > > + > > + if (state == SitePermissions.getDefault(aPermission)) { > > + button.setAttribute('class', 'cancel'); > > SitePermissions.set(gBrowser.currentURI, aPermission, aState); > > + } else { > > + button.setAttribute('class', 'reload'); > > + SitePermissions.remove(gBrowser.currentURI, aPermission); > > + } > > It's unclear to me what this code is trying to accomplish. Do we need this > logic? setPermission was used as a callback function for an event that could > set a permission to many possible states, it seems it could be simpler if we > only need to revert a decision. > > This was also used by regression tests. Did you run the browser-chrome tests > for permissions to see how they are impacted? > > ::: browser/base/content/browser.js:7186 > (Diff revision 2) > > - menuitem.setAttribute("label", SitePermissions.getStateLabel(aPermission, state)); > > - menupopup.appendChild(menuitem); > > - } > > - menulist.appendChild(menupopup); > > + let stateLabel = document.createElement("label"); > > + stateLabel.setAttribute("align", "right"); > > + stateLabel.setAttribute("class", "identity-popup-permission-state-label"); > > + stateLabel.textContent = SitePermissions.getStateLabel(aPermission, aState); > > What is the impact of having a state label present in the main area if the > permission label or the state label are a long string? Can you post > screenshots? > > To have a good average reference for localization, you can edit the state > strings temporarily to be something like 30% longer than the English version. > > ::: browser/base/content/browser.js:7195 > (Diff revision 2) > > - menulist.setAttribute("value", aState); > > - menulist.setAttribute("oncommand", "gIdentityHandler.setPermission('" + > > - aPermission + "', this.value)"); > > - menulist.setAttribute("id", "identity-popup-permission:" + aPermission); > > - > > - let label = document.createElement("label"); > > + > > + let button = document.createElement("button"); > > + button.setAttribute("align", "right"); > > + button.setAttribute("class", "cancel"); > > + button.addEventListener("click", () => { > > + gIdentityHandler.setPermission(aPermission, aState, button) > > missing semicolon > > Please use "r?paolo" in the commit message, lowercase. In general you should > specify just one word, reviewer nicknames in commit messages don't have > spaces. we don't want to refresh for user's after clicking the X in the released version because someone could have unsaved content on the page and not realize it will be lost (ie. wrtitten form fields, uploads, etc)... please don't auto-refresh for release

(In reply to agrigas from comment #24) > we don't want to refresh for user's after clicking the X in the released > version because someone could have unsaved content on the page and not > realize it will be lost (ie. wrtitten form fields, uploads, etc)... To clarify, I'm talking about refreshing the state of the panel, not auto-refresing the web page when the permission is changed. I think this would result in the permission line on which the user clicked to be removed immediately, instead of remaining there until the panel is closed, like it does now when a permission is changed back to the default from the drop-down. Another intermediate state could be to make the X disappear but keep the line still there until the panel is closed. It's a bit more work though. Do you have any preference?

(In reply to :Paolo Amadini from comment #25) > (In reply to agrigas from comment #24) > > we don't want to refresh for user's after clicking the X in the released > > version because someone could have unsaved content on the page and not > > realize it will be lost (ie. wrtitten form fields, uploads, etc)... > > To clarify, I'm talking about refreshing the state of the panel, not > auto-refresing the web page when the permission is changed. > > I think this would result in the permission line on which the user clicked > to be removed immediately, instead of remaining there until the panel is > closed, like it does now when a permission is changed back to the default > from the drop-down. > > Another intermediate state could be to make the X disappear but keep the > line still there until the panel is closed. It's a bit more work though. Do > you have any preference? Ah - ok. Removing the line is fine. We'll still have the text show at the bottom saying 'reload' correct?

(In reply to agrigas from comment #26) > Ah - ok. Removing the line is fine. We'll still have the text show at the > bottom saying 'reload' correct? Yeah, that's part of bug 1206229.

https://reviewboard.mozilla.org/r/55672/#review52448 > Can you explain what this logic is checking? According to comment 14 and comment 19, checking each permission which default value is 'always ask' (SitePermissions.UNKNOWN) and its value has been changed by notification popup should be displayed on panel. > It's unclear to me what this code is trying to accomplish. Do we need this logic? setPermission was used as a callback function for an event that could set a permission to many possible states, it seems it could be simpler if we only need to revert a decision. > > This was also used by regression tests. Did you run the browser-chrome tests for permissions to see how they are impacted? One condition is for reverting to default 'always ask' and another one is for reverting to original state, so I kept almost original logic here to achieve our requirements. I forgot to clarify here I just need your feedback first and make sure we're not off the track and following the spec. I would begin to fix these impacted tests for permissions once we agree with this changeset :)

(In reply to Ricky Chien [:rickychien] from comment #28) > I forgot to clarify here I just need your feedback first and make sure we're > not off the track and following the spec. I would begin to fix these > impacted tests for permissions once we agree with this changeset :) Good, so based on the discussion with Aislinn I think that there is more logic than we need in this first version, we just need the line to be removed when the "X" button is clicked. In bug 1206229 we will keep the line with the "reload" icon, but what the specification asks for is only a "reload" icon that is not clickable, we don't need an "undo" icon that gives a way to restore the previous state. If the X is clicked accidentally, it's easy to grant the permission again when asked. When checking if permissions are set to a non-default value, we only need to check if the current "get" call returns UNKNOWN. A bit counter-intuitively, the nsIPermissions API returns UNKNOWN to indicate that there is no entry in the database, and the caller should use other logic to know what the default value is.

Thanks, removing the line (permission item) works well but the height of permission panel doesn't shrink automatically as expected. According to [1], subView (permission panel) should be aware of deletion of child permissions and then update its height at [2] (If I'm wrong please correct me) but it's not. Paolo, do you have any idea of how it works? Or is there better solution to update height according to permission item? [1] https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.xml?from=browser%2Fcomponents%2Fcustomizableui%2Fcontent%2FpanelUI.xml#211-216 [2] https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.xml?from=browser%2Fcomponents%2Fcustomizableui%2Fcontent%2FpanelUI.xml#375-376

(In reply to Ricky Chien [:rickychien] from comment #30) > Thanks, removing the line (permission item) works well but the height of > permission panel doesn't shrink automatically as expected. Apparently the code you pointed to only affects the panel while a sliding subview is visible. I see Gijs has reviewed panelUI.xml code, maybe he knows if there is a known issue for which the panel does not resize automatically when the main view is visible. If you have a work in progress patch it might help.

(In reply to :Paolo Amadini from comment #31) > (In reply to Ricky Chien [:rickychien] from comment #30) > > Thanks, removing the line (permission item) works well but the height of > > permission panel doesn't shrink automatically as expected. > > Apparently the code you pointed to only affects the panel while a sliding > subview is visible. I see Gijs has reviewed panelUI.xml code, maybe he knows > if there is a known issue for which the panel does not resize automatically > when the main view is visible. If you have a work in progress patch it might > help. XUL panels generally do not adjust their size for content. The hacks in panelUI.xml... kind of work... it isn't clear to me right now what the issue is. Have you verified if the binding's _syncContainerWithMainView method is being called or not? If it is being called, do you know why it's not updating the size? (I'm assuming this is the main view, not a subview - if it's a subview, same question but for _syncContainerWithSubView.)

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/2-3/

https://reviewboard.mozilla.org/r/55672/#review54620 ::: browser/components/controlcenter/content/panel.inc.xul:93 (Diff revision 3) > <hbox class="identity-popup-section"> > <vbox id="identity-popup-permissions-content" flex="1"> > <label class="identity-popup-headline" > value="&identity.permissions;"/> > <vbox id="identity-popup-permission-list"/> > - <description>&identity.permissionsEmpty;</description> > + <description style="height: 30px;">&identity.permissionsEmpty;</description> The description appears when permission panel is empty. After looking into inspector's box model here that shows with a 30px height but it's not in fact (I don't know why). Give a specific 30px height can workaround this issue.

My previous description on comment 30 was wrong so please ignore this question. I've updated my patch and almost my job was that figure out how to shrink the permission panel itself depends on permission items. There are so many manual-calculated heights in xul elements and I hope this patch doesn't break anything. Likewise, I haven't write test yet and just want to get a feedback first until we agree with an appropriate solution.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon https://reviewboard.mozilla.org/r/55672/#review55846 ::: browser/base/content/browser.js:7160 (Diff revision 3) > - if (state == SitePermissions.UNKNOWN) > - continue; > + if (state != SitePermissions.UNKNOWN && > + SitePermissions.getDefault(permission) == SitePermissions.UNKNOWN) { > - > - let item = this._createPermissionItem(permission, state); > + let item = this._createPermissionItem(permission, state); > - this._permissionList.appendChild(item); > + this._permissionList.appendChild(item); > - } > + } > + } This change is not needed, right? ::: browser/base/content/browser.js:7180 (Diff revision 3) > + let button = document.createElement("button"); > + button.setAttribute("align", "right"); > + button.setAttribute("class", "cancel"); > + button.addEventListener("click", () => { > + this._permissionList.removeChild(container); > + SitePermissions.remove(gBrowser.currentURI, aPermission); > + }); Looks good, but needs to be rebased on top of the lastest changes landed in mozilla-central. ::: browser/components/controlcenter/content/panel.inc.xul:93 (Diff revision 3) > - <description>&identity.permissionsEmpty;</description> > + <description style="height: 30px;">&identity.permissionsEmpty;</description> > </vbox> I suspect we'll need to find a way to avoid a hardcoded height. I know multi-line <description> elements cause issues with automatic window sizing, I had to use code like this before: element.style.height = window.getComputedStyle(element).height; This was for a fixed-width independent window though, I don't know if this is the right solution here. ::: browser/components/customizableui/content/panelUI.xml (Diff revision 3) > - container.style.height = `${aHeight}px`; > ]]></body> I'm not familiar with panelUI.xml, Gijs should review this. ::: browser/themes/shared/jar.inc.mn:28 (Diff revision 3) > skin/classic/browser/addons/addon-install-restart.svg (../shared/addons/addon-install-restart.svg) > skin/classic/browser/addons/addon-install-warning.svg (../shared/addons/addon-install-warning.svg) > skin/classic/browser/addons/addon-install-anchor.svg (../shared/addons/addon-install-anchor.svg) > skin/classic/browser/controlcenter/arrow-subview.svg (../shared/controlcenter/arrow-subview.svg) > skin/classic/browser/controlcenter/arrow-subview-back.svg (../shared/controlcenter/arrow-subview-back.svg) > + skin/classic/browser/controlcenter/cancel.svg (../shared/controlcenter/cancel.svg) The cancel icon can now be a glyph as soon as bug 1274480 lands. ::: browser/themes/shared/jar.inc.mn:34 (Diff revision 3) > skin/classic/browser/controlcenter/conn-not-secure.svg (../shared/controlcenter/conn-not-secure.svg) > skin/classic/browser/controlcenter/conn-degraded.svg (../shared/controlcenter/conn-degraded.svg) > skin/classic/browser/controlcenter/conn-secure.svg (../shared/controlcenter/conn-secure.svg) > skin/classic/browser/controlcenter/mcb-disabled.svg (../shared/controlcenter/mcb-disabled.svg) > skin/classic/browser/controlcenter/permissions.svg (../shared/controlcenter/permissions.svg) > + skin/classic/browser/controlcenter/reload.svg (../shared/controlcenter/reload.svg) This is now unused and can be removed.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon For the panelUI.xml changes.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Removing all this code outright is almost certainly wrong and would lead to the main menu panel not resizing properly either initially, if its content has changed since last opening the panel, or after leaving a subview and returning to the main panel. What are you actually trying to accomplish?

Hey Ricky, do you still plan to work on this in the following days?

I'm switching back and will be focusing on it now.

How can I convert shared/controlcenter/cancel.svg from 16x16 to 32 x 32? Is there any online tool or command line tool that is able to covert it easily? thanks!

I've converted them via inkscape <path id="cancel-default" d="M30,6.81,20.7,16.1,29.9,25.3,25.3,30,16.1,20.8,6.93,29.9,2.27,25.3,11.4,16.1,2,6.67,6.66,2,16.1,11.4,25.3,2.14z"/> And you may able to use the same icon with different color filters

Sorry for clearing this ni accidentally. I'm still wondering what is the best way or tool we can use for converting / optimizing svg. (see comment 41)

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/3-4/

https://reviewboard.mozilla.org/r/55672/#review55846 > This change is not needed, right? According to comment 19, we should check and display these permissions whoes default value are not set to 'always ask'. > I suspect we'll need to find a way to avoid a hardcoded height. I know multi-line <description> elements cause issues with automatic window sizing, I had to use code like this before: > > element.style.height = window.getComputedStyle(element).height; > > This was for a fixed-width independent window though, I don't know if this is the right solution here. Is it a bug of multi-line description element? I noticed that some of description elements don't trigger this kind of issue but some of them do. > The cancel icon can now be a glyph as soon as bug 1274480 lands. I'll request a new 32 x 32 svg for glyph.svg use.

> I'll request a new 32 x 32 svg for glyph.svg use. Uh, cancel icon has been integrated into glyph in my patch so I don't need to request the new one anymore.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/4-5/

(In reply to Ricky Chien [:rickychien] from comment #41) > How can I convert shared/controlcenter/cancel.svg from 16x16 to 32 x 32? Is > there any online tool or command line tool that is able to covert it easily? > thanks! As Fred noted we've used Inkscape, there is some discussion in bug 1193006 comment 76 and 77. The process is mostly manual though - we haven't figured out how to automate this yet. Eventually we'll be able to obtain the glyph from the UX team with no need to edit ourselves. For this icon I had to align the nodes to the grid, because the rounding made the shape a bit irregular, and this is the final result: <path id="cancel" d="m 6,9.5 6.5,6.5 -6.5,6.5 3.5,3.5 6.5,-6.5 6.5,6.5 3.5,-3.5 -6.5,-6.5 6.5,-6.5 -3.5,-3.5 -6.5,6.5 -6.5,-6.5 z" />

We are working on a new back-end API to power getPermissionsByURI in bug 1206252 in order to improve performance, so we still need to keep the single getPermissionsByURI call in the front-end. While we wait for the other bug to land, I can still review the rest of the patch so we can move the styling part forward.

(In reply to Ricky Chien [:rickychien] from comment #45) > https://reviewboard.mozilla.org/r/55672/#review55846 > > > This change is not needed, right? > > According to comment 19, we should check and display these permissions whoes > default value are not set to 'always ask'. The latest thinking is that we don't need to hide permission types whose default value is not "always ask". The current getPermissionsByURI call gives us a fair approximation of the wanted behavior, displaying only the permissions that have a custom value in the database. If we realize we need to change this later, we can do that in another bug when needed. > > element.style.height = window.getComputedStyle(element).height; > > Is it a bug of multi-line description element? I noticed that some of > description elements don't trigger this kind of issue but some of them do. Yes, only multi-line or styled description elements (those with the text in a text node) have this issue.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon https://reviewboard.mozilla.org/r/55672/#review58152 ::: browser/components/customizableui/content/panelUI.xml:315 (Diff revision 5) > + // Set description's height manually because multi-line description > + // elements cause issues with automatic window sizing. > + let permContent = this._panel.querySelector("#identity-popup-permissions-content"); > + if (permContent) { > + let description = permContent.querySelector("description"); > + description.style.height = window.getComputedStyle(description).height; > + } We shouldn't reference elements from a specific consumer inside the shared panelUI.xml code, so this is surely not the full solution to the issue. It's still unclear to me why the rest of the code in panelUI.xml is being removed. Here you're hard-coding a fix for the permissions panel, so how can this change make the other code unnecessary for the other panels? I've not seen an answer to the question Gijs made in comment 32. Maybe it's not relevant to the new code, but in any case I would suggest you to write a separate comment explaining why these changes are necessary, then separate the panelUI.xml changes to a different changeset (or even a separate bug), and request review from Gijs on it. ::: browser/themes/shared/controlcenter/panel.inc.css:388 (Diff revision 5) > +.identity-popup-permission-state-label { > + margin: 0 6px 0 0; > + color: #999; > + margin-inline-start: 0; > + word-wrap: break-word; > +} I'll let someone more familiar with our themes review these changes later, for the moment my question is whether you have tested with long labels as I asked in comment 23? Is the break-word actually needed? (The property is apparently called overflow-wrap instead of word-wrap now.) Please post a screenshot after editing the state strings temporarily to be something like 30% longer than the English version. Another test with strings that are 100% longer may be useful for reference. ::: browser/themes/shared/controlcenter/panel.inc.css:395 (Diff revision 5) > +#identity-popup-permission-list button { > + margin: 0 2px; You should probably just assign a class to the button like you did for the labels, instead of using a tag selector. I guess that for now you can use a single rule since we only have the cancel button. ::: browser/themes/shared/controlcenter/panel.inc.css:398 (Diff revision 5) > + min-width: 20px; > + min-height: 20px; Did you mean just "width" and "height"? ::: browser/themes/shared/controlcenter/panel.inc.css:400 (Diff revision 5) > +#identity-popup-permission-list button { > + margin: 0 2px; > + -moz-appearance: none; > + min-width: 20px; > + min-height: 20px; > + border-radius: 100%; Is displaying a white X inside a circle the desired hover behavior? I assumed we just wanted to change the color slightly instead of inverting, but I'm not sure. Did you check with Bryan? ::: browser/themes/shared/controlcenter/panel.inc.css:417 (Diff revision 5) > + filter: url(chrome://browser/skin/filters.svg#invert); > + fill: #999; You're not inverting the alpha channel in the filter, so this is equivalent to specifying "fill: #fff;" and using "filters.svg#fill". Please use that syntax, if this is the desired hover behavior.

Brian, we haven't decided the look of cancel button while mouse is hovering. Here is a picture took from my WIP patch. Could you give me your feedback and provide spec for it? thanks!

(In reply to Ricky Chien [:rickychien] from comment #52) > Created attachment 8766642 [details] > cancel-hover-icon.png > > Brian, we haven't decided the look of cancel button while mouse is hovering. > Here is a picture took from my WIP patch. Could you give me your feedback > and provide spec for it? thanks! Not sure - forwarding to Aislinn who might know or be able to redirect appropriately

https://reviewboard.mozilla.org/r/55672/#review58336 ::: browser/components/customizableui/content/panelUI.xml (Diff revision 5) > - height = this._mainView.scrollHeight; > - } > - this._viewContainer.style.height = height + "px"; > - } > - ]]></body> > - </method> This code removing is for auto-shirnk panel's height itself due to permission items will be removed after user click 'x' button. Panel height is hard-coded in here but I didn't understand the reason behind it. Removing this part can make panel shrink automatically as expected.

(In reply to Ricky Chien [:rickychien] from comment #54) > https://reviewboard.mozilla.org/r/55672/#review58336 > > ::: browser/components/customizableui/content/panelUI.xml > (Diff revision 5) > > - height = this._mainView.scrollHeight; > > - } > > - this._viewContainer.style.height = height + "px"; > > - } > > - ]]></body> > > - </method> > > This code removing is for auto-shirnk panel's height itself due to > permission items will be removed after user click 'x' button. > > Panel height is hard-coded in here but I didn't understand the reason behind > it. Removing this part can make panel shrink automatically as expected. If you open the menu panel, then open the history button, the panel expands to show you the history items. Then if you close the history view it shrinks again. The panel itself can't deal well with these changes (in particular because the subviews are scrollable), and so we deal with it manually. Removing all this code without ensuring all this still works (which it likely does not) isn't OK. It sounds like you basically just need to call _syncContainerWithMainView() on the panelmultiview when the permission has been removed.

(In reply to :Gijs Kruitbosch from comment #55) > (In reply to Ricky Chien [:rickychien] from comment #54) > > https://reviewboard.mozilla.org/r/55672/#review58336 > > > > ::: browser/components/customizableui/content/panelUI.xml > > (Diff revision 5) > > > - height = this._mainView.scrollHeight; > > > - } > > > - this._viewContainer.style.height = height + "px"; > > > - } > > > - ]]></body> > > > - </method> > > > > This code removing is for auto-shirnk panel's height itself due to > > permission items will be removed after user click 'x' button. > > > > Panel height is hard-coded in here but I didn't understand the reason behind > > it. Removing this part can make panel shrink automatically as expected. > > If you open the menu panel, then open the history button, the panel expands > to show you the history items. Then if you close the history view it shrinks > again. The panel itself can't deal well with these changes (in particular > because the subviews are scrollable), and so we deal with it manually. > Removing all this code without ensuring all this still works (which it > likely does not) isn't OK. You're totally right! history panel cannot expand and shrink correctly after removing all this code. > It sounds like you basically just need to call _syncContainerWithMainView() > on the panelmultiview when the permission has been removed. No, it doesn't work for me.

This panelUI.xml bug may be the same one that Drew worked around for the new sliding subview for downloads, and it might get fixed generically in bug 1280709 or in a separate bug. Ricky, in the meantime you can address the other review comments and just revert the code changes related to the issue of resizing. We'll just make sure to have that issue fixed before we land this bug.

The cancel state UI is here: http://people.mozilla.org/~shorlander/mockups-interactive/panel-mockups/panel-control-center.html

Paolo, thank you for your information. Gijs, I haven't dig into the issue of resizing too much since as Paolo said there would be an another bug to address the issue soon. As far as I can tell is that issue could be at [1] because MutationObserver works well and _syncContainerWithMainView is able to be triggered when every permission item is removed. However, key point is that this._mainView.scrollHeight doesn't shrink every time after permission is removed. Thus, it's the root cause of this issue. [1] https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.xml?q=browser%2Fcomponents%2Fcustomizableui%2Fcontent%2FpanelUI.xml&redirect_type=direct#361

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/5-6/

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/6-7/

https://reviewboard.mozilla.org/r/55672/#review58336 > This code removing is for auto-shirnk panel's height itself due to permission items will be removed after user click 'x' button. > > Panel height is hard-coded in here but I didn't understand the reason behind it. Removing this part can make panel shrink automatically as expected. It has been reverted!

https://reviewboard.mozilla.org/r/55672/#review58152 > We shouldn't reference elements from a specific consumer inside the shared panelUI.xml code, so this is surely not the full solution to the issue. > > It's still unclear to me why the rest of the code in panelUI.xml is being removed. Here you're hard-coding a fix for the permissions panel, so how can this change make the other code unnecessary for the other panels? > > I've not seen an answer to the question Gijs made in comment 32. Maybe it's not relevant to the new code, but in any case I would suggest you to write a separate comment explaining why these changes are necessary, then separate the panelUI.xml changes to a different changeset (or even a separate bug), and request review from Gijs on it. Good point! I moved hard-coded part to panel.inc.xul and panel.inc.css with a fixed height: 30px. For the reason why the rest of the code in panelUI.xml is being removed that has replied in different comment. > I'll let someone more familiar with our themes review these changes later, for the moment my question is whether you have tested with long labels as I asked in comment 23? > > Is the break-word actually needed? (The property is apparently called overflow-wrap instead of word-wrap now.) > > Please post a screenshot after editing the state strings temporarily to be something like 30% longer than the English version. Another test with strings that are 100% longer may be useful for reference. It has been removed now! > Did you mean just "width" and "height"? There is a min-width: 79px in button.css so that we need to overwrite. > Is displaying a white X inside a circle the desired hover behavior? I assumed we just wanted to change the color slightly instead of inverting, but I'm not sure. Did you check with Bryan? I almost forgot I took mockup from http://people.mozilla.org/~shorlander/mockups-interactive/panel-mockups/panel-control-center.html, so it's right thing to keep this circle styling for cancel button. > You're not inverting the alpha channel in the filter, so this is equivalent to specifying "fill: #fff;" and using "filters.svg#fill". Please use that syntax, if this is the desired hover behavior. It doesn't work for me after adopting "fill: #fff;" and using "filters.svg#fill" in hover. However, I finally find out a way to draw a pretty close color for hover background by filter: opacity(65%) invert(100%);

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/7-8/

Push again due to git rebase incorrectly in last patch.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon https://reviewboard.mozilla.org/r/55672/#review58558 ::: browser/components/customizableui/content/panelUI.xml:379 (Diff revision 8) > + if (this._shouldSetHeight()) { > + this._adjustContainerHeight(); > + } I really don't understand this, and you still aren't explaining why this change is necessary. The code above this insertion already changes the container's height, and if we're syncing the container with the subview we shouldn't adjust the height to that of the main view, which is what this call does... Nit: indenting is also wonky here.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/8-9/

(In reply to :Gijs Kruitbosch from comment #66) > Comment on attachment 8757123 [details] > Bug 1203292 - Replace permissions dropdown with 'x' icon > > https://reviewboard.mozilla.org/r/55672/#review58558 > > ::: browser/components/customizableui/content/panelUI.xml:379 > (Diff revision 8) > > + if (this._shouldSetHeight()) { > > + this._adjustContainerHeight(); > > + } > > I really don't understand this, and you still aren't explaining why this > change is necessary. The code above this insertion already changes the > container's height, and if we're syncing the container with the subview we > shouldn't adjust the height to that of the main view, which is what this > call does... > > Nit: indenting is also wonky here. Oh, it was my mistake from last patch and I did not pay attention to it when reverting rest of my patch. Patch has been updated!

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon https://reviewboard.mozilla.org/r/55672/#review58594 At this stage I think Paolo can review this and he knows the permissions code better, so I'll leave him to it.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/9-10/

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/10-11/

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Thanks! The changes here look generally like the way to go, there may just be some tweaks to the CSS required later. At present there are other bugs blocking this one, that operate on the same code area, so we'll need to wait for them to land, then rebase afterwards. I'll continue reviewing the patch when that happens. Can you post the screenshots with long strings in the meantime?

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/11-12/

Updated patch again for rebasing to newer glyphs.svg

Here is the capture of longer string 1. only permission label with a long string 2. only state label with a long string 3. both of permission and state labels with a long string

Comment on attachment 8768273 [details] Screen Shot 2016-07-06 at 11.17.59 AM.png This alignment when wrapping looks sensible to me, assuming most translations of the "allowed" and "allowed temporarily" strings we'll have in the future won't be too long. Bryan, do you agree?

(In reply to :Paolo Amadini from comment #76) > Comment on attachment 8768273 [details] > Screen Shot 2016-07-06 at 11.17.59 AM.png > > This alignment when wrapping looks sensible to me, assuming most > translations of the "allowed" and "allowed temporarily" strings we'll have > in the future won't be too long. Bryan, do you agree? That's more a question for localization folks (ie. flod). I wouldn't be surprised if some translations of "Allowed temporarily" were twice as long.

(In reply to Florian Quèze [:florian] [:flo] from comment #77) > That's more a question for localization folks (ie. flod). I wouldn't be > surprised if some translations of "Allowed temporarily" were twice as long. In general it's a good approach to design for at least English+50%, but that rule is kind of hard to apply when it comes to very short strings (like "Allow"). I think it would be safe to let the Allow label wrap, e.g. after it reaches 40/50% of the dialog, because I wouldn't be surprised to see languages put sentences in there. Sadly we don't have any similar strings in the repo to double check how long it could become.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/12-13/

I forgot to apply class "identity-popup-permission-label" to stateLabel which should be same as permLabel too. As a result, allow label is able to be wrapped as expected. Patch has been updated!

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Clearing review flag while this bug is still blocked on dependencies.

https://reviewboard.mozilla.org/r/55672/#review62574 Thanks for your patience. Drew is working on the panel shrinking bug this week, and the code this patch touches has stabilized, so we can resume work on this bug. It's getting closer to completion! The main work will be rebasing on top of the latest mozilla-central. As noted below, you can discard "glyphs.svg" since the way we handle it has changed in the meantime. Then you can find my major review comments below. I'll save the smaller issues and styling details for the rebased version. ::: browser/base/content/test/general/browser_permissions.js:51 (Diff revision 13) > gIdentityHandler._identityBox.click(); > - ok(!is_hidden(emptyLabel), "List of permissions is empty"); > + ok(!is_hidden(emptyDescription), "List of permissions is empty"); > gIdentityHandler._identityPopup.hidden = true; > }); > > -add_task(function* testIdentityIcon() { > +add_task(function* testPermissionRemoving() { I'll look at the tests again after they've been updated for the other changes in the patch. In the meantime, I guess the removal of testIdentityIcon is by mistake? ::: browser/themes/shared/controlcenter/panel.inc.css:366 (Diff revision 13) > min-width: 60px; > + min-height: 20px; Seems like min-width is a leftover. ::: browser/themes/shared/controlcenter/panel.inc.css:378 (Diff revision 13) > +#identity-popup-permission-list:empty + description { > + height: 30px; > +} > + Is this rule only needed to solve the panel shrinking bug? ::: browser/themes/shared/controlcenter/panel.inc.css:391 (Diff revision 13) > + > +button.cancel { > + margin: 0 2px; > + -moz-appearance: none; > + min-width: 20px; > + min-height: 20px; > + border-radius: 100%; > + background-repeat: no-repeat; > + background-position: center center; > + background-image: url(chrome://browser/skin/glyphs.svg#cancel); > +} > + > +button.cancel:hover { > + background-image: url(chrome://browser/skin/glyphs.svg#cancel-hover); > + background-color: #999; > +} Styling icon buttons is really complicated! Try the code below that should do what you need. Just set class="identity-popup-permission-remove-button" when creating the button. .identity-popup-permission-remove-button { -moz-appearance: none; margin: 0; border: none; border-radius: 50%; min-width: 0; padding: 2px; } .identity-popup-permission-remove-button > .button-box { border: none; padding: 0; } .identity-popup-permission-remove-button > .button-box > .button-icon { margin: 0; width: 16px; height: 16px; list-style-image: url(chrome://browser/skin/panel-icons.svg#cancel); filter: url(chrome://browser/skin/filters.svg#fill); fill: #999; } .identity-popup-permission-remove-button > .button-box > .button-text { display: none; } .identity-popup-permission-remove-button:hover { background-color: #999; } .identity-popup-permission-remove-button:hover > .button-box > .button-icon { fill: #fff; } .identity-popup-permission-remove-button:hover:active { background-color: #808080; } I'll ask Jared for a review on the styling before the patch lands. ::: browser/themes/shared/glyphs.svg:45 (Diff revision 13) > <path id="microphone-icon" d="m 8,14 0,4 a 8,8 0 0 0 6,7.7 l 0,2.3 -2,0 a 2,2 0 0 0 -2,2 l 12,0 a 2,2 0 0 0 -2,-2 l -2,0 0,-2.3 a 8,8 0 0 0 6,-7.7 l 0,-4 -2,0 0,4 a 6,6 0 0 1 -12,0 l 0,-4 z m 4,4 a 4,4 0 0 0 8,0 l 0,-12 a 4,4 0 0 0 -8,0 z" /> > <path id="microphone-detailed-icon" d="m 8,18 a 8,8 0 0 0 6,7.7 l 0,2.3 -1,0 a 3,2 0 0 0 -3,2 l 12,0 a 3,2 0 0 0 -3,-2 l -1,0 0,-2.3 a 8,8 0 0 0 6,-7.7 l 0,-4 a 1,1 0 0 0 -2,0 l 0,4 a 6,6 0 0 1 -12,0 l 0,-4 a 1,1 0 0 0 -2,0 z m 4,0 a 4,4 0 0 0 8,0 l 0,-12 a 4,4 0 0 0 -8,0 z" /> > <path id="pointerLock-icon" d="m 8,24 6,-5 5,10 4,-2 -5,-10 7,-1 -17,-14 z" /> > <path id="popup-icon" d="m 2,24 a 4,4 0 0 0 4,4 l 8,0 a 10,10 0 0 1 -2,-4 l -4,0 a 2,2 0 0 1 -2,-2 l 0,-12 18,0 0,2 a 10,10 0 0 1 4,2 l 0,-8 a 4,4 0 0 0 -4,-4 l -18,0 a 4,4 0 0 0 -4,4 z m 12,-2.1 a 8,8 0 1 1 0,0.2 m 10.7,-4.3 a 5,5 0 0 0 -6.9,6.9 z m -5.4,8.4 a 5,5 0 0 0 6.9,-6.9 z" /> > <path id="screen-icon" d="m 2,18 a 2,2 0 0 0 2,2 l 2,0 0,-6 a 4,4 0 0 1 4,-4 l 14,0 0,-6 a 2,2 0 0 0 -2,-2 l -18,0 a 2,2 0 0 0 -2,2 z m 6,10 a 2,2 0 0 0 2,2 l 18,0 a 2,2 0 0 0 2,-2 l 0,-14 a 2,2 0 0 0 -2,-2 l -18,0 a 2,2 0 0 0 -2,2 z" /> > + <path id="cancel-icon" d="m 21,12.7-3.32,3.32,3.3,3.3-1.67,1.67-3.3-3.3-3.27,3.27-1.67-1.67,3.27-3.27-3.37-3.37,1.67-1.67,3.37,3.37,3.32-3.32 z"/> For the cancel glyph, we have determined that it does not belong to the same SVG we used for permissions, and the "glyphs.svg" file itself will disappear after the rebase. So, for this icon, add a new file called "panel-icons.svg" with the basic structure of the old "glyphs.svg": https://hg.mozilla.org/mozilla-central/file/bfc23df4f226/browser/themes/shared/glyphs.svg In this patch, include only this path in the "panel-icons.svg" file: <path id="cancel" d="m 6,9.5 6.5,6.5 -6.5,6.5 3.5,3.5 6.5,-6.5 6.5,6.5 3.5,-3.5 -6.5,-6.5 6.5,-6.5 -3.5,-3.5 -6.5,6.5 -6.5,-6.5 z" />

We'd like to have this bug ready at some point next week, so that we can integrate it with other work we're doing. There will probably be one or two more review cycles. Ricky, will you have time to resume work on this bug this week, or are you busy with other work?

Sure, I'm working on rebase stuff in these days

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/13-14/

Rebased on top of the latest m-c and clean up review issues.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/14-15/

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon https://reviewboard.mozilla.org/r/55672/#review62900 Thanks for the rebase, I've tried the patch locally and you can find the detailed review comments below. The tests don't currently pass. Now that we're near to the end of the review cycle, ensure to run the tests locally before submitting for the next review. Thanks! ::: browser/base/content/browser.js:7268 (Diff revision 15) > - SitePermissions.set(gBrowser.currentURI, aPermission, aState); > - }, > - > _createPermissionItem: function (aPermission) { > - let menulist = document.createElement("menulist"); > - let menupopup = document.createElement("menupopup"); > + let container = document.createElement("hbox"); > + let permLabel = document.createElement("label"); The XUL "description" element is recommended instead of "label" when you don't have a "control" attribute. nit: call this nameLabel to distinguish it from stateLabel? ::: browser/base/content/browser.js:7273 (Diff revision 15) > - menupopup.appendChild(menuitem); > - } > - menulist.appendChild(menupopup); > - menulist.setAttribute("value", aPermission.state); > - menulist.setAttribute("oncommand", "gIdentityHandler.setPermission('" + > + let stateLabel = document.createElement("label"); > + stateLabel.setAttribute("class", "identity-popup-permission-label"); > + stateLabel.setAttribute("align", "right"); > + stateLabel.textContent = SitePermissions.getStateLabel( > + aPermission.id, aPermission.state); The "align" attribute does not apply to contained elements, it only applies to XUL containers. What you need here is a separate class and a rule like this: .identity-popup-permission-state-label { text-align: right; opacity: 0.6; } Note also the opacity to match the visual design approximately. You also need to add flex=1 to this label too, so that it wraps properly. ::: browser/base/content/browser.js:7282 (Diff revision 15) > - menulist.setAttribute("oncommand", "gIdentityHandler.setPermission('" + > - aPermission.id + "', this.value)"); > - menulist.setAttribute("id", "identity-popup-permission:" + aPermission.id); > - > - let label = document.createElement("label"); > - label.setAttribute("flex", "1"); > + aPermission.id, aPermission.state); > + > + let button = document.createElement("button"); > + button.setAttribute("align", "right"); > + button.setAttribute("class", "identity-popup-permission-remove-button"); > + button.addEventListener("click", () => { This should listen to the "command" event. ::: browser/base/content/browser.js:7292 (Diff revision 15) > - let container = document.createElement("hbox"); > + container.setAttribute("class", "identity-popup-permission-item"); > container.setAttribute("align", "center"); nit: move these setAttribute calls after the createElement at the top, and leave a blank line after them. ::: browser/base/content/browser.js:7294 (Diff revision 15) > container.appendChild(img); > - container.appendChild(label); > - container.appendChild(menulist); > - > + container.appendChild(permLabel); > + container.appendChild(stateLabel); > + container.appendChild(button); nit: the code will be easier to read if it creates the elements in the same order as they are added to the container. ::: browser/base/content/test/general/browser_permissions.js (Diff revision 15) > - SitePermissions.remove(gBrowser.currentURI, "install"); > - SitePermissions.remove(gBrowser.currentURI, "cookie"); This cleanup... ::: browser/base/content/test/general/browser_permissions.js:25 (Diff revision 15) > - let emptyLabel = permissionsList.nextSibling; > + let emptyDescription = permissionsList.nextSibling; > ...and these renames are a good idea, but they should be moved to a separate changeset. This makes it clear what has changed in the tests in order to support the main change in production code, versus what is a cleanup of existing tests. If you want you can keep the two changesets in the same review request, or move the test cleanup to a separate bug, whatever you prefer. ::: browser/base/content/test/general/browser_permissions.js:31 (Diff revision 15) > - gIdentityHandler.setPermission("install", SitePermissions.ALLOW); > + SitePermissions.set(gBrowser.currentURI, "camera", SitePermissions.ALLOW); > There are more instances of setPermission that need to be rewritten. ::: browser/base/content/test/general/browser_permissions.js:73 (Diff revision 15) > - gIdentityHandler.setPermission("cookie", SitePermissions.SESSION); > + let cancelButtons = permissionsList.querySelectorAll('button.cancel'); > + cancelButtons[0].click(); > + labels = permissionsList.querySelectorAll(".identity-popup-permission-label"); > + is(labels.length, 1, "One permission should be removed"); > + cancelButtons[1].click(); > + labels = permissionsList.querySelectorAll(".identity-popup-permission-label"); > + is(labels.length, 0, "One permission should be removed"); You should add a separate test case to test the new button, and not change an existing unrelated test case. This test here in particular is designed to thest the identity block in the address bar without opening the panel. ::: browser/themes/shared/controlcenter/panel.inc.css:365 (Diff revision 15) > -#identity-popup-permission-list menulist { > - min-width: 60px; > +#identity-popup-permission-list .identity-popup-permission-item { > + min-height: 20px; > } You can avoid the #identity-popup-permission-list ancestor selector because the class is already specific enough. Since the button is 20px high, we probably need a min-height of 24px to get a 4px margin between the buttons. A min-height of 20px has no effect here. ::: browser/themes/shared/controlcenter/panel.inc.css:384 (Diff revision 15) > height: 16px; > } > > .identity-popup-permission-label { > margin-inline-start: 1em; > word-wrap: break-word; We can try removing "word-wrap: break-word" from this rule. I think that wrapping normally on a word boundary will work well with this new layout. If it affects localizations, maybe the right solution is to expand the panel width in those cases.

https://reviewboard.mozilla.org/r/55672/#review62900 > This should listen to the "command" event. I've no idea why oncommand is necessary here? If so, do I need to keep addEventListener anymore? I suspect that your suggestion is like: button.setAttribute("oncommand", "..."); If I'm wrong, please correct me. > This cleanup... Should we keep this as it is anymore? According to the new spec, permission paenl only deal with permission which default value is "always ask". For install and cookie, their default value aren't "always ask" and won't be displayed on permission panel. I think it makes sense to remove them from test case since they are unnecessary to be enabled anymore. > ...and these renames are a good idea, but they should be moved to a separate changeset. This makes it clear what has changed in the tests in order to support the main change in production code, versus what is a cleanup of existing tests. > > If you want you can keep the two changesets in the same review request, or move the test cleanup to a separate bug, whatever you prefer. I'm going to revert this change to keep as it is. However, I'd keep this in two seperate changesets if you prefer to rename it on this bug. > There are more instances of setPermission that need to be rewritten. I don't understand here. Could you give me an exmample? thanks! > We can try removing "word-wrap: break-word" from this rule. I think that wrapping normally on a word boundary will work well with this new layout. If it affects localizations, maybe the right solution is to expand the panel width in those cases. Longer string looks perfectly after removing this line. thank your suggestion.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/15-16/

https://reviewboard.mozilla.org/r/55672/#review62900 > Should we keep this as it is anymore? According to the new spec, permission paenl only deal with permission which default value is "always ask". For install and cookie, their default value aren't "always ask" and won't be displayed on permission panel. I think it makes sense to remove them from test case since they are unnecessary to be enabled anymore. I will keep cookie and remove unused install permission.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/16-17/

https://reviewboard.mozilla.org/r/55672/#review62900 > I don't understand here. Could you give me an exmample? thanks! oh, I got it. I've rewritten all gIdentityHandler to SitePermissions.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon https://reviewboard.mozilla.org/r/55672/#review63144 Thanks, looks good! There were only a few minor changes that I went ahead and fixed already, so I can start a tryserver build before the week ends. We still need to wait for the dependent bug before landing this on mozilla-central, but we can land on the "elm" branch if required. ::: browser/base/content/browser.js:7288 (Diff revision 17) > + stateLabel.setAttribute("class", "identity-popup-permission-state-label"); > + stateLabel.textContent = SitePermissions.getStateLabel( > + aPermission.id, aPermission.state); > + > + let button = document.createElement("button"); > + button.setAttribute("align", "right"); The "align" attribute is also not necessary here. ::: browser/base/content/test/general/browser_permissions.js:63 (Diff revision 17) > - gIdentityHandler.setPermission("geo", SitePermissions.getDefault("geo")); > + SitePermissions.set(gBrowser.currentURI, "geo", SitePermissions.getDefault("geo")); > This should be a SitePermissions.remove(). I've also noticed and fixed pre-existing cases where permissions are not cleared between test cases, because, a bit counter-intuitively I admit, the cleanup function only runs at the end of the whole test file. ::: browser/base/content/test/general/browser_permissions.js:103 (Diff revision 17) > + let labels = permissionsList.querySelectorAll(".identity-popup-permission-label"); > + is(labels.length, 1, "One permission should be removed"); > + cancelButtons[1].click(); > + labels = permissionsList.querySelectorAll(".identity-popup-permission-label"); > + is(labels.length, 0, "One permission should be removed"); > +}); For correctness, this line is required at the end: gIdentityHandler._identityPopup.hidden = true;

https://reviewboard.mozilla.org/r/55672/#review62900 > I've no idea why oncommand is necessary here? If so, do I need to keep addEventListener anymore? > > I suspect that your suggestion is like: > > button.setAttribute("oncommand", "..."); > > If I'm wrong, please correct me. It's simpler than that, XUL buttons generate a "command" event that you could listen to using an addEventListener call. Just replacing "click" with "command" does the trick. This is necessary because not all clicks on buttons result in a command invocation, for example right clicks don't. > I'm going to revert this change to keep as it is. However, I'd keep this in two seperate changesets if you prefer to rename it on this bug. That's fine, the rationale for changing which permission is used makes sense to me, and the modifications are limited enough.

Review commit: https://reviewboard.mozilla.org/r/66448/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66448/

Comment on attachment 8773715 [details] Bug 1203292 - Replace permissions dropdown with a remove button. Hey Jared, does the styling for the new icon button look correct? (Most of it is overriding all platform styling for buttons, apparently we do the same mostly everywhere for icon buttons.)

Comment on attachment 8773715 [details] Bug 1203292 - Replace permissions dropdown with a remove button. https://reviewboard.mozilla.org/r/66448/#review63286 I like the final outcome, looks slick! ::: browser/themes/shared/controlcenter/panel.inc.css:387 (Diff revision 1) > margin-inline-start: 1em; > - word-wrap: break-word; > } > + > +.identity-popup-permission-state-label { > + text-align: right; How does this look with RTL? You should use text-align: end; ::: browser/themes/shared/controlcenter/panel.inc.css:394 (Diff revision 1) > +} > + > +.identity-popup-permission-remove-button { > + -moz-appearance: none; > + margin: 0; > + border: none; Please use border-width: 0; ::: browser/themes/shared/controlcenter/panel.inc.css:401 (Diff revision 1) > + min-width: 0; > + padding: 2px; > +} > + > +.identity-popup-permission-remove-button > .button-box { > + border: none; border-width: 0; ::: browser/themes/shared/controlcenter/panel.inc.css:409 (Diff revision 1) > + list-style-image: url(chrome://browser/skin/panel-icons.svg#cancel); > + filter: url(chrome://browser/skin/filters.svg#fill); > + fill: #999; > +} > + > +.identity-popup-permission-remove-button > .button-box > .button-text { > + display: none; > +} > + > +.identity-popup-permission-remove-button:hover { > + background-color: #999; > +} > + > +.identity-popup-permission-remove-button:hover > .button-box > .button-icon { > + fill: #fff; > +} > + > +.identity-popup-permission-remove-button:hover:active { > + background-color: #808080; Historically we've set the fill inside of the SVG file itself, and then use different IDs to specify which fill the graphic should get. See /browser/themes/shared/urlbar-star.svg for an example. After reading bug 1274480 I'm OK with your new approach.

Comment on attachment 8757123 [details] Bug 1203292 - Replace permissions dropdown with 'x' icon Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55672/diff/17-18/

Updated patch and push to try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c87c6d4c949

Thanks for the patch update. I've integrated this with bug 1280709 and will soon land those bugs together.

Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/fx-team/rev/b655b3fb6153 Replace permissions dropdown with a remove button. r=paolo

Cool! thanks for helping this integration!

On Windows I see a grey circle around "X", without hovering. No circle on Mac and Linux. Is this expected?

Yeah, I saw the problem as you said. Windows applied `background-color: ThreeDFace` at toolkit/themes/windows/global/button.css:13 and its style is distinct with osx. I'm going to file a bug for this.

Verified fixed FX 50.0a2 (2016-08-02) Win 7, Ubuntu 14.04, OS X 10.9.5