Johann Hofmann [:johannh] Reporter
	Description • 7 years ago

+++ This bug was initially created as a clone of Bug #1374339 +++

There is a UX spec for updating the identity popup (https://mozilla.invisionapp.com/share/URCPD936K#/screens) and relevant discussion in bug 1374339.

	Johann Hofmann [:johannh] Reporter
	Comment 1 • 7 years ago

Another thing I'd like to note is that the identity popup can have more UI states than shown in the invision mock. Some of those affect the sizing of the header row, you can try them out on

http://http-password.badssl.com/
https://mixed-script.badssl.com/
https://very.badssl.com/

This might impact the design with the large hover background, or it might be fine, I just wanted to point it out.

	:Paolo Amadini Assignee
	Comment 3 • 7 years ago

Johann, I've scoped this patch to the subview only, since it is already complex enough, and this is the only part that is strictly needed to convert the binding.

I left out the main view redesign with the large hover background, as I understand from comment 1 that we may want some reassurance that it is really what we should implement. If you're fine with this incremental approach, I'd prefer to leave the main view redesign for another bug, and keep the view identical to the current state.

	:Paolo Amadini Assignee
	Comment 4 • 7 years ago

There might be more tests to update, maybe even some that are not browser-chrome. Let's see a tryserver build:

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

	:Paolo Amadini Assignee
	Comment 5 • 7 years ago

Reference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=10f1fb1fffd7bd55447c1f5af292588913b2d733
Screenshots: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe28b867e42b3987bb43f8af6eabed96f21dc448

	:Paolo Amadini Assignee
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8926500 [details]
Bug 1409301 - Update the site security subview to the Photon style.

https://reviewboard.mozilla.org/r/197754/#review202966

::: browser/base/content/test/siteIdentity/head.js:222
(Diff revision 1)
> -  let securityViewBG = tabbrowser.ownerGlobal.getComputedStyle(securityView).
> -                       getPropertyValue("background-image");
> -  let securityContentBG = tabbrowser.ownerGlobal.getComputedStyle(securityView).
> -                          getPropertyValue("background-image");
> +    .getComputedStyle(document.getElementById("identity-popup-securityView")
> +                              .getElementsByClassName("identity-popup-security-content")[0])
> +    .getPropertyValue("background-image");
> +  let securityContentBG = tabbrowser.ownerGlobal

By the way, this test was originally checking the same element, and is now fixed.

Also note that the obvious solution of calling getElementsByClassName on the document doesn't work, and I guess that's because each subview is a separate XBL binding, so we need the indirection.

	Johann Hofmann [:johannh] Reporter
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8926500 [details]
Bug 1409301 - Update the site security subview to the Photon style.

https://reviewboard.mozilla.org/r/197754/#review203678

I'm good with the incremental approach. Thanks for this patch, looks good to me on the whole.

::: browser/base/content/browser.js:7670
(Diff revision 1)
>      this._identityPopupMixedContentLearnMore
>          .setAttribute("href", baseURL + "mixed-content");
>      this._identityPopupInsecureLoginFormsLearnMore
>          .setAttribute("href", baseURL + "insecure-password");
>  
> -    // The expander switches its tooltip when toggled, change it to the default.
> +    // This is in the properties file because the expander used to switch its tooltip.

I wonder, is it worth keeping it there? That is, is moving the tooltip a big strain on translators? I'm not blocking on this, just wanted to say that I would have probably moved it.

::: browser/themes/shared/controlcenter/panel.inc.css:68
(Diff revision 1)
>    fill-opacity: .6;
>  }
>  
> -.panel-mainview[panelid=identity-popup] {
> +#identity-popup-mainView {
>    min-width: 30em;
> +  max-width: 30em;

There's probably a good reason, but why not just use width: 30em?

::: browser/themes/shared/controlcenter/panel.inc.css:123
(Diff revision 1)
>    -moz-context-properties: fill;
>    fill: currentColor;
>    color: inherit;
>  }
>  
>  .identity-popup-expander[panel-multiview-anchor] {

If I'm not mistaken, these rules also need to be cleaned up now.

https://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/browser/themes/shared/controlcenter/panel.inc.css#141-151,157-159

	Johann Hofmann [:johannh] Reporter
	Comment 8 • 7 years ago

(In reply to :Paolo Amadini from comment #6)
> By the way, this test was originally checking the same element, and is now
> fixed.

Hah, great find. :D

	:Paolo Amadini Assignee
	Comment 9 • 7 years ago

(In reply to Johann Hofmann [:johannh] from comment #7)
> > -    // The expander switches its tooltip when toggled, change it to the default.
> > +    // This is in the properties file because the expander used to switch its tooltip.
> 
> I wonder, is it worth keeping it there? That is, is moving the tooltip a big
> strain on translators? I'm not blocking on this, just wanted to say that I
> would have probably moved it.

I've not moved it considering that this string might just be reworded or go away entirely when the main view is updated.

> ::: browser/themes/shared/controlcenter/panel.inc.css:68
> > -.panel-mainview[panelid=identity-popup] {
> > +#identity-popup-mainView {
> >    min-width: 30em;
> > +  max-width: 30em;
> 
> There's probably a good reason, but why not just use width: 30em?

The "photonpanelmultiview" implementation currently controls and removes the width attribute, even on the main view.

> ::: browser/themes/shared/controlcenter/panel.inc.css:123
> If I'm not mistaken, these rules also need to be cleaned up now.

Done, thanks! Note that I've left the current scaleX approach instead of changing the background image, so the main view screenshots will still match exactly.

	Johann Hofmann [:johannh] Reporter
	Comment 11 • 7 years ago

Good points, thank you!

	:Paolo Amadini Assignee
	Comment 12 • 7 years ago

I fixed one of the photonpanelmultiview bugs properly in bug 1392340, as the quick solution here caused an additional reflow.

	:Paolo Amadini Assignee
	Comment 14 • 7 years ago

New tryserver build after fixing the photonpanelmultiview bugs:

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

	:Paolo Amadini Assignee
	Comment 15 • 7 years ago

After the photonpanelmultiview fixes, some more tests had to be changed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68d6d74d5c553cb459420e591f78dfda62db1c09

	:Paolo Amadini Assignee
	Comment 17 • 7 years ago

Okay, now more photonpanelmultiview issues are showing up as a result of the asynchronous approach that avoids the reflows.

This is a chicken-and-egg situation because it will be much easier to reason about the code and solve the current issues once the two bindings are unified in bug 1414244, but that work is blocked by this bug.

I'll just go with the reflows for now, and I've filed bug 1416498 to figure out the race conditions. Let's see how well this works:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14e53e78223377d23f12ae90c40b25b267221d10

If the tryserver build is green, I'll land this bug at the beginning of the next cycle, so we have time to fix the reflow issues after landing bug 1414244.

	Pulsebot
	Comment 18 • 7 years ago

Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ded163bd1d46
Update the site security subview to the Photon style. r=johannh

	Stefan Hindli [:stefan_hindli]
	Comment 19 • 7 years ago
bugherder

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

	Johann Hofmann [:johannh] Reporter
	Comment 20 • 7 years ago

Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=d63b1d316177d7a5d238c390f3c7cb718310f18d&newProject=mozilla-central&newRev=1f91961bb79ad06fd4caef9e5dfd546afd5bf42c&filter=controlCenter

Looking very good to me!

	:Paolo Amadini Assignee
	Comment 21 • 7 years ago

Thanks, I'm quite happy that the main view has zero pixels of difference :-)

	Oana Botisan, Desktop QA
	Comment 22 • 7 years ago

I managed to reproduce the bug using an older version of Nightly (2017-10-17) on macOS 10.13.

I retested everything using latest Nightly and beta 58.0b5 on macOS 10.13, Ubuntu 16.04 x64, Windows 10 x64 and Windows 7 x64 and the bug is not reproducing anymore. The identity subview is displayed correctly just like UX specs from https://mozilla.invisionapp.com/share/URCPD936K#/screens.

Should I be searching for something else? Or is this verification enough to close the bug?

	:Paolo Amadini Assignee
	Comment 23 • 7 years ago

I can't think of anything else to test for this bug. Thanks Oana!