1170759 - [Control Center] Implement subpanels

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Tim Taubert [:ttaubert] (inactive)]

We need subpanels to expose more details for some of the (future) sections of the panel.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Just leaving this here: http://cl.ly/image/3o441R1P3A3R

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Looks like I can fix bug 1167340 here too.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1170759-Prevent-the-mainView-of-a-panelmultiview.patch

<panelmultiview>s are great, the only thing I ran into is that when you call .showSubView() with an anchor that's at the very right of the mainView, then the mainView will be shifted so far to the left that empty space is revealed. We shouldn't shift it that far.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(There's a lot more to come, just wanted to get started with that.)

Comment 5

[:Gijs (he/him)]

Comment on attachment 8620924 [details] [diff] [review]
0001-Bug-1170759-Prevent-the-mainView-of-a-panelmultiview.patch

Review of attachment 8620924 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/content/panelUI.xml
@@ +249,5 @@
> +            let neg = direction == "ltr" ? "-" : "";
> +            // If the anchor is an element on the far end of the mainView we
> +            // don't want to shift the mainView too far, we would reveal empty
> +            // space otherwise.
> +            let target = Math.min(edge + center, mainViewRect.width - 38);

I am not comfortable with the hardcoded 38px here. Is there nothing else we can use? What does the 38 actually correspond to?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #5)
> > +            let target = Math.min(edge + center, mainViewRect.width - 38);
> 
> I am not comfortable with the hardcoded 38px here. Is there nothing else we
> can use? What does the 38 actually correspond to?

Tried to use getComputedStyle() to read that value but |-moz-margin-start: 38px| [1] isn't set by the time we shift the main view to the left. |@exitSubviewGutterWidth@| is a CSS variable but I'm not sure how to read that dynamically...

[1] https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#122

Comment 7

[:Gijs (he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > > +            let target = Math.min(edge + center, mainViewRect.width - 38);
> > 
> > I am not comfortable with the hardcoded 38px here. Is there nothing else we
> > can use? What does the 38 actually correspond to?
> 
> Tried to use getComputedStyle() to read that value but |-moz-margin-start:
> 38px| [1] isn't set by the time we shift the main view to the left.
> |@exitSubviewGutterWidth@| is a CSS variable but I'm not sure how to read
> that dynamically...
> 
> [1]
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> customizableui/panelUIOverlay.inc.css#122

It's not a CSS variable, it's a preprocessor substitution filter thing. You could make it a CSS variable, and then you could use getComputedStyle(elementOnWhichVariableIsSet).getPropertyValue("myvariablename") to read it. Or so says the internet, I've not tested it.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1170759-Prevent-the-mainView-of-a-panelmultiview.patch, v2

(In reply to :Gijs Kruitbosch from comment #7)
> You
> could make it a CSS variable, and then you could use
> getComputedStyle(elementOnWhichVariableIsSet).
> getPropertyValue("myvariablename") to read it.

That worked, thanks!

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8620944 [details] [diff] [review]
0001-Bug-1170759-Prevent-the-mainView-of-a-panelmultiview.patch, v2

Sorry.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1170759-Prevent-the-mainView-of-a-panelmultiview.patch, v3

Forgot a parseInt() in the previous patch.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1170759-PanelUI-doesn-t-correctly-remove-transit.patch

Just stumbled upon this while looking at the code - shouldn't cause failures. addEventListener("foo", function () {}.bind(this)) and then trying to remove it is probably a rather common mistake :/

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1170759-Update-permissions-section-when-switchin.patch

After opening the identity panel you can switch tabs using the keyboard without making the panel disappear. We update everything accordingly unless there are permissions for any of the hosts involved. If the first tab has permissions and the newly selected doesn't then we need to remove them or vice-versa, or update them.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8622432 [details] [diff] [review]
0002-Bug-1170759-PanelUI-doesn-t-correctly-remove-transit.patch

Review of attachment 8622432 [details] [diff] [review]:
-----------------------------------------------------------------

r=me because correctness and leaks. But I wonder if this will cause issues because if there is more than 1 property that transitions, the handler will be called repeatedly and could be setting things back to false (esp. in automated test scenarios).

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1170759-Move-detailed-security-information-to-a-.patch

This is the rest of it. Tested on OS X, Linux, Windows, and in RTL mode. It does a few things:

1) Fix bug 1167340 by using "max-height: 0" on the popup. This didn't work with the previous design for whatever reasons but works with the new. It's even more important to fix this now that the panel resizes a lot easier.

2) Move all the certificate information and mixed content warning text to a subpanel, expandable by using the PanelUI component.

3) Replace "Larry" with three new, shiny SVG icons.

4) Fix the the display of internal (.chromeUI) pages to show the URL (instead of the brand name) and bring back the label "This is a secure Firefox page."

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8622972 [details] [diff] [review]
0004-Bug-1170759-Move-detailed-security-information-to-a-.patch

Review of attachment 8622972 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/osx/jar.mn
@@ -190,5 @@
>    skin/classic/browser/yosemite/loop/[e-mail]       (loop/[e-mail])
>    skin/classic/browser/yosemite/loop/toolbar.png            (loop/toolbar-yosemite.png)
>    skin/classic/browser/yosemite/loop/[e-mail]         (loop/[e-mail])
>  * skin/classic/browser/controlcenter/panel.css        (controlcenter/panel.css)
> -  skin/classic/browser/customizableui/background-noise-toolbar.png  (customizableui/background-noise-toolbar.png)

Oops, didn't mean to remove that line. Fixed locally.

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8622972 [details] [diff] [review]
0004-Bug-1170759-Move-detailed-security-information-to-a-.patch

Review of attachment 8622972 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/controlcenter/arrow-subview.svg
@@ +1,3 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +

Please clean up the SVGs, see bug 1160771 for reference (where all SVG had been cleaned up).
Also, they'll need a license header.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Nguyen [:ntim] from comment #16)
> Please clean up the SVGs, see bug 1160771 for reference (where all SVG had
> been cleaned up).
> Also, they'll need a license header.

Will do that, thanks for the hint!

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1170759-Move-detailed-security-information-to-a-.patch, v2

SVGs cleaned up a little, added license headers.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8623227 [details] [diff] [review]
0004-Bug-1170759-Move-detailed-security-information-to-a-.patch, v2

Review of attachment 8623227 [details] [diff] [review]:
-----------------------------------------------------------------

(I'm sorry for the delay here, I am halfway reviewing this but I was distracted several times today so I need to look at this some more. I should be able to get it done tomorrow. Here's what I have so far (not all of it necessarily well-researched)

::: browser/components/controlcenter/content/panel.inc.xul
@@ +11,5 @@
>         orient="vertical"
>         level="top">
> +
> +  <broadcasterset>
> +    <broadcaster id="identity-popup-content-host" value=""/>

Maybe just stick this in the main broadcasterset ?

::: browser/themes/shared/controlcenter/panel.inc.css
@@ +8,5 @@
> +#identity-popup-security-content:not(.unknownIdentity) > .identity-popup-connection-not-secure,
> +#identity-popup-securityView:not(.verifiedIdentity) > #identity-popup-securityView-connection,
> +#identity-popup-security-content.unknownIdentity:not(.mixedContent) + .identity-popup-expander,
> +#identity-popup-security-content:not(.chromeUI) > .identity-popup-connection-internal,
> +#identity-popup-security-content.chromeUI + .identity-popup-expander {

I think this would be more readable if split up by state and commented:

/* Hide EV items if the page doesn't use EV: */
#identity-popup-securityView:not(.verifiedIdentity) > #identity-popup-content-owner,
#identity-popup-securityView:not(.verifiedIdentity) > #identity-popup-securityView-connection,

/* Hide all verification items if the page is neither EV nor DV */
#identity-popup-securityView:not(.verifiedIdentity):not(.verifiedDomain) > #identity-popup-content-verifier,
#identity-popup-securityView:not(.verifiedIdentity):not(.verifiedDomain) > #identity-popup-securityView-header > .identity-popup-connection-secure,
#identity-popup-security-content:not(.verifiedIdentity):not(.verifiedDomain) > .identity-popup-connection-secure,

/* Hide "this is insecure" indicators if the page does not have an unknown identity: */
#identity-popup-security-content:not(.unknownIdentity) > .identity-popup-connection-not-secure,
#identity-popup-securityView:not(.unknownIdentity) > #identity-popup-securityView-header > .identity-popup-connection-not-secure,

/* Hide Chrome UI indicators if this is not an about/chrome page: */
#identity-popup-securityView:not(.chromeUI) > #identity-popup-securityView-header > .identity-popup-connection-internal,
#identity-popup-security-content:not(.chromeUI) > .identity-popup-connection-internal,

/* Hide the supplemental part of the popup if there is neither EV nor mixed content data to display */
#identity-popup-securityView:not(.verifiedIdentity):not(.mixedContent) > #identity-popup-content-supplemental,

/* Hide the expander if there is no security information to display: */
#identity-popup-security-content.unknownIdentity:not(.mixedContent) + .identity-popup-expander,
#identity-popup-security-content.chromeUI + .identity-popup-expander {
  display: none;
}

@@ -13,5 @@
>  }
>  
> -#identity-popup-container {
> -  min-width: 25em;
> -  padding: 10px;

This padding seems to have vanished. Intentional change?

Comment 20

[:Gijs (he/him)]

Comment on attachment 8623227 [details] [diff] [review]
0004-Bug-1170759-Move-detailed-security-information-to-a-.patch, v2

Review of attachment 8623227 [details] [diff] [review]:
-----------------------------------------------------------------

This is essentially r+ but I'd like to see the interdiff for the final version, see below for issues I found / wanted to have noted/discussed:


If you go to an insecure website with permissions (mine had popups allowed), there is a thin white line underneath the "More Information" footer on OS X.

::: browser/components/controlcenter/content/panel.inc.xul
@@ +63,5 @@
> +               value="&identity.connectionSecure;"/>
> +        <label class="identity-popup-connection-not-secure identity-popup-text"
> +               value="&identity.connectionNotSecure;"/>
> +        <label class="identity-popup-connection-internal identity-popup-text"
> +               value="&identity.connectionInternal;"/>

For the labels we use here, do you ever want any of these to wrap, also considering l10n, in which case, d'you want to stick the contents in the element as textcontent instead of the value attribute? ("no" is a fair answer)

::: browser/themes/shared/controlcenter/panel.inc.css
@@ +74,5 @@
>  
> +.identity-popup-expander {
> +  margin: 0;
> +  padding: 4px 0;
> +  min-width: auto;

Gah, stupid internal flex thing. Maybe add a comment for why we need this?

@@ +76,5 @@
> +  margin: 0;
> +  padding: 4px 0;
> +  min-width: auto;
> +  width: 38px;
> +  font-weight: 700;

Why this, as there's no text in here, AFAICT?

@@ +81,5 @@
> +  border: 0 none;
> +  -moz-appearance: none;
> +  background-image: url("chrome://browser/skin/controlcenter/arrow-subview.svg"),
> +                    linear-gradient(rgba(255,255,255,0.3), transparent);
> +  background-size: 16px auto, auto auto;

Pretty sure this is identical to:

background-size: 16px, auto;

@@ +82,5 @@
> +  -moz-appearance: none;
> +  background-image: url("chrome://browser/skin/controlcenter/arrow-subview.svg"),
> +                    linear-gradient(rgba(255,255,255,0.3), transparent);
> +  background-size: 16px auto, auto auto;
> +  background-position: 50% 50%;

background-position: center;

@@ +110,5 @@
> +.identity-popup-expander > .button-box:focus {
> +  padding: 0;
> +  -moz-appearance: none;
> +  border: solid #e5e5e5;
> +  border-width: 0 0 0 1px;

Mm. So for the buttons in the footer of the main menupanel, the border expands on hover to connect with the top + bottom. I guess that wasn't in the design here?

Also, this border ends up looking thinner than the one for the "more information" button because the latter is much darker. By design?

@@ +123,5 @@
> +  background-color: hsla(210,4%,10%,.07);
> +}
> +
> +.identity-popup-expander:hover:active {
> +  color: inherit;

There doesn't seem to be any text in this thing, why is this necessary?

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #20)
> If you go to an insecure website with permissions (mine had popups allowed),
> there is a thin white line underneath the "More Information" footer on OS X.

Yeah... so. That seems to be an illusion. I see it too but couldn't find the cause. Here's a screenshot:

http://i.imgur.com/OteBXHW.png

I can see it there, when I zoom in the thin line stays the exact size. And when I pick the color of those pixels in Gimp they're the exact same color as the rest above it. Not sure what to do about that?

> ::: browser/components/controlcenter/content/panel.inc.xul
> @@ +63,5 @@
> > +               value="&identity.connectionSecure;"/>
> > +        <label class="identity-popup-connection-not-secure identity-popup-text"
> > +               value="&identity.connectionNotSecure;"/>
> > +        <label class="identity-popup-connection-internal identity-popup-text"
> > +               value="&identity.connectionInternal;"/>
> 
> For the labels we use here, do you ever want any of these to wrap, also
> considering l10n, in which case, d'you want to stick the contents in the
> element as textcontent instead of the value attribute? ("no" is a fair
> answer)

I actually think that "no" is indeed the answer, those shouldn't wrap and there's enough place that I don't expect a locale to be problematic. I do think we want a clear single line telling the user what connection security to expect.

> ::: browser/themes/shared/controlcenter/panel.inc.css
> @@ +74,5 @@
> >  
> > +.identity-popup-expander {
> > +  margin: 0;
> > +  padding: 4px 0;
> > +  min-width: auto;
> 
> Gah, stupid internal flex thing. Maybe add a comment for why we need this?

We actually only need this to override a min-width set for buttons by the basic OS theme.

> @@ +76,5 @@
> > +  margin: 0;
> > +  padding: 4px 0;
> > +  min-width: auto;
> > +  width: 38px;
> > +  font-weight: 700;
> 
> Why this, as there's no text in here, AFAICT?

Thanks for catching. Used to have a ">" placeholder while waiting for icons :)

> @@ +81,5 @@
> > +  border: 0 none;
> > +  -moz-appearance: none;
> > +  background-image: url("chrome://browser/skin/controlcenter/arrow-subview.svg"),
> > +                    linear-gradient(rgba(255,255,255,0.3), transparent);
> > +  background-size: 16px auto, auto auto;
> 
> Pretty sure this is identical to:
> 
> background-size: 16px, auto;

Yes, thanks!

> @@ +82,5 @@
> > +  -moz-appearance: none;
> > +  background-image: url("chrome://browser/skin/controlcenter/arrow-subview.svg"),
> > +                    linear-gradient(rgba(255,255,255,0.3), transparent);
> > +  background-size: 16px auto, auto auto;
> > +  background-position: 50% 50%;
> 
> background-position: center;

Fixed.

> @@ +110,5 @@
> > +.identity-popup-expander > .button-box:focus {
> > +  padding: 0;
> > +  -moz-appearance: none;
> > +  border: solid #e5e5e5;
> > +  border-width: 0 0 0 1px;
> 
> Mm. So for the buttons in the footer of the main menupanel, the border
> expands on hover to connect with the top + bottom. I guess that wasn't in
> the design here?

No, there are no hover states specified. Changing only the .button-box's background color looks weird, esp. given that when the mainview slides to the left the whole button becomes blue (that was in the design at least). Do you think this looks weird? I actually like it and Aislinn didn't flag anything after she looked at the build and played around with it.

> Also, this border ends up looking thinner than the one for the "more
> information" button because the latter is much darker. By design?

I *think* it was by design. When I darken the border color they are in sync and the left border stays visible when hovering the button, the arrow button looks unnecessarily separated though. It's not a separator like between the "More Information" button and everything else but rather a subdivision of a section itself.

> @@ +123,5 @@
> > +  background-color: hsla(210,4%,10%,.07);
> > +}
> > +
> > +.identity-popup-expander:hover:active {
> > +  color: inherit;
> 
> There doesn't seem to be any text in this thing, why is this necessary?

Copy&paste probably, removed.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0005-Part-4-interdiff.patch

Comment 23

[:Gijs (he/him)]

Attachment: white line at the bottom

Actually, no, that's not what I meant. I mean this, and the eyedropper confirms it's not just an illusion - there's a 1px line of f9f9f9 below the button (which comes out as e9e9e9 here).

Comment 24

[:Gijs (he/him)]

Actually, thinking about it, I wonder if it's a bug with the resizing of the panelview or something. ISTR we've had issues with this before... but I can't recall the details off the top of my head.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0005-Part-4-interdiff.patch, v2

Aha! The button container was missing a flex=1.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8625606 [details] [diff] [review]
0005-Part-4-interdiff.patch, v2

Review of attachment 8625606 [details] [diff] [review]:
-----------------------------------------------------------------

Wonderful, thanks much.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2555d8ffbd6b
https://hg.mozilla.org/integration/fx-team/rev/dbb266905780
https://hg.mozilla.org/integration/fx-team/rev/a810067a6cf1
https://hg.mozilla.org/integration/fx-team/rev/fde6d2f7d6bc

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2555d8ffbd6b
https://hg.mozilla.org/mozilla-central/rev/dbb266905780
https://hg.mozilla.org/mozilla-central/rev/a810067a6cf1
https://hg.mozilla.org/mozilla-central/rev/fde6d2f7d6bc