[photon] Add a checkbox to control whether the separate Search Bar is present in the toolbar

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
a year ago
7 months ago

People

(Reporter: Paolo, Assigned: standard8)

Tracking

(Depends on: 1 bug)

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

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

a year ago
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

a year ago
Assignee: nobody → standard8
(Assignee)

Comment 1

a year ago
Philipp, have you got a design/layout/wording for this checkbox please?
Flags: needinfo?(philipp)
Adding Michelle on for needinfo since she has the best words.
Flags: needinfo?(mheubusch)
Status: NEW → ASSIGNED
Flags: needinfo?(philipp) → needinfo?(epang)
Created attachment 8904736 [details]
Search-bar-settings.png

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)
Created attachment 8904737 [details]
[SPEC]Search-bar-settings.png
Created attachment 8904738 [details]
Illustration SVGs.zip

SVGs for the illustrations
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year 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

a year 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

a year ago
Flags: needinfo?(mheubusch)
(Assignee)

Comment 11

a year 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

a year ago
Attachment #8905781 - Attachment is obsolete: true
Created attachment 8907196 [details]
SVGs.zip

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

a year ago
Created attachment 8907222 [details]
Image of implementation

This is the layout on Mac with the patch applied.
Attachment #8907222 - Flags: ui-review?(epang)
Attachment #8904738 - Attachment is obsolete: true
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.
Attachment #8907222 - Flags: ui-review?(epang) → ui-review+

Comment 17

a year 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

a year 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)
Created attachment 8907724 [details]
SVGs.zip

Updated the images so they are at 630x35.
Attachment #8907196 - Attachment is obsolete: true
Flags: needinfo?(epang)
Comment hidden (mozreview-request)

Comment 21

a year 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

a year 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
Created attachment 8908172 [details]
search.mp4

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

a year 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)
https://hg.mozilla.org/mozilla-central/rev/62517cf482ae
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Comment 27

a year 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

a year 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

a year ago
As per Comment 27 and Comment 28, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified

Updated

10 months ago
Depends on: 1424480

Updated

7 months ago
Depends on: 1439033

Updated

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