Closed
Bug 911876
Opened 8 years ago
Closed 7 years ago
Clickable area for findbar close button is too small
Categories
(Toolkit :: Find Toolbar, defect)
Toolkit
Find Toolbar
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: mbrubeck, Assigned: kstrecker)
References
(Depends on 1 open bug)
Details
(Keywords: regression, ux-efficiency, Whiteboard: [mentor=mbrubeck][lang=css][good first bug][good first verify])
Attachments
(1 file, 7 obsolete files)
5.81 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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
Reporter | ||
Updated•8 years ago
|
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]
Comment 2•8 years ago
|
||
Please assign me this bug to solve.
Updated•8 years ago
|
Assignee: nobody → abhinav.singla
Reporter | ||
Comment 3•8 years ago
|
||
(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
Assignee | ||
Comment 4•8 years ago
|
||
This is my first bug patch. I hope that it's ok.
Attachment #811442 -
Flags: review?(dao)
Comment 5•8 years ago
|
||
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|.
Assignee | ||
Comment 6•8 years ago
|
||
Here's my second patch. I hope, that this is ok. There are no negative values made by me.
Attachment #815035 -
Flags: review?
Updated•8 years ago
|
Assignee: abhinav.singla → do6kai
Reporter | ||
Comment 7•8 years ago
|
||
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?
Reporter | ||
Updated•8 years ago
|
Attachment #811442 -
Attachment is obsolete: true
Attachment #811442 -
Flags: review?(dao)
Assignee | ||
Comment 8•8 years ago
|
||
Here's the third patch. But i don't know, which rows i should indent.
Attachment #815456 -
Flags: review?(dao)
Reporter | ||
Comment 9•8 years ago
|
||
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).
Assignee | ||
Comment 10•8 years ago
|
||
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)
Assignee | ||
Comment 11•8 years ago
|
||
Here it is hopefully the final version.
Attachment #815476 -
Attachment is obsolete: true
Attachment #815476 -
Flags: review?(dao)
Attachment #815481 -
Flags: review?(dao)
Comment 12•8 years ago
|
||
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+
Comment 13•8 years ago
|
||
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)
Assignee | ||
Comment 14•8 years ago
|
||
Ok, here it is.
Attachment #815481 -
Attachment is obsolete: true
Attachment #821806 -
Flags: review?(dao)
Flags: needinfo?(do6kai)
Comment 15•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(mbrubeck)
Reporter | ||
Comment 16•8 years ago
|
||
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)
Comment 17•7 years ago
|
||
(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+
Comment 18•7 years ago
|
||
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
Comment 19•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8d0bcaf687c5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
![]() |
||
Comment 20•7 years ago
|
||
I think it's weird that the clickable area is different from the size of the button.
Comment 21•7 years ago
|
||
Why wasn't this passed by UX for - at least - a ui-review?
Updated•7 years ago
|
Flags: needinfo?(dao)
Reporter | ||
Comment 22•7 years ago
|
||
(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
Comment 23•7 years ago
|
||
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.
Updated•7 years ago
|
Whiteboard: [mentor=mbrubeck][lang=css][good first bug] → [mentor=mbrubeck][lang=css][good first bug][good first verify]
Updated•6 years ago
|
Flags: needinfo?(dao)
You need to log in
before you can comment on or make changes to this bug.
Description
•