1476220 - Show a "site information" header in the identity popup

Status: RESOLVED FIXED

(Firefox :: Site Identity, enhancement, P2)

Description

[Johann Hofmann [:johannh]]

See design spec: https://mozilla.invisionapp.com/share/VSMVD99JK6D#/screens/301705296

We'd like to have consistent headers for the sections of the identity popup, this means that the connection/security section at the top which currently has the domain as headline will get called "Connection" instead.

For showing the domain name we introduce a Photon-style header at the top instead.

Comment 1

[Johann Hofmann [:johannh]]

This needs to move to 64, this part of the UI needs some extra care to make sure we don't sever the UI while keeping the right dimensions, as well as making tests pass.

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Show a "site information" header in the identity popup

WIP Patch

Comment 3

[Johann Hofmann [:johannh]]

Attachment: Show a "site information" header in the identity popup

Paolo, this is a pretty hacky first attempt at it, feature-wise I think this should be fine, but I still need to fix up tests.

I was mostly looking for feedback on whether you can think of less awful ways to work around some of the limitations here.

A few notes:

- I would like to reuse the panel header from PanelMultiView

- The panel header is normally removed for mainViews, so we need to work around that

- For spoofing protection we need to use the multiline .textContent variant of the XUL <label> instead of the default .value

- Because the panel header has a minimum height of 40 I also needed to add a workaround to the descriptionHeightWorkaround

Comment 4

[:Paolo Amadini]

Comment on attachment 9011469 [details] [diff] [review]
Show a "site information" header in the identity popup

(In reply to Johann Hofmann [:johannh] from comment #3)
> - I would like to reuse the panel header from PanelMultiView
> - The panel header is normally removed for mainViews, so we need to work
> around that
> - Because the panel header has a minimum height of 40 I also needed to add a
> workaround to the descriptionHeightWorkaround

Probably the simplest thing to do here would be to avoid reusing the subview header styles, and associated flex layout, which I guess is causing the issues with the descriptionHeightWorkaround. You may also want the horizontal margin of this header to be different, and match the margin we have between the panel border and the section icons.

Also, at the cost of scope creep, it would be good to fix the style of the button that opens the subview to look consistent with the setting buttons, and also rearrange how the subview looks, avoiding the need for multi-line section-header-style label we still have there. While easy to implement, it doesn't have to happen in this bug, but I'd recommend doing it in the same release.

Comment 5

[Johann Hofmann [:johannh]]

(In reply to :Paolo Amadini from comment #4)
> Comment on attachment 9011469 [details] [diff] [review]
> Show a "site information" header in the identity popup
> 
> (In reply to Johann Hofmann [:johannh] from comment #3)
> > - I would like to reuse the panel header from PanelMultiView
> > - The panel header is normally removed for mainViews, so we need to work
> > around that
> > - Because the panel header has a minimum height of 40 I also needed to add a
> > workaround to the descriptionHeightWorkaround
> 
> Probably the simplest thing to do here would be to avoid reusing the subview
> header styles, and associated flex layout, which I guess is causing the
> issues with the descriptionHeightWorkaround. You may also want the
> horizontal margin of this header to be different, and match the margin we
> have between the panel border and the section icons.

Hm, that doesn't really sound less hacky to me than what I'm doing right now. Just a different kind of workaround. We should probably chat about this.

> 
> Also, at the cost of scope creep, it would be good to fix the style of the
> button that opens the subview to look consistent with the setting buttons,
> and also rearrange how the subview looks, avoiding the need for multi-line
> section-header-style label we still have there. While easy to implement, it
> doesn't have to happen in this bug, but I'd recommend doing it in the same
> release.

I'd leave that up to Bryan. Bryan? :)

Comment 6

[Johann Hofmann [:johannh]]

Attachment: Bug 1476220 - Show a "site information" header in the identity popup. r=paolo

Comment 7

[:Paolo Amadini]

Attachment: Other approach to proper word wrapping

I tried the patch locally, and if I understand correctly the entire reason why box layout cannot be used is that the "overflow-wrap" CSS property is not supported in XUL labels. This is unfortunate, but it looks we need to use this wrapping method, because "word-break: break-all;" would only work if we had a domain name with no other text.

Here I coded another version that doesn't use flex layout and doesn't require changes to the descriptionHeightWorkaround function. This is still a workaround for lack of proper wrapping support in XUL, but looks slightly better than before.

Some basic testing shows that this works alright, let me know if there is any issue with this other approach that I haven't noticed.

Comment 8

[Johann Hofmann [:johannh]]

Thanks for digging into this. I can confirm that this seems to work fine and I don't really mind either way, so I'm happy to incorporate your changes. I'll get a new patch up!

Comment 9

[Tanvi Vyas[:tanvi]]

Do you think this will go into Firefox 64?  Do you still need something from Bryan?  We can try pinging him directly.

Comment 10

[Johann Hofmann [:johannh]]

(In reply to Tanvi Vyas[:tanvi] from comment #9)
> Do you think this will go into Firefox 64?  Do you still need something from
> Bryan?  We can try pinging him directly.

Yeah, I hope this will get into 64. I don't think we need any immediate action from Bryan to move it forward, the needinfo was just for the question in comment 5.

Paolo, did you see that I updated the patch in Phabricator last week? I know that it's sometimes easy to miss these updates.

Thanks!

Comment 11

[:Paolo Amadini]

I don't get reminder e-mails for these reviews, so when needed in the future feel free to set an additional needinfo.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/015250ccee42
Show a "site information" header in the identity popup. r=paolo

Comment 13

[Narcis Beleuzu [:NarcisB]]

Backed out for causing en-US failures on identity-popup-host.

Backout link: https://hg.mozilla.org/integration/autoland/rev/a66fcc9bca65225494acbc59fb4460309b7fc216
Push link: https://hg.mozilla.org/integration/autoland/rev/015250ccee42c76fb7d2437ac9f1b6d6272486db
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=204176661&repo=autoland&lineNumber=5136

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/679d009963f9
Show a "site information" header in the identity popup. r=paolo

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/679d009963f9

Comment 16

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 63.0a1 (2018-07-21)on WIndows 7, 64 Bit!
This bug's fix is verified with latest Nightly!

Build ID 	20181015100128
User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0