1203292 - Replace permissions dropdown with 'x' icon in Permissions main view in Control Center

Status: VERIFIED FIXED

(Firefox :: General, defect, P2)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[[e-mail]]

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

Comment 2

[Ash Grigas]

(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

Comment 3

[[e-mail]]

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

Comment 4

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 5

[Ricky Chien [:rickychien] (inactive)]

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?

Comment 6

[Ricky Chien [:rickychien] (inactive)]

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

Comment 7

[Ash Grigas]

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

Comment 8

[Ricky Chien [:rickychien] (inactive)]

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

Comment 9

[Ricky Chien [:rickychien] (inactive)]

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

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Attachment: reload-icon+close-icon.png

Comment 11

[Ash Grigas]

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

Comment 12

[[e-mail]]

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

Comment 13

[Ricky Chien [:rickychien] (inactive)]

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

Comment 14

[Ash Grigas]

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

[Ricky Chien [:rickychien] (inactive)]

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?

Comment 16

[Ash Grigas]

(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

Comment 17

[Ash Grigas]

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

Comment 18

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 19

[Ash Grigas]

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

Comment 20

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1203292 - Replace permissions dropdown with 'x' icon

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

Comment 21

[Ricky Chien [:rickychien] (inactive)]

Thanks for clarifying that! patch has submitted.

Comment 22

[Ricky Chien [:rickychien] (inactive)]

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 23

[:Paolo Amadini]

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.

Comment 24

[Ash Grigas]

(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

Comment 25

[:Paolo Amadini]

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

Comment 26

[Ash Grigas]

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

Comment 27

[:Paolo Amadini]

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

Comment 28

[Ricky Chien [:rickychien] (inactive)]

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

Comment 29

[:Paolo Amadini]

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

Comment 30

[Ricky Chien [:rickychien] (inactive)]

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

Comment 31

[:Paolo Amadini]

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

Comment 32

[:Gijs (out for now; he/him)]

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

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 34

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 35

[Ricky Chien [:rickychien] (inactive)]

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 36

[:Paolo Amadini]

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 37

[:Paolo Amadini]

Comment on attachment 8757123 [details]
Bug 1203292 - Replace permissions dropdown with 'x' icon

For the panelUI.xml changes.

Comment 38

[:Gijs (out for now; he/him)]

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?

Comment 39

[:Paolo Amadini]

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

Comment 40

[Ricky Chien [:rickychien] (inactive)]

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

Comment 41

[Ricky Chien [:rickychien] (inactive)]

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!

Comment 42

[[e-mail]]

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

Comment 43

[Ricky Chien [:rickychien] (inactive)]

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 44

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 45

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 46

[Ricky Chien [:rickychien] (inactive)]

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

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 48

[:Paolo Amadini]

(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" />

Comment 49

[:Paolo Amadini]

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.

Comment 50

[:Paolo Amadini]

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

[:Paolo Amadini]

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.

Comment 52

[Ricky Chien [:rickychien] (inactive)]

Attachment: 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!

Comment 53

[Brian Grinstead [:bgrins]]

(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

Comment 54

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 55

[:Gijs (out for now; he/him)]

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

Comment 56

[Ricky Chien [:rickychien] (inactive)]

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

Comment 57

[:Paolo Amadini]

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.

Comment 58

[Ash Grigas]

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

Comment 59

[Ricky Chien [:rickychien] (inactive)]

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 60

[Ricky Chien [:rickychien] (inactive)]

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 61

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 62

[Ricky Chien [:rickychien] (inactive)]

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!

Comment 63

[Ricky Chien [:rickychien] (inactive)]

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 64

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 65

[Ricky Chien [:rickychien] (inactive)]

Push again due to git rebase incorrectly in last patch.

Comment 66

[:Gijs (out for now; he/him)]

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 67

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 68

[Ricky Chien [:rickychien] (inactive)]

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

[:Gijs (out for now; he/him)]

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 70

[Ricky Chien [:rickychien] (inactive)]

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 71

[Ricky Chien [:rickychien] (inactive)]

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 72

[:Paolo Amadini]

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 73

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 74

[Ricky Chien [:rickychien] (inactive)]

Updated patch again for rebasing to newer glyphs.svg

Comment 75

[Ricky Chien [:rickychien] (inactive)]

Attachment: Screen Shot 2016-07-06 at 11.17.59 AM.png

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 76

[:Paolo Amadini]

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?

Comment 77

[Florian Quèze [:florian]]

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

Comment 78

[Francesco Lodolo [:flod]]

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

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 80

[Ricky Chien [:rickychien] (inactive)]

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 81

[:Paolo Amadini]

Comment on attachment 8757123 [details]
Bug 1203292 - Replace permissions dropdown with 'x' icon

Clearing review flag while this bug is still blocked on dependencies.

Comment 82

[:Paolo Amadini]

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

Comment 83

[:Paolo Amadini]

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?

Comment 84

[Ricky Chien [:rickychien] (inactive)]

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

Comment 85

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 86

[Ricky Chien [:rickychien] (inactive)]

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

Comment 87

[Ricky Chien [:rickychien] (inactive)]

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 88

[:Paolo Amadini]

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.

Comment 89

[Ricky Chien [:rickychien] (inactive)]

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 90

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 91

[Ricky Chien [:rickychien] (inactive)]

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 92

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 93

[Ricky Chien [:rickychien] (inactive)]

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 94

[:Paolo Amadini]

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;

Comment 95

[:Paolo Amadini]

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.

Comment 96

[:Paolo Amadini]

Attachment: Bug 1203292 - Replace permissions dropdown with a remove button.

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

Comment 97

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e645b3110e8

Comment 98

[:Paolo Amadini]

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 99

[Jared Wein [:jaws] (please needinfo? me)]

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 100

[Ricky Chien [:rickychien] (inactive)]

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/

Comment 101

[Ricky Chien [:rickychien] (inactive)]

Updated patch and push to try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c87c6d4c949

Comment 102

[:Paolo Amadini]

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

Comment 103

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/b655b3fb6153
Replace permissions dropdown with a remove button. r=paolo

Comment 104

[Ricky Chien [:rickychien] (inactive)]

Cool! thanks for helping this integration!

Comment 105

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b655b3fb6153

Comment 106

[Paul Silaghi, QA [:pauly]]

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

Comment 107

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 108

[Paul Silaghi, QA [:pauly]]

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