1368460 - Update urlbar in chromeless popups to match Photon style

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Johann Hofmann [:johannh]]

Attachment: Screen Shot 2017-05-29 at 12.30.08.png

See screenshot. This probably regressed during Photon work. I also have to say I'm not a great fan of the spacing around the urlbar in that window mode. We might want to ask UX for better specifications on popup windows.

Comment 1

[:Gijs (he/him)]

I think it's disabled and that's why it's grey? I think it's always been like that.

Comment 2

[Johann Hofmann [:johannh]]

It's white in release for me on OSX. Is that the bug then? In any case IMO this looks a bit weird, especially with the larger urlbar size.

Comment 3

[Bram Pitoyo [:bram]]

Hi Johann, let’s morph this issue into redesigning the urlbar so it matches the rest of Photon. Along the way, the background colour issue will be fixed!

Comment 4

[Bram Pitoyo [:bram]]

Added mocks on InVision.

I also did a survey of how different browsers handle chromeless windows:
https://docs.google.com/a/mozilla.com/presentation/d/1PvypQCer39ott1NVqcTWhmYuSZZRr3_plybOvS2visU/edit

Comment 5

[Johann Hofmann [:johannh]]

Awesome, thanks Bram! Let's make this an engineering issue then.

Putting it up for triage again since we have new designs and should reconsider if we want to keep this in reserve.

Comment 6

[Johann Hofmann [:johannh]]

Popups look much better now, except on Windows, which we probably should fix.

Comment 7

[Bram Pitoyo [:bram]]

This issue still persists on Mac – additionally, there are a few elements that probably shouldn’t be there (page menu, pocket, bookmarks, hamburger menu).

Let me know if that a Mac-specific spec is needed, or whether we can use the Windows design as a reference.

Comment 8

[Bram Pitoyo [:bram]]

Forgot to attach a Mac example. https://cl.ly/2c1I0T0I1D31

Comment 9

[:Gijs (he/him)]

(In reply to Bram Pitoyo [:bram] from comment #7)
> This issue still persists on Mac – additionally, there are a few elements
> that probably shouldn’t be there (page menu, pocket, bookmarks, hamburger
> menu).

Those are there intentionally. Specifically, the hamburger menu is there because of an explicit UX decision in bug 1015163 comment 4. The page action, pocket and bookmark items are there because prior to australis, the bookmark icon was also in the URL bar and also showed up in popup windows, and we tracked breaking it in australis as a regression (bug 1003779). It seems reasonable that if we show the bookmark icon, we also show other page action items.

> Let me know if that a Mac-specific spec is needed, or whether we can use the
> Windows design as a reference.

I'm confused - comment #6 suggests the opposite, namely that it looks OK on Mac and not on Windows.

Comment 10

[Johann Hofmann [:johannh]]

(In reply to :Gijs from comment #9)
> I'm confused - comment #6 suggests the opposite, namely that it looks OK on
> Mac and not on Windows.

That had me confused for a second as well, but it turns out that I was testing on compact mode instead of normal UI density. In normal UI density it still looks a bit broken. We should make sure it looks fine on all three densities.

Comment 11

[:prathiksha]

Attachment: Bug 1368460 - Update urlbar in chromeless popups to match Photon style.

Review commit: https://reviewboard.mozilla.org/r/186762/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186762/

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8915570 [details]
Bug 1368460 - Update urlbar in chromeless popups to match Photon style.

https://reviewboard.mozilla.org/r/186762/#review191862

::: browser/themes/shared/urlbar-searchbar.inc.css:52
(Diff revision 1)
>  :root[uidensity=touch] .searchbar-textbox {
>    min-height: 32px;
>  }
>  
>  :root[chromehidden~="toolbar"] #urlbar {
> +  margin: 5px;

Can you just set a vertical margin regardless of chromehidden?

::: browser/themes/shared/urlbar-searchbar.inc.css:58
(Diff revision 1)
>    /* Remove excess space between the address bar and the menu button in popups. */
>    margin-inline-end: 0;
>  }
>  
> +:root[chromehidden~="toolbar"] #urlbar:-moz-lwtheme-darktext {
> +  background-color: -moz-Dialog;

-moz-dialog is a native color that can be any RGB value including black, potentially rendering the text invisible.

I suggest we drop the background color change from this bug, as it looks like this opens a can of worms which might not be worthwhile.

Comment 13

[Dão Gottwald [:dao]]

We could also just exclude popup windows here, but I'm not sure we really want that: http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/browser/themes/shared/urlbar-searchbar.inc.css#26-37

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8915570 [details]
Bug 1368460 - Update urlbar in chromeless popups to match Photon style.

https://reviewboard.mozilla.org/r/186762/#review192186

Please request review from dao, as he seems to be on this anyway (I can't forward review because mozreview seems to have a draft pending)

Thanks :)

Comment 15

[:prathiksha]

Comment on attachment 8915570 [details]
Bug 1368460 - Update urlbar in chromeless popups to match Photon style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186762/diff/1-2/

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8915570 [details]
Bug 1368460 - Update urlbar in chromeless popups to match Photon style.

https://reviewboard.mozilla.org/r/186762/#review192208

::: browser/themes/shared/urlbar-searchbar.inc.css:15
(Diff revision 2)
>    background-clip: content-box;
>    border: 1px solid hsla(240,5%,5%,.25);
>    border-radius: var(--toolbarbutton-border-radius);
>    box-shadow: 0 1px 4px rgba(0,0,0,.05);
>    padding: 0;
> -  margin: 0 5px;
> +  margin: 5px 5px;

I think this needs to be "4px 5px" in order to not increase the toolbar height in compact mode. Please double-check.

Comment 17

[:prathiksha]

Comment on attachment 8915570 [details]
Bug 1368460 - Update urlbar in chromeless popups to match Photon style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186762/diff/2-3/

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8915570 [details]
Bug 1368460 - Update urlbar in chromeless popups to match Photon style.

https://reviewboard.mozilla.org/r/186762/#review192284

Thanks!

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aa2cb2d7255839381da3fdbb9fb9079a3994351

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa16d953827f
Update urlbar in chromeless popups to match Photon style. r=dao

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8915570 [details]
Bug 1368460 - Update urlbar in chromeless popups to match Photon style.

Approval Request Comment
[Feature/Bug causing the regression]: photon polish
[User impact if declined]: popup window UI looks a bit crammed
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: trivial patch that adds vertical margin to the address and search bars
[String changes made/needed]: /

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8915570 [details]
Bug 1368460 - Update urlbar in chromeless popups to match Photon style.

Photon polish, beta57+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b4eb5da347e5

Comment 24

[ovidiu boca[:Ovidiu]]

Attachment: Windows 10 popup dark.png

I tested this on Mac OS X 10.12 and Windows 10 with FF Nightly 58.0a1(2017-10-09) and I don't have the same results as the one presented in https://mozilla.invisionapp.com/share/VGBXYQ8WF#/screens. Please see the attached files with the actual results.

Comment 25

[ovidiu boca[:Ovidiu]]

Attachment: Windows 10 popup light.png

Comment 26

[ovidiu boca[:Ovidiu]]

Attachment: Mac popup - dark theme.png

Comment 27

[ovidiu boca[:Ovidiu]]

Attachment: Mac popup - light theme.png

Comment 28

[ovidiu boca[:Ovidiu]]

Hi, :prathiksha can you please tell me which is the expected behavior? Thanks

Comment 29

[:prathiksha]

(In reply to ovidiu boca[:Ovidiu] from comment #28)
> Hi, :prathiksha can you please tell me which is the expected behavior? Thanks

All your attachments look alright to me. We decided not to include background-color changes for the urlbar, in this bug. You only have to check the spacing around the urlbar and test that it does not look like attachment 8872313 [details] for all themes in all modes. Thanks!

Comment 30

[Bram Pitoyo [:bram]]

Thanks for testing this, Ovidiu. bgcolor change will be discussed on bug 1406814 – so don’t worry about testing that.

Comment 31

[ovidiu boca[:Ovidiu]]

Thanks, :prathiksha for your response, based on your comment 29 and the results from comment 24 I will mark this as verified on FF 58.0a1 Nightly. 

Thanks Bram for clearing this out.

Comment 32

[ovidiu boca[:Ovidiu]]

I verified this on Mac OS X 10.10, Windows 10 and Ubuntu 16.04 with FF beta 57.0b9 and I can confirm the fix.