Show a "site information" header in the identity popup

RESOLVED FIXED in Firefox 64

Status

()

P2
normal
RESOLVED FIXED
8 months ago
4 months ago

People

(Reporter: johannh, Assigned: johannh, NeedInfo)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [privacy-panel])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

8 months ago
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.
(Assignee)

Comment 1

7 months ago
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.
Whiteboard: [privacy-panel-64]
(Assignee)

Updated

6 months ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 months ago
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
Attachment #9008653 - Attachment is obsolete: true
Attachment #9011469 - Flags: feedback?(paolo.mozmail)

Comment 4

6 months ago
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.
Attachment #9011469 - Flags: feedback?(paolo.mozmail)
(Assignee)

Comment 5

6 months ago
(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? :)
Flags: needinfo?(bbell)
(Assignee)

Updated

6 months ago
Attachment #9011469 - Attachment is obsolete: true

Comment 7

6 months ago
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.

Updated

6 months ago
Flags: needinfo?(jhofmann)
(Assignee)

Comment 8

6 months ago
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!
Flags: needinfo?(jhofmann)
Whiteboard: [privacy-panel-64] → [privacy-panel]

Comment 9

6 months ago
Do you think this will go into Firefox 64?  Do you still need something from Bryan?  We can try pinging him directly.
(Assignee)

Comment 10

6 months ago
(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!
Flags: needinfo?(paolo.mozmail)
(Assignee)

Updated

6 months ago
Blocks: 1496696

Comment 11

6 months ago
I don't get reminder e-mails for these reviews, so when needed in the future feel free to set an additional needinfo.
Flags: needinfo?(paolo.mozmail)

Comment 12

6 months ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/015250ccee42
Show a "site information" header in the identity popup. r=paolo

Comment 14

6 months ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/679d009963f9
Show a "site information" header in the identity popup. r=paolo

Comment 15

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/679d009963f9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
(Assignee)

Updated

6 months ago
Blocks: 1497827
(Assignee)

Updated

6 months ago
Flags: needinfo?(jhofmann)
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
QA Whiteboard: [bugday-20181010]

Updated

4 months ago
Depends on: 1511834
You need to log in before you can comment on or make changes to this bug.