Closed
Bug 1368460
Opened 7 years ago
Closed 7 years ago
Update urlbar in chromeless popups to match Photon style
Categories
(Firefox :: Theme, defect, P1)
Firefox
Theme
Tracking
()
VERIFIED
FIXED
Firefox 58
People
(Reporter: johannh, Assigned: prathiksha)
References
(Depends on 1 open bug, Blocks 1 open bug, )
Details
(Whiteboard: [reserve-photon-visual])
Attachments
(6 files)
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.
Updated•7 years ago
|
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
Comment 1•7 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•7 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•7 years ago
|
Summary: Urlbar in chromeless popups doesn't have a white background → Redesign Urlbar in chromeless popups to match Photon style
Comment 3•7 years ago
|
||
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!
Updated•7 years ago
|
Comment 4•7 years ago
|
||
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•7 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]
Updated•7 years ago
|
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Updated•7 years ago
|
Priority: P3 → P4
Reporter | ||
Comment 6•7 years ago
|
||
Popups look much better now, except on Windows, which we probably should fix.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Updated•7 years ago
|
Priority: P4 → P1
Comment 7•7 years ago
|
||
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 9•7 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•7 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.
Comment hidden (mozreview-request) |
Comment 12•7 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.
Comment 13•7 years ago
|
||
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
Reporter | ||
Comment 14•7 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•7 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•7 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•7 years ago
|
Keywords: checkin-needed
Comment 19•7 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
![]() |
||
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/aa16d953827f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 21•7 years ago
|
||
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?
status-firefox57:
--- → affected
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+
Comment 23•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/b4eb5da347e5
Comment 24•7 years ago
|
||
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•7 years ago
|
||
Comment 26•7 years ago
|
||
Comment 27•7 years ago
|
||
Comment 28•7 years ago
|
||
Hi, :prathiksha can you please tell me which is the expected behavior? Thanks
Flags: needinfo?(prathikshaprasadsuman)
Assignee | ||
Comment 29•7 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)
Comment 30•7 years ago
|
||
Thanks for testing this, Ovidiu. bgcolor change will be discussed on bug 1406814 – so don’t worry about testing that.
Comment 31•7 years ago
|
||
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.
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
Blocks: photon-visual
Comment 32•7 years ago
|
||
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.
Description
•