Closed Bug 1170759 Opened 4 years ago Closed 4 years ago

[Control Center] Implement subpanels

Categories

(Firefox :: General, defect, P1)

defect
Points:
8

Tracking

()

RESOLVED FIXED
Firefox 41
Iteration:
41.3 - Jun 29
Tracking Status
firefox41 --- fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

References

()

Details

(Whiteboard: [fxprivacy])

Attachments

(6 files, 4 obsolete files)

7.22 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
3.29 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.07 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
97.98 KB, patch
Gijs
: feedback+
Details | Diff | Splinter Review
282.65 KB, image/png
Details
1.59 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
We need subpanels to expose more details for some of the (future) sections of the panel.
Blocks: 1170761
Points: --- → 8
Flags: qe-verify-
Flags: firefox-backlog+
Whiteboard: [fxprivacy]
Just leaving this here: http://cl.ly/image/3o441R1P3A3R
Rank: 4
Priority: -- → P1
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 41.3 - Jun 29
Looks like I can fix bug 1167340 here too.
Blocks: 1167340
<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.
Attachment #8620924 - Flags: review?(gijskruitbosch+bugs)
(There's a lot more to come, just wanted to get started with that.)
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?
Attachment #8620924 - Flags: review?(gijskruitbosch+bugs)
(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
(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.
(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!
Attachment #8620924 - Attachment is obsolete: true
Attachment #8620944 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8620944 [details] [diff] [review]
0001-Bug-1170759-Prevent-the-mainView-of-a-panelmultiview.patch, v2

Sorry.
Attachment #8620944 - Flags: review?(gijskruitbosch+bugs)
Forgot a parseInt() in the previous patch.
Attachment #8620944 - Attachment is obsolete: true
Attachment #8620945 - Flags: review?(gijskruitbosch+bugs)
Attachment #8620945 - Flags: review?(gijskruitbosch+bugs) → review+
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 :/
Attachment #8622432 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8622434 - Flags: review?(gijskruitbosch+bugs)
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).
Attachment #8622432 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8622434 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1168457
Blocks: 1170762
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."
Attachment #8622972 - Flags: review?(gijskruitbosch+bugs)
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/menuPanel@2x.png       (loop/menuPanel-yosemite@2x.png)
>    skin/classic/browser/yosemite/loop/toolbar.png            (loop/toolbar-yosemite.png)
>    skin/classic/browser/yosemite/loop/toolbar@2x.png         (loop/toolbar-yosemite@2x.png)
>  * 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 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.
(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!
SVGs cleaned up a little, added license headers.
Attachment #8622972 - Attachment is obsolete: true
Attachment #8622972 - Flags: review?(gijskruitbosch+bugs)
Attachment #8623227 - Flags: review?(gijskruitbosch+bugs)
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 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?
Attachment #8623227 - Flags: review?(gijskruitbosch+bugs) → feedback+
(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.
Attached patch 0005-Part-4-interdiff.patch (obsolete) — Splinter Review
Attachment #8625587 - Flags: review?(gijskruitbosch+bugs)
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).
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.
Aha! The button container was missing a flex=1.
Attachment #8625587 - Attachment is obsolete: true
Attachment #8625587 - Flags: review?(gijskruitbosch+bugs)
Attachment #8625606 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8625606 [details] [diff] [review]
0005-Part-4-interdiff.patch, v2

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

Wonderful, thanks much.
Attachment #8625606 - Flags: review?(gijskruitbosch+bugs) → review+
Depends on: 1177161
Depends on: 1177238
Rank: 4
Depends on: 1177437
Depends on: 1177438
Depends on: 1177524
Depends on: 1177779
Depends on: 1178160
Depends on: 1178163
Depends on: 1181978
Depends on: 1187705
Depends on: 1195753
Depends on: 1196053
Depends on: 1201437
Depends on: 1202641
Depends on: 1204226
Depends on: 1203264
Depends on: 1204825
Depends on: 1220753
Depends on: 1259793
Depends on: 1304973
You need to log in before you can comment on or make changes to this bug.