911876 - Clickable area for findbar close button is too small

Status: RESOLVED FIXED

Description

[Matt Brubeck (:mbrubeck)]

The "close" button on the find toolbar is very small, making it hard to click.  We should make the clickable area extend farther than the bounds of the tiny "X" button.  In particular, it should extend all the way to the edge of the screen when the window is maximized.

We already do this for the close button in <notification> bars, by using negative margins:
http://hg.mozilla.org/mozilla-central/file/644aac25b7ab/toolkit/themes/winstripe/global/notification.css#l171

We should add similar styles to the findbar theme files:
http://mxr.mozilla.org/mozilla-central/find?string=theme.*findBar.css&tree=mozilla-central

Comment 1

[Manish Goregaokar [:manishearth]]

The bug does not apply to Linux. I believe that it doesn't apply to Mac as well.

http://i.stack.imgur.com/KbAli.gif

Comment 2

[Abhinav Singhal]

Please assign me this bug to solve.

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #0)
> We already do this for the close button in <notification> bars, by using
> negative margins:
> http://hg.mozilla.org/mozilla-central/file/644aac25b7ab/toolkit/themes/
> winstripe/global/notification.css#l171

Oops, please ignore this link; it's for a different widget than I thought it was.  The correct link to the CSS that gives the <notification> close button its padding is here:
http://hg.mozilla.org/mozilla-central/file/681a8e611ede/toolkit/themes/windows/global/notification.css#l60

We will also need to get rid of the padding in the <findbar> widget and make its buttons taller instead:
http://hg.mozilla.org/mozilla-central/file/681a8e611ede/toolkit/themes/windows/global/findBar.css#l8

Comment 4

[Kai Strecker [:kstrecker]]

Attachment: 911876.patch

This is my first bug patch. I hope that it's ok.

Comment 5

[Dão Gottwald [::dao]]

This already looks pretty good, but the negative margin is a little sketchy. I think we should instead move the close button outside the "findbar-container" hbox (here: http://hg.mozilla.org/mozilla-central/annotate/2f4397db1830/toolkit/content/widgets/findbar.xml#l193) and move the padding from |findbar| to |.findbar-container|.

Comment 6

[Kai Strecker [:kstrecker]]

Attachment: 911876_2.patch

Here's my second patch.
I hope, that this is ok.
There are no negative values made by me.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 815035 [details] [diff] [review]
911876_2.patch

Thanks again, this looks nice.  Some drive-by feedback before this is reviewed:

>     <content hidden="true">
>+	<xul:toolbarbutton anonid="find-closebutton"
>+                        class="findbar-closebutton"
>+                        tooltiptext="&findCloseButton.tooltip;"
>+                        oncommand="close();"/>
>     <xul:hbox anonid="findbar-container" class="findbar-container" flex="1" align="center">

Shouldn't the close button come *after* the #findbar-container, to keep it in the same location on the right?

Style nit: The indentation should be adjusted.

>+  //padding: 4px 8px;

We should just remove this line if it is no longer needed.  (// is not valid for comments in CSS.)

>+.findbar-container {
>+	padding: 0px 0px 0px 8px;
>+}

Would it make sense to keep the 4px top/bottom padding here (just to ensure that the padding will remain even if something inside this box gets enlarged for some reason, like a font size setting)?

Very minor nit: You can replace "0px" with "0" for consistency and conciseness.

Comment 8

[Kai Strecker [:kstrecker]]

Attachment: 911876.patch

Here's the third patch.
But i don't know, which rows i should indent.

Comment 9

[Matt Brubeck (:mbrubeck)]

kstrecker, could you post a single patch with all of your changes?  If you are using the "mq" extension, you can apply the first patch and then "hg qfold" the other two patches to create a combined patch.  Or if not, you can run "hg diff -r $PARENT >combined-patch.diff" (replace $PARENT with the last commit before your patches).

Comment 10

[Kai Strecker [:kstrecker]]

Attachment: 911876.patch

Here it is.

Comment 11

[Kai Strecker [:kstrecker]]

Attachment: 911876.patch

Here it is hopefully the final version.

Comment 12

[Dão Gottwald [::dao]]

Comment on attachment 815481 [details] [diff] [review]
911876.patch

This is going in the right direction but isn't quite there yet.

>+.findbar-container {
>+	padding: 0 0 0 8px;
>+}

Use this here:

  padding-top: 4px;
  padding-bottom: 4px;
  -moz-padding-start: 8px;

You need to use -moz-padding-start instead of padding-left since otherwise the padding is on the wrong side in right-to-left locales such as Arabic or Hebrew.

> .findbar-closebutton {
>   -moz-margin-start: 4px;
>   border: none;
>-  padding: 0;
>+  padding: 7px 11px 7px 7px;
>+  margin-left: 0;
>   list-style-image: url("chrome://global/skin/icons/close.png");
>   -moz-appearance: none;
>   -moz-image-region: rect(0, 16px, 16px, 0);
> }

Remove the "-moz-margin-start: 4px;" line instead of overriding it with margin-left.

Use -moz-padding-start and -moz-padding-end instead of -left/-right.

I think you can stop setting the top and bottom padding once you've added the vertical padding to .findbar-container like I mentioned above.

You'll also need to update toolkit/themes/linux/global/findBar.css and toolkit/themes/osx/global/findBar.css.

Comment 13

[Dão Gottwald [::dao]]

Kai, were you able to make progress based on comment 12? Let me know if something wasn't clear or you got stuck somewhere.

Comment 14

[Kai Strecker [:kstrecker]]

Attachment: 911876.patch

Ok, here it is.

Comment 15

[Dão Gottwald [::dao]]

Comment on attachment 821806 [details] [diff] [review]
911876.patch

>--- a/toolkit/themes/windows/global/findBar.css
>+++ b/toolkit/themes/windows/global/findBar.css
>@@ -1,16 +1,15 @@
> /* 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/. */
> 
> @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> 
> findbar {
>-  //padding: 4px 8px;
>   box-shadow: 0 1px 1px rgba(0,0,0,.1) inset;

This doesn't seem to be diffed against mozilla-central. Matt, can you help Kai fix this up? I suspect it has something to do with mq, which I don't use myself as I consider it overkill ;)

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: patch for mozilla-central

Here's the patch rebased onto mozilla-central.  Kai, you might want to remove any previous versions of the patch and then import this one.  If you make further changes, make sure to do it by modifying this patch rather than creating any additional commits/patches on top of it.

One quick thing I noticed:

> .findbar-container {
>   padding: 0 0 0 8px;
> }

I think this should use -moz-padding-start so it works in RTL locales.

We should make sure to test this change with the Linux and Mac themes to make sure they still look correct.  (I didn't see any obvious problems, but I didn't look very closely.)

Comment 17

[Dão Gottwald [::dao]]

Attachment: updated patch

(In reply to Matt Brubeck (:mbrubeck) from comment #16)
> One quick thing I noticed:
> 
> > .findbar-container {
> >   padding: 0 0 0 8px;
> > }
> 
> I think this should use -moz-padding-start so it works in RTL locales.

Yes. I've also moved some margins/padding around between the close button and .findbar-container. Should be ready to land with these changes.

Comment 18

[Dão Gottwald [::dao]]

https://hg.mozilla.org/integration/fx-team/rev/8d0bcaf687c5

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d0bcaf687c5

Comment 20

[Guilherme Lima]

I think it's weird that the clickable area is different from the size of the button.

Comment 21

[Mike de Boer [:mikedeboer]]

Why wasn't this passed by UX for - at least - a ui-review?

Comment 22

[Matt Brubeck (:mbrubeck)]

(In reply to Mike de Boer [:mikedeboer] from comment #21)
> Why wasn't this passed by UX for - at least - a ui-review?

I don't think there's a real design change to review here -- this is restoring the originally-designed behavior of the find bar close button that was present from Firefox 3 (or earlier - I haven't checked farther back) up until Firefox 25 when it was unintentionally regressed by bug 776708.  It also makes the findbar's close button consistent again with the ones in our other closeable toolbars (such as the <notification> widget, and the late add-on bar).

I did check to make sure that removing the padding wasn't an intentional change specified by UX in bug 565552 or bug 776708.

Comment 23

[Luís Miguel [:quicksaver] (not reading bugmail)]

Although I don't agree with the behavior this patch introduces (mainly for reason 2 in bug 939523), that is at least reasonable of course.

I do have a technical question, why was the close button physically moved just to achieve this? I did a quick test by adding "padding: 3px 5px;" back to the windows stylesheet (which was the only OS affected apparently, although to be honest I didn't check in Mac yet but I will tomorrow) and it works perfectly without physically moving it outside the findbar-container element and adding a lot of different margins and paddings all around that obviously screwed it up at least in linux. I didn't understand the need for all of this.