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)

enhancement

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: nobody → standard8
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)
Attached image 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)
Attached file Illustration SVGs.zip (obsolete) —
SVGs for the illustrations
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.
(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.
Flags: needinfo?(mheubusch)
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)
Attachment #8905781 - Attachment is obsolete: true
Attached file SVGs.zip (obsolete) —
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)
Attached image 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 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-
(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)
Attached file SVGs.zip
Updated the images so they are at 630x35.
Attachment #8907196 - Attachment is obsolete: true
Flags: needinfo?(epang)
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+
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
Attached video 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)
(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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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]
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]
As per Comment 27 and Comment 28, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
Depends on: 1424480
Depends on: 1439033
Depends on: 1438180
You need to log in before you can comment on or make changes to this bug.