Update urlbar in chromeless popups to match Photon style

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: johannh, Assigned: prathiksha)

Tracking

(Depends on 1 bug, Blocks 1 bug)

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

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-visual], URL)

Attachments

(6 attachments)

(Reporter)

Description

2 years ago
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.
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot

Comment 1

2 years ago
I think it's disabled and that's why it's grey? I think it's always been like that.
(Reporter)

Comment 2

2 years ago
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.

Updated

2 years ago
Summary: Urlbar in chromeless popups doesn't have a white background → Redesign Urlbar in chromeless popups to match Photon style
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!
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
(Reporter)

Comment 5

2 years ago
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.
Summary: Redesign Urlbar in chromeless popups to match Photon style → Update urlbar in chromeless popups to match Photon style
Whiteboard: [reserve-photon-visual] → [photon-visual][triage]
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Priority: P3 → P4
(Reporter)

Comment 6

2 years ago
Popups look much better now, except on Windows, which we probably should fix.
(Assignee)

Updated

2 years ago
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Priority: P4 → P1
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.
Forgot to attach a Mac example. https://cl.ly/2c1I0T0I1D31

Comment 9

2 years ago
(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.
(Reporter)

Comment 10

2 years ago
(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.
Depends on: 1403853
Depends on: 1404248
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
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.
(Reporter)

Comment 14

2 years ago
mozreview-review
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 :)
Attachment #8915570 - Flags: review?(jhofmann)
Comment hidden (mozreview-request)

Comment 16

2 years ago
mozreview-review
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.
Attachment #8915570 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)

Comment 18

2 years ago
mozreview-review
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
Attachment #8915570 - Flags: review?(dao+bmo) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 19

2 years ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa16d953827f
Update urlbar in chromeless popups to match Photon style. r=dao
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aa16d953827f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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]: /
Attachment #8915570 - Flags: approval-mozilla-beta?
(Reporter)

Updated

2 years ago
Depends on: 1406814
Comment on attachment 8915570 [details]
Bug 1368460 - Update urlbar in chromeless popups to match Photon style.

Photon polish, beta57+
Attachment #8915570 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Hi, :prathiksha can you please tell me which is the expected behavior? Thanks
Flags: needinfo?(prathikshaprasadsuman)
(Assignee)

Comment 29

2 years ago
(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!
Flags: needinfo?(prathikshaprasadsuman)
Thanks for testing this, Ovidiu. bgcolor change will be discussed on bug 1406814 – so don’t worry about testing that.
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.
Status: RESOLVED → VERIFIED
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.