Closed
Bug 1393437
Opened 7 years ago
Closed 7 years ago
[photon] Add a checkbox to control whether the separate Search Bar is present in the toolbar
Categories
(Firefox :: Settings UI, enhancement, P1)
Firefox
Settings UI
Tracking
()
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: Paolo, Assigned: standard8)
References
(Depends on 1 open bug)
Details
(Whiteboard: [fxsearch])
Attachments
(6 files, 3 obsolete files)
A new checkbox in the preferences should control whether the separate Search Bar is present in the toolbar. This checkbox is redundant as it would always reflect the state of the widget that is set using Customize mode.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → standard8
Assignee | ||
Comment 1•7 years ago
|
||
Philipp, have you got a design/layout/wording for this checkbox please?
Flags: needinfo?(philipp)
Comment 2•7 years ago
|
||
Adding Michelle on for needinfo since she has the best words.
Flags: needinfo?(mheubusch)
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 3•7 years ago
|
||
Handing this over to Eric
Updated•7 years ago
|
Flags: needinfo?(philipp) → needinfo?(epang)
Comment 4•7 years ago
|
||
Instead of adding a single checkbox we should add 2 radio buttons to change between a tab bar with the search bar and without.
Each radio button also has a UI illustration that accompanies the copy (attached as SVGs).
The toggles should live update the UI when changed. Also, the toggles should update when the search bar is removed/added with customize menu.
Michelle, can you help review the copy?
Thanks!
Flags: needinfo?(epang)
Comment 5•7 years ago
|
||
Comment 6•7 years ago
|
||
SVGs for the illustrations
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Attaching what I have currently. The existing <radio> elements aren't capable enough for laying out in the style requested, so I'm creating a custom binding.
Unfortunately I'm currently stuck with the grid - the grid element doesn't seem to be behaving like a grid element, the second row is not in aligned with the first row.
Posting as I'm out for at least some of today, and maybe other folks have better ideas.
Comment 10•7 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #9)
> Attaching what I have currently. The existing <radio> elements aren't
> capable enough for laying out in the style requested, so I'm creating a
> custom binding.
Well, looking at the spec, you could just have to radio items interspersed with hardcoded XUL image elements that are aligned correctly, right? That seems like it might be a simpler solution, though perhaps less elegant.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(mheubusch)
Assignee | ||
Comment 11•7 years ago
|
||
Eric, I'm confused by the height/width of the images in your specifications. When I try and lay that out, I'm getting it almost twice the width of the preferences page.
Did you generate it on a high resolution display?
Also, I can change these down to 634px and 35px, however the ratio doesn't seem quite right - any ideas?
Flags: needinfo?(epang)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8905781 -
Attachment is obsolete: true
Comment 13•7 years ago
|
||
Hey Mark, must be cause I created them with a retina display. I've updated the svgs to 1255x70 which should help when re-scaling to 633x35. Let me know if you're still running into problems.
Flags: needinfo?(epang)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
This is the layout on Mac with the patch applied.
Attachment #8907222 -
Flags: ui-review?(epang)
Updated•7 years ago
|
Attachment #8904738 -
Attachment is obsolete: true
Comment 16•7 years ago
|
||
Comment on attachment 8907222 [details]
Image of implementation
This looks to me! Thanks for the try build, everything worked as expected! Also wanted to mention I spoke with Michelle and we agreed to keep the illustrations in and the text as is.
Updated•7 years ago
|
Attachment #8907222 -
Flags: ui-review?(epang) → ui-review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8905780 [details]
Bug 1393437 - Add a checkbox to preferences to control whether the separate Search Bar is present in the toolbar. ui-review=epang
https://reviewboard.mozilla.org/r/177584/#review184064
::: browser/components/preferences/in-content/search.xul:37
(Diff revision 3)
>
> + <groupbox id="searchbarGroup" data-category="paneSearch">
> + <caption><label id="searchbarLabel">&searchBar.label;</label></caption>
> + <radiogroup id="searchBarVisibleGroup" aria-labelledby="searchbarLabel" preference="browser.search.widget.inNavBar">
> + <radio id="searchBarHiddenRadio" value="false" label="&searchBar.hidden.label;"/>
> + <image class="searchBarImage searchBarHiddenImage"/>
Here and for the other one, you'll need an alt text description, or perhaps you should set role=presentation if you don't think we need an accessible description besides the radio element.
::: browser/themes/shared/incontentprefs/no-search-bar.svg:1
(Diff revision 3)
> +<svg id="no_side_bar.svg" xmlns="http://www.w3.org/2000/svg" width="1267" height="71" viewBox="0 0 1267 71">
This doesn't need an ID, and it does need a license header.
::: browser/themes/shared/incontentprefs/no-search-bar.svg:3
(Diff revision 3)
> +<svg id="no_side_bar.svg" xmlns="http://www.w3.org/2000/svg" width="1267" height="71" viewBox="0 0 1267 71">
> + <defs>
> + <style>
You don't need the `<defs>` tag here. Did you run this through svgo?
::: browser/themes/shared/incontentprefs/no-search-bar.svg:4
(Diff revision 3)
> +<svg id="no_side_bar.svg" xmlns="http://www.w3.org/2000/svg" width="1267" height="71" viewBox="0 0 1267 71">
> + <defs>
> + <style>
> + .cls-1 {
Please use more meaningful class names.
::: browser/themes/shared/incontentprefs/no-search-bar.svg:28
(Diff revision 3)
> + .cls-4 {
> + fill: #f9f9fa;
> + }
> + </style>
> + </defs>
> + <rect id="Background" class="cls-1" x="0.5" y="0.5" width="1266" height="70" rx="4" ry="4"/>
The ids and data-name attributes seem superfluous.
::: browser/themes/shared/incontentprefs/no-search-bar.svg:29
(Diff revision 3)
> + fill: #f9f9fa;
> + }
> + </style>
> + </defs>
> + <rect id="Background" class="cls-1" x="0.5" y="0.5" width="1266" height="70" rx="4" ry="4"/>
> + <rect id="Address_Bar" data-name="Address Bar" class="cls-2" x="207.5" y="12.5" width="876" height="46" rx="4" ry="4"/>
This is the only element with `cls-2` so it probably doesn't make sense to make that a class. Just use the relevant attributes.
It looks like this is the case for several of the classes (ie they're used for only 1 element).
::: browser/themes/shared/incontentprefs/no-search-bar.svg:32
(Diff revision 3)
> + </defs>
> + <rect id="Background" class="cls-1" x="0.5" y="0.5" width="1266" height="70" rx="4" ry="4"/>
> + <rect id="Address_Bar" data-name="Address Bar" class="cls-2" x="207.5" y="12.5" width="876" height="46" rx="4" ry="4"/>
> + <path id="Div_Line" data-name="Div Line" class="cls-3" d="M1803,524h1v68h-1V524Z" transform="translate(-596.5 -522.5)"/>
> + <path id="Search_Icon" data-name="Search Icon" class="cls-3" d="M835.818,567.008l-6.169-6.17a7.767,7.767,0,0,0-1.286-10.411,7.708,7.708,0,0,0-10.8,10.925,7.9,7.9,0,0,0,10.411,1.286l6.169,6.169a1.243,1.243,0,0,0,1.8,0,1.377,1.377,0,0,0-.129-1.8h0Zm-12.467-5.527a5.141,5.141,0,1,1,5.141-5.141A5.27,5.27,0,0,1,823.351,561.481Z" transform="translate(-596.5 -522.5)"/>
> + <path id="Menu" class="cls-3" d="M1841.5,557.5h-15a1.5,1.5,0,0,0,0,3h15a1.5,1.5,0,0,0,0-3m0,7.5h-15a1.5,1.5,0,0,0,0,3h15a1.5,1.5,0,0,0,0-3m0-15h-15a1.5,1.5,0,0,0,0,3h15a1.5,1.5,0,0,0,0-3" transform="translate(-596.5 -522.5)"/>
Here and elsewhere, please reconcile the transforms and the coordinates. It seems like the initial Move instruction in the path is the only thing you'd need to touch for this, so it should be quite straightforward.
::: browser/themes/shared/incontentprefs/search.css:6
(Diff revision 3)
> /* This Source Code Form is subject to the terms of the Mozilla Public
> * License, v. 2.0. If a copy of the MPL was not distributed with this
> * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>
> +.searchBarImage {
> + height: 35px;
The fact that this doesn't evenly divide the actual size specified in the SVG isn't right. Please make the size of the SVG match the size specified here.
I'll also note that I'm concerned about how this will look on lodpi given that the original image was specified as hidpi. Because the stroke width is 1px in the SVG's terms, and we then halve the size of the image, it seems like it would be possible for some of the strokes to go missing or become very faint. Did you test this? IMO the image should be made for lodpi and, if necessary, we can use a media query or something like that to alter the stroke widths to make the lines thinner, if desired.
::: browser/themes/shared/incontentprefs/search.css:13
(Diff revision 3)
> + margin-left: 33px;
> + background-size: 631px 35px;
> +}
> +
> +.searchBarHiddenImage {
> + background-image: url("chrome://browser/skin/preferences/in-content/no-search-bar.svg");
Shouldn't this use list-style image, or just be set in the markup? We no longer support 'full' themes, so we don't need to worry about that aspect of things. Then we can remove the double height/width vs. background-size stuff.
::: browser/themes/shared/incontentprefs/search.css:17
(Diff revision 3)
> +.searchBarHiddenImage {
> + background-image: url("chrome://browser/skin/preferences/in-content/no-search-bar.svg");
> +}
> +
> +#searchBarShownRadio {
> + /* Allow a little to separate the radio from the image above it. */
Nit: A little what? :-)
Attachment #8905780 -
Flags: review?(gijskruitbosch+bugs) → review-
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to :Gijs from comment #17)
> ::: browser/themes/shared/incontentprefs/no-search-bar.svg:32
> (Diff revision 3)
> > + </defs>
> > + <rect id="Background" class="cls-1" x="0.5" y="0.5" width="1266" height="70" rx="4" ry="4"/>
> > + <rect id="Address_Bar" data-name="Address Bar" class="cls-2" x="207.5" y="12.5" width="876" height="46" rx="4" ry="4"/>
> > + <path id="Div_Line" data-name="Div Line" class="cls-3" d="M1803,524h1v68h-1V524Z" transform="translate(-596.5 -522.5)"/>
> > + <path id="Search_Icon" data-name="Search Icon" class="cls-3" d="M835.818,567.008l-6.169-6.17a7.767,7.767,0,0,0-1.286-10.411,7.708,7.708,0,0,0-10.8,10.925,7.9,7.9,0,0,0,10.411,1.286l6.169,6.169a1.243,1.243,0,0,0,1.8,0,1.377,1.377,0,0,0-.129-1.8h0Zm-12.467-5.527a5.141,5.141,0,1,1,5.141-5.141A5.27,5.27,0,0,1,823.351,561.481Z" transform="translate(-596.5 -522.5)"/>
> > + <path id="Menu" class="cls-3" d="M1841.5,557.5h-15a1.5,1.5,0,0,0,0,3h15a1.5,1.5,0,0,0,0-3m0,7.5h-15a1.5,1.5,0,0,0,0,3h15a1.5,1.5,0,0,0,0-3m0-15h-15a1.5,1.5,0,0,0,0,3h15a1.5,1.5,0,0,0,0-3" transform="translate(-596.5 -522.5)"/>
>
> Here and elsewhere, please reconcile the transforms and the coordinates. It
> seems like the initial Move instruction in the path is the only thing you'd
> need to touch for this, so it should be quite straightforward.
>
> ::: browser/themes/shared/incontentprefs/search.css:6
> (Diff revision 3)
> > /* This Source Code Form is subject to the terms of the Mozilla Public
> > * License, v. 2.0. If a copy of the MPL was not distributed with this
> > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >
> > +.searchBarImage {
> > + height: 35px;
>
> The fact that this doesn't evenly divide the actual size specified in the
> SVG isn't right. Please make the size of the SVG match the size specified
> here.
>
> I'll also note that I'm concerned about how this will look on lodpi given
> that the original image was specified as hidpi. Because the stroke width is
> 1px in the SVG's terms, and we then halve the size of the image, it seems
> like it would be possible for some of the strokes to go missing or become
> very faint. Did you test this? IMO the image should be made for lodpi and,
> if necessary, we can use a media query or something like that to alter the
> stroke widths to make the lines thinner, if desired.
Eric, can you help here with the images please?
Flags: needinfo?(epang)
Comment 19•7 years ago
|
||
Updated the images so they are at 630x35.
Attachment #8907196 -
Attachment is obsolete: true
Flags: needinfo?(epang)
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8905780 [details]
Bug 1393437 - Add a checkbox to preferences to control whether the separate Search Bar is present in the toolbar. ui-review=epang
https://reviewboard.mozilla.org/r/177584/#review184974
r=me with the image sizes fixed.
::: browser/themes/shared/incontentprefs/search.css:6
(Diff revision 4)
> /* This Source Code Form is subject to the terms of the Mozilla Public
> * License, v. 2.0. If a copy of the MPL was not distributed with this
> * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>
> +.searchBarImage {
> + height: 35px;
The actual images are 36px high, no?
::: browser/themes/shared/incontentprefs/search.css:7
(Diff revision 4)
> * License, v. 2.0. If a copy of the MPL was not distributed with this
> * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>
> +.searchBarImage {
> + height: 35px;
> + width: 630px;
and 631 wide...
Attachment #8905780 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request) |
Comment 23•7 years ago
|
||
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62517cf482ae
Add a checkbox to preferences to control whether the separate Search Bar is present in the toolbar. r=Gijs ui-review=epang
Comment 24•7 years ago
|
||
Hey Mark,
just noticed an issue in the build you sent me the other day, not sure if you've already seen it though.
When you first launch the browser (with the search bar active) and go to prefs it automatically has the first radio button selected. After you change the settings it’s right but it really should launch with whatever is currently set in the toolbar. See video.
Flags: needinfo?(standard8)
Assignee | ||
Comment 25•7 years ago
|
||
(In reply to Eric Pang [:epang] UX from comment #24)
> When you first launch the browser (with the search bar active) and go to
> prefs it automatically has the first radio button selected. After you
> change the settings it’s right but it really should launch with whatever is
> currently set in the toolbar. See video.
Thanks for pointing that out. I've taken a look and it is actually a problem with the preference value not being set correctly, rather than a problem with the patch I landed here.
I've separated this out into bug 1399968 and will investigate it there.
Flags: needinfo?(standard8)
Comment 26•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 27•7 years ago
|
||
I have reproduced this bug with Nightly 57.0a1 (2017-08-24) on Ubuntu 16.04, 64 bit!
The fix is now verified on Latest beta 57.0b3
Build ID 20170925150345
User Agent Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170927]
Comment 28•7 years ago
|
||
I have reproduced this bug with Nightly 57.0a1 (2017-08-24) on Windows 8.1, 64 bit!
The fix is now verified on Latest Beta.
Build ID : 20170928180207
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0
[bugday-20170927]
Comment 29•7 years ago
|
||
As per Comment 27 and Comment 28, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•