Clickable area for findbar close button is too small

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: mbrubeck, Assigned: kstrecker)

Tracking

(Depends on 1 bug, {regression, ux-efficiency})

Trunk
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=mbrubeck][lang=css][good first bug][good first verify])

Attachments

(1 attachment, 7 obsolete attachments)

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
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
OS: All → Windows 8
Summary: Clickable area for findbar close button is too small → Clickable area for findbar close button is too small on Windows
Whiteboard: [mentor=mbrubeck][lang=css] → [mentor=mbrubeck][lang=css][good first bug]
Please assign me this bug to solve.
Assignee: nobody → abhinav.singla
(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
Posted patch 911876.patch (obsolete) — Splinter Review
This is my first bug patch. I hope that it's ok.
Attachment #811442 - Flags: review?(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|.
Posted patch 911876_2.patch (obsolete) — Splinter Review
Here's my second patch.
I hope, that this is ok.
There are no negative values made by me.
Attachment #815035 - Flags: review?
Assignee: abhinav.singla → do6kai
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.
Attachment #815035 - Flags: review?
Attachment #811442 - Attachment is obsolete: true
Attachment #811442 - Flags: review?(dao)
Posted patch 911876.patch (obsolete) — Splinter Review
Here's the third patch.
But i don't know, which rows i should indent.
Attachment #815456 - Flags: review?(dao)
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).
Posted patch 911876.patch (obsolete) — Splinter Review
Here it is.
Attachment #815035 - Attachment is obsolete: true
Attachment #815456 - Attachment is obsolete: true
Attachment #815456 - Flags: review?(dao)
Attachment #815476 - Flags: review?(dao)
Posted patch 911876.patch (obsolete) — Splinter Review
Here it is hopefully the final version.
Attachment #815476 - Attachment is obsolete: true
Attachment #815476 - Flags: review?(dao)
Attachment #815481 - Flags: review?(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.
Attachment #815481 - Flags: review?(dao)
Attachment #815481 - Flags: review-
Attachment #815481 - Flags: feedback+
Kai, were you able to make progress based on comment 12? Let me know if something wasn't clear or you got stuck somewhere.
Flags: needinfo?(do6kai)
Posted patch 911876.patch (obsolete) — Splinter Review
Ok, here it is.
Attachment #815481 - Attachment is obsolete: true
Attachment #821806 - Flags: review?(dao)
Flags: needinfo?(do6kai)
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 ;)
Attachment #821806 - Flags: review?(dao)
Flags: needinfo?(mbrubeck)
Posted patch patch for mozilla-central (obsolete) — Splinter Review
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.)
Attachment #821806 - Attachment is obsolete: true
Attachment #823404 - Flags: review?(dao)
Flags: needinfo?(mbrubeck)
Posted patch updated patchSplinter Review
(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.
Attachment #823404 - Attachment is obsolete: true
Attachment #823404 - Flags: review?(dao)
Attachment #829268 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/8d0bcaf687c5
OS: Windows 8 → All
Summary: Clickable area for findbar close button is too small on Windows → Clickable area for findbar close button is too small
https://hg.mozilla.org/mozilla-central/rev/8d0bcaf687c5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
I think it's weird that the clickable area is different from the size of the button.
Why wasn't this passed by UX for - at least - a ui-review?
Flags: needinfo?(dao)
(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.
Blocks: 776708
Keywords: regression
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.
Whiteboard: [mentor=mbrubeck][lang=css][good first bug] → [mentor=mbrubeck][lang=css][good first bug][good first verify]
Flags: needinfo?(dao)
You need to log in before you can comment on or make changes to this bug.