Update site identity popup to Photon style

VERIFIED FIXED in Firefox 58

Status

()

P1
normal
VERIFIED FIXED
a year ago
9 months ago

People

(Reporter: johannh, Assigned: Paolo)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 unaffected, firefox57 wontfix, firefox58 verified, firefox59 verified)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(1 attachment)

(Reporter)

Description

a year 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.
Flags: qe-verify+
(Reporter)

Comment 1

a year 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.

Updated

a year ago
Whiteboard: [photon-structure][triage]
Priority: -- → P3
Whiteboard: [photon-structure][triage]
Whiteboard: [photon-structure][triage]
(Assignee)

Updated

a year ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year 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.
(Assignee)

Comment 4

a year 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
(Assignee)

Comment 6

a year 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.
(Reporter)

Comment 7

a year 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
Attachment #8926500 - Flags: review?(jhofmann) → review+
(Reporter)

Comment 8

a year 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
(Assignee)

Comment 9

a year 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.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1416192
(Reporter)

Comment 11

a year ago
Good points, thank you!
(Assignee)

Comment 12

a year ago
I fixed one of the photonpanelmultiview bugs properly in bug 1392340, as the quick solution here caused an additional reflow.
Depends on: 1392340
Comment hidden (mozreview-request)
(Assignee)

Comment 14

a year ago
New tryserver build after fixing the photonpanelmultiview bugs:

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

Comment 15

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68d6d74d5c553cb459420e591f78dfda62db1c09
Comment hidden (mozreview-request)
(Assignee)

Comment 17

a year 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.

Comment 18

a year 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
https://hg.mozilla.org/mozilla-central/rev/ded163bd1d46
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Depends on: 1416628
(Reporter)

Updated

a year ago
Blocks: 1327621
(Assignee)

Comment 21

a year ago
Thanks, I'm quite happy that the main view has zero pixels of difference :-)
(Reporter)

Updated

a year ago
Blocks: 1372045
(Assignee)

Updated

a year ago
No longer depends on: 1392340
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?
(Assignee)

Comment 23

a year ago
I can't think of anything else to test for this bug. Thanks Oana!
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
status-firefox59: --- → verified
Flags: qe-verify+
Depends on: 1422125
Depends on: 1468319
You need to log in before you can comment on or make changes to this bug.