Closed Bug 776708 Opened 12 years ago Closed 11 years ago

Improve the visual appearance of the "find in page" bar

Categories

(Toolkit :: Find Toolbar, defect)

Product:
Toolkit
Component:
Find Toolbar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25
Tracking Flags:
Tracking Status
relnote-firefox --- +

People

(Reporter: cleer, Assigned: mikedeboer)

References

(Depends on 2 open bugs, Blocks 2 open bugs,
URL
)

Details

(Keywords: addon-compat)

Attachments

(5 files, 32 obsolete files)

Screenshot Win8 (desktop)
69.56 KB, image/png
Details
Findbar showing buttons without additional styling
86.65 KB, image/png
Details
screenshot OSX
34.46 KB, image/png
Details
Screenshot Linux (Ubuntu 13.04)
25.57 KB, image/png
Details
Patch: Change the findBar appearance and improve its UX
38.55 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
Attached patch Mac OS X Patch v3 (obsolete) — — Splinter Review
Decided to split this off as a separate bug from https://bugzilla.mozilla.org/show_bug.cgi?id=565552 Mac OS X: Latest patch updated in response to fryn's review. Also added a class for the text field in minimal (quick find) mode to appropriately style it given that the previous/next buttons are hidden. This required the addition of some helper hasClass/addClass/removeClass functions. Not sure if there's a better way to do this. Windows: Ready for development; waiting on mockup.
Attachment #645077 - Flags: review?(fryn)
(In reply to Frank Yan (:fryn) from comment #97) > Comment on attachment 635543 [details] [diff] [review] > Patch for find bar on Mac OS X (tested on 10.7 Lion) > > Review of attachment 635543 [details] [diff] [review]: > ----------------------------------------------------------------- > > Why were the "#ifdef MOZ_WIDGET_GTK" lines removed? > Do you know why they were there before? I don't, but it appears that what they were doing is conditionally making the button order (previous, next) instead of (next, previous). I assumed it to obey system standards. One of the changes for the redesign is to always use (previous, next), so I just removed the conditional check. If this doesn't make sense, I'll change it. > > @@ +914,5 @@ > > <parameter name="aHighlight"/> > > <parameter name="aWord"/> > > <parameter name="aWindow"/> > > <body><![CDATA[ > > + aHighlight = true; > > I don't think this is right... Yep, accidentally left that in from debugging. :P > > ::: toolkit/locales/en-US/chrome/global/findbar.dtd > @@ +17,3 @@ > > <!ENTITY highlight.accesskey "a"> > > <!ENTITY highlight.tooltiptext "Highlight all occurrences of the phrase"> > > +<!ENTITY caseSensitiveCheckbox.label "Match Case"> > > I guess these entity names don't really need to be updated. It's perhaps more trouble than it's worth, but it seems like Firefox's other string names on toolbar buttons and menus are usually title case. Stephen's mockup uses title case as well, FWIW. > > ::: toolkit/themes/pinstripe/global/findBar.css > @@ +63,5 @@ > > + padding: 2px 6px; > > +} > > + > > +.findbar-find-next:active, > > +.findbar-find-previous:active, > > Why are the :active states of next/previous matching the checked states of > highlight/case but different from the :active states of highlight/case? This > is inconsistent. Fixed this. I guess I need Stephen's input on whether the active states should look distinct from the toggled states.
Attached image screenshot of patch v3 (obsolete) —
Here's a screenshot of the patch. It looks like there should be more padding on the right (-end) side of the next button. Also, this styling is broken in RTL mode. Please test by enabling the Force RTL extension and then opening a new window. For example, you should be using -moz-*-end not *-right when it should be on the left for RTL mode. (In reply to Chris Lee (:cleer) from comment #1) > > > +<!ENTITY caseSensitiveCheckbox.label "Match Case"> > > > > I guess these entity names don't really need to be updated. > It's perhaps more trouble than it's worth, but it seems like Firefox's other > string names on toolbar buttons and menus are usually title case. Stephen's > mockup uses title case as well, FWIW. I meant that the _name_ doesn't need to be updated, even though we usually do that when we change the _string_, so localizers know that they should update the string in other languages too. In this case, a change in the English capitalization doesn't seem to warrant that. In short, what you did here is usually not okay, but in this case probably okay. > > > +.findbar-find-next:active, > > > +.findbar-find-previous:active, > > > > Why are the :active states of next/previous matching the checked states of > > highlight/case but different from the :active states of highlight/case? This > > is inconsistent. > Fixed this. I guess I need Stephen's input on whether the active states > should look distinct from the toggled states. Yeah, let's ask Stephen. I think that they should be different, but the :hover:active styling and the checked styling should each be consistent across all the buttons. Also, :active should be :hover:active. :active means that the mousedown occurred on the element, but the cursor could be anywhere. Mouseup while :active is in effect but the cursor is outside does not trigger a button's command.
Comment on attachment 645077 [details] [diff] [review] Mac OS X Patch v3 I'm gonna try to do less-than-two-business-day review cycles. :)
Attachment #645077 - Flags: review?(fryn) → review-
Attached patch WIP Mac OS X patch v4 (obsolete) — — Splinter Review
Fixes RTL styling and a bug with how the quick find class was being added.
Attachment #645077 - Attachment is obsolete: true
Attachment #645137 - Flags: review?(fryn)
Attached patch Mac OS X Patch v5 (obsolete) — — Splinter Review
This version uses global textbox styling for the focused state, as approved by Stephen. He also mentioned that we could possibly make that styling closer to the Mac system styling, but that can be a separate bug.
Attachment #645137 - Attachment is obsolete: true
Attachment #645137 - Flags: review?(fryn)
Attachment #645891 - Flags: review?(fryn)
Attachment #645891 - Attachment is patch: true
Status: NEW → ASSIGNED
Comment on attachment 645891 [details] [diff] [review] Mac OS X Patch v5 Review of attachment 645891 [details] [diff] [review]: ----------------------------------------------------------------- Almost there! I think it'll be an r+ after this round. :) ::: toolkit/content/widgets/findbar.xml @@ +293,5 @@ > + var regexp = new RegExp('(\\s|^)'+aClass+'(\\s|$)'); > + aElement.className=aElement.className.replace(regexp,' '); > + } > + ]]></body> > + </method> Use this instead: https://developer.mozilla.org/en/DOM/element.classList ::: toolkit/themes/pinstripe/global/findBar.css @@ +117,1 @@ > -moz-padding-start: 19px; I think the icon is supposed to be on the right in RTL mode (as it is in the search box), instead of having the large amount of blank padding on the right and the icon on the left.
Attachment #645891 - Flags: review?(fryn) → review-
Attached patch Mac OS X Patch v6 (obsolete) — — Splinter Review
Should fix both issues.
Attachment #645891 - Attachment is obsolete: true
Attachment #647252 - Flags: review?(fryn)
Attached patch Mac OS X Patch v6.1 (obsolete) — — Splinter Review
Oops, an image somehow went missing in the previous patch.
Attachment #647252 - Attachment is obsolete: true
Attachment #647252 - Flags: review?(fryn)
Attachment #647258 - Flags: review?(fryn)
Attached patch Mac OS X Patch v6.2 (obsolete) — — Splinter Review
Double oops, I uploaded the wrong patch...
Attachment #647258 - Attachment is obsolete: true
Attachment #647258 - Flags: review?(fryn)
Attachment #647259 - Flags: review?(fryn)
Comment on attachment 647259 [details] [diff] [review] Mac OS X Patch v6.2 Review of attachment 647259 [details] [diff] [review]: ----------------------------------------------------------------- Just 1 thing left to fix, I think. I'll file a bug for the "Highlight All" issues that I explain below that existed independently of this bug. ::: toolkit/content/widgets/findbar.xml @@ +228,5 @@ > + accesskey="&caseSensitiveCheckbox.accesskey;" > + tooltiptext="Search with case sensitivity" > + oncommand="_setCaseSensitivity(this.checked);" > + type="checkbox" > + disabled="true" See comment below. @@ +1461,5 @@ > <body><![CDATA[ > this.getElement("find-next").disabled = > this.getElement("find-previous").disabled = > + this.getElement("highlight").disabled = > + this.getElement("find-case-sensitive").disabled = !aEnable; Actually, "Match Case" shouldn't be disabled when the textbox is blank. For example, if you type something into the textbox, select "Match Case" and clear the textbox, it's quite bizarre for the "Match Case" button to reveal suddenly that it is actually enabled upon the next input into the textbox. "Highlight All" should follow this too, but let's leave that alone for now, since "Highlight All" has several other problems that we need to fix in separate bugs. For example, it doesn't properly restore state when switching between tabs, and it often displays "Phrase not found" incorrectly when clicking it. :/
Attachment #647259 - Flags: review?(fryn) → review-
Attached patch Mac OS X Patch v7 (obsolete) — — Splinter Review
Fixed the issue mentioned above.
Attachment #647259 - Attachment is obsolete: true
Attachment #650335 - Flags: review?(fryn)
Comment on attachment 650335 [details] [diff] [review] Mac OS X Patch v7 \o/
Attachment #650335 - Flags: review?(fryn) → review+
Comment on attachment 650335 [details] [diff] [review] Mac OS X Patch v7 Note that as a toolkit patch, this needs review from a toolkit peer. I skimmed over this quickly and noticed that you're adding find-arrows.png in browser/ while using it in toolkit/.
Attachment #650335 - Flags: review-
Any suggestions for who would be an appropriate "toolkit peer"?
(In reply to Chris Lee (:cleer) from comment #15) > Any suggestions for who would be an appropriate "toolkit peer"? https://wiki.mozilla.org/Modules/Toolkit - I'd suggest Asaf Romano, Neil Rashbrook or me.
Attached patch Mac OS X Patch v8 (obsolete) — — Splinter Review
Moved the image into toolkit/ Please let me know if you're too busy to check over soon, and if so, I'll request from one of those other people. Thanks!
Attachment #650335 - Attachment is obsolete: true
Attachment #651877 - Flags: review?(dao)
Comment on attachment 651877 [details] [diff] [review] Mac OS X Patch v8 >-<!ENTITY highlight.label "Highlight all"> >+<!ENTITY highlight.label "Highlight All"> > <!ENTITY highlight.accesskey "a"> > <!ENTITY highlight.tooltiptext "Highlight all occurrences of the phrase"> >-<!ENTITY caseSensitiveCheckbox.label "Match case"> >+<!ENTITY caseSensitiveCheckbox.label "Match Case"> >-NormalFindLabel=Find: >-FastFindLabel=Quick Find: >-FastFindLinksLabel=Quick Find (links only): >+NormalFindLabel=Find in page >+FastFindLabel=Quick Find >+FastFindLinksLabel=Quick Find (links only) These strings need new names for localizers to pick up the changes. > findbar { >- background: @scopeBarBackground@; >- border-top: @scopeBarSeparatorBorder@; >+ background: -moz-linear-gradient(#ededed, #d9d9d9); Please use the unprefixed linear-gradient syntax. > label.findbar-find-fast { >- margin: 1px 3px 0 !important; >- color: @scopeBarTitleColor@; >- font-weight: bold; >- text-shadow: @loweredShadow@; >+ color: rgba(0,0,0,.5); >+ margin: 1px 1px 0 !important; >+ -moz-margin-start: 12px !important; Why is !important needed here? >+.findbar-highlight, >+.findbar-case-sensitive { > -moz-appearance: none; > border-radius: 10000px; >- border: @roundButtonBorder@; >- background: @roundButtonBackground@; >- box-shadow: @roundButtonShadow@; >- color: buttontext; >+ border: 1px solid; >+ border-color: #8d8d8d #9b9b9b #9f9f9f; >+ background: -moz-linear-gradient(#f7f7f7, #ebebeb); >+ box-shadow: 0 1px rgba(255,255,255,.2), inset 0 1px 1px rgba(255,255,255,.2); >+ color: #333 !important; >+ margin: 0 4px; >+ text-shadow: 0 1px rgba(255,255,255,.25); >+ padding: 2px 6px; >+} Why are you moving away from using the shared values (roundButtonBorder etc.)? > .findbar-textbox { >+ overflow: visible; What is this doing here? Do you intend to land this without winstripe & gnomestripe changes?
Attachment #651877 - Flags: review?(dao) → review-
Attached patch Mac OS X Patch v9 (obsolete) — — Splinter Review
Thanks for the review. (In reply to Dão Gottwald [:dao] from comment #18) > Comment on attachment 651877 [details] [diff] [review] > Mac OS X Patch v8 > These strings need new names for localizers to pick up the changes. I updated the names for the second set. I agree with Frank that the first set is close enough (capitalization changes) to remain the same in other languages. > Please use the unprefixed linear-gradient syntax. Done. > > label.findbar-find-fast { > Why is !important needed here? Because `label`s have a margin by default. > >+.findbar-highlight, > >+.findbar-case-sensitive { > Why are you moving away from using the shared values (roundButtonBorder > etc.)? Because the shared values don't accomplish what we want to here. It's a different style. I assume that other elements would gradually be converted to this style in the Australis redesign and could then use a shared value, but that's a bit farther off. You could make the argument that we should use the shared values for now and change them later, but if so, take that up with Stephen, since he's the one who decided on this when I asked him a while ago. > > .findbar-textbox { > >+ overflow: visible; > What is this doing here? > Good catch. Fixed. > Do you intend to land this without winstripe & gnomestripe changes? No. But I most likely won't be working on those, so I hope that someone does and this doesn't just sit here forever. And I hope that this Mac patch gets sufficiently approved before I leave in two weeks.
Attachment #651877 - Attachment is obsolete: true
Attachment #651942 - Flags: review?(dao)
Attached patch Mac OS X Patch v9.1 (obsolete) — — Splinter Review
Forgot an image.
Attachment #651942 - Attachment is obsolete: true
Attachment #651942 - Flags: review?(dao)
Attachment #652244 - Flags: review?(dao)
Comment on attachment 652244 [details] [diff] [review] Mac OS X Patch v9.1 (In reply to Chris Lee (:cleer) from comment #19) > > These strings need new names for localizers to pick up the changes. > I updated the names for the second set. I agree with Frank that the first > set is close enough (capitalization changes) to remain the same in other > languages. The context is different though, changing from a checkbox to a button, which I think may be relevant to some localizers. The name "caseSensitiveCheckbox" is also kind of confusing. > > > label.findbar-find-fast { > > Why is !important needed here? > Because `label`s have a margin by default. Right, but I'm interested in why you need !important to set a custom margin here. > > >+.findbar-highlight, > > >+.findbar-case-sensitive { > > Why are you moving away from using the shared values (roundButtonBorder > > etc.)? > Because the shared values don't accomplish what we want to here. It's a > different style. I assume that other elements would gradually be converted > to this style in the Australis redesign and could then use a shared value, > but that's a bit farther off. You could make the argument that we should use > the shared values for now and change them later, but if so, take that up > with Stephen, since he's the one who decided on this when I asked him a > while ago. Hardcoding stuff in order to unify it later seems backwards and will generate significant amounts of followup work. The whole point of sharing is the ability to make bulk changes. So just making that bulk change now would be the normal way to approach this. What's unclear to me: Would updating the shared values be a problem for places where they're used today? If so, maybe the solution is to introduce a new set of shared values.
Attachment #652244 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #21) > Comment on attachment 652244 [details] [diff] [review] > Mac OS X Patch v9.1 > > (In reply to Chris Lee (:cleer) from comment #19) > > > > label.findbar-find-fast { > > > Why is !important needed here? > > Because `label`s have a margin by default. > > Right, but I'm interested in why you need !important to set a custom margin > here. I really don't know why. I've checked the styles on the element and it's just the standard `label` styling in global.css that seems to have priority over `label.findbar-find-fast` normally, and I have no idea why. > > > >+.findbar-highlight, > > > >+.findbar-case-sensitive { > > > Why are you moving away from using the shared values (roundButtonBorder > > > etc.)? > > Because the shared values don't accomplish what we want to here. It's a > > different style. I assume that other elements would gradually be converted > > to this style in the Australis redesign and could then use a shared value, > > but that's a bit farther off. You could make the argument that we should use > > the shared values for now and change them later, but if so, take that up > > with Stephen, since he's the one who decided on this when I asked him a > > while ago. > > Hardcoding stuff in order to unify it later seems backwards and will > generate significant amounts of followup work. The whole point of sharing is > the ability to make bulk changes. So just making that bulk change now would > be the normal way to approach this. What's unclear to me: Would updating the > shared values be a problem for places where they're used today? If so, maybe > the solution is to introduce a new set of shared values. I'll ask Stephen about this.
Attached patch Mac OS X Patch v10 (obsolete) — — Splinter Review
- Edited entity names. - Using shared values for button/toolbar styles. Might file a separate bug to update these styles.
Attachment #652244 - Attachment is obsolete: true
Attachment #653525 - Flags: review?(dao)
Attached patch Mac OS X Patch v11 (obsolete) — — Splinter Review
Managed to cut down the icon image in half after realizing that the disabled state is just the same icon at half opacity. I'm starting work on the Windows patch now.
Attachment #653525 - Attachment is obsolete: true
Attachment #653525 - Flags: review?(dao)
Attachment #653957 - Flags: review?(dao)
Attached patch Windows and Mac OS X Patch v1 (obsolete) — — Splinter Review
This version adds Windows styling. All the Mac parts should be the same except for one thing: the find bar is now inserted into the chrome at a different point. This was to make fit within the border that runs around Windows windows. This also has the side effect of making the find bar not cover the area covered by a sidebar (unlike the current behavior), which I also think looks better and makes more sense hierarchically.
Attachment #653957 - Attachment is obsolete: true
Attachment #653957 - Flags: review?(dao)
Attachment #654483 - Flags: review?(dao)
Assignee: chlee → nobody
Blocks: 466740
Status: ASSIGNED → NEW
No longer blocks: 466740
This should be one of top Australis priorities now. Current patch could be even launched on UX at least on OSX
Can the .slideIn be changed to .slideOut? The current patch doesn't allow backwards compatibility for themes, as the margin-bottom:-1em and opacity:0 cannot be set in FF's before this patch. By using a slideOut to specify the moving direction one can leave the normal style rule for findbar as-is: findbar.slideOut{ margin-bottom:-1em; opacity:0}
(In reply to Alfred Kayser from comment #27) Oops, wrong bug
Nice to have for Australis part one.
nice! what's needed still to land this? If this is known, I'd be happy to take this bug.
(In reply to Mike de Boer [:mikedeboer] from comment #30) > nice! what's needed still to land this? If this is known, I'd be happy to > take this bug. I think new mockups are needed. Shorlander may have new ones (especially for Linux/Windows). The visual redesign is a first step : dependencies of bug 565552 also show the work to be done and this outdated wikipage could also be useful : https://wiki.mozilla.org/Improve_find-in-page
Comment on attachment 654483 [details] [diff] [review] Windows and Mac OS X Patch v1 I don't know if we need new mockups, but we'd certainly need a patch that works on all platforms.
Attachment #654483 - Flags: review?(dao)
This was intern's work last summer, but Chris Lee sadly hasn't had the time to finish it. AFAIR he was waiting on answers and mockups from the UX team.
First I un-bitrotted the patch of Chris Lee, fixed it up on Windows first and after that on OSX. OSX needed most of the work, but it ended up looking quite ok. One thing is still nagging me: the searchbox' top border is aligned 1px lower than the top border of the previous/ next buttons. Does anyone know how to properly fix that?
Attachment #654483 - Attachment is obsolete: true
Attachment #739628 - Flags: ui-review?(fyan)
Comment on attachment 739628 [details] [diff] [review] Change the findBar appearance to be on top and improve its UX Review of attachment 739628 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this. :) I haven't tested it yet, but I will later today. Here is some initial code feedback: ::: browser/base/content/browser.js @@ +51,5 @@ > let findbar = document.createElementNS(XULNS, "findbar"); > findbar.id = "FindToolbar"; > > let browserBottomBox = document.getElementById("browser-bottombox"); > browserBottomBox.insertBefore(findbar, browserBottomBox.firstChild); Remove these two lines, since the two lines that you added should replace these. ::: toolkit/content/widgets/findbar.xml @@ +182,5 @@ > <xul:toolbarbutton anonid="find-closebutton" > class="findbar-closebutton" > tooltiptext="&findCloseButton.tooltip;" > oncommand="close();"/> > +#endif This closebutton duplication is no good. You can actually reorder the layout of a box's children in legacy flexbox by setting -moz-box-ordinal-group: https://developer.mozilla.org/en-US/docs/CSS/box-ordinal-group See how we're already using it: https://mxr.mozilla.org/mozilla-central/search?string=-moz-box-ordinal-group @@ +208,5 @@ > + control="findbar-textbox" > + class="findbar-find-fast findbar-find-status"> > + <!-- Do not use value, first child is used because it provides a11y with text change events --> > + </xul:description> > + <xul:spacer flex="1" /> Nit: remove space before the slash. ::: toolkit/themes/osx/global/findBar.css @@ +132,5 @@ > > .findbar-textbox { > -moz-appearance: none; > + border: @roundButtonBorder@; > + border-radius: 10000px 0 0 10000px; Using 50% is now the accepted practice instead of 10000px. Likewise for the other border-radius rules below.
Thank you for continuing work on this patch, Mike. I kind of gave up on it, to be honest, since the review process was taking an unreasonable/indefinite amount of time (and due to a lack of time/motivation to work on it post-internship), but I'd love to see the find bar cleaned up.
Comment on attachment 739628 [details] [diff] [review] Change the findBar appearance to be on top and improve its UX Review of attachment 739628 [details] [diff] [review]: ----------------------------------------------------------------- The opening & closing transitions are really rough (at least on OS X) in their easing and the shifting of the web content. This is a clear regression from what we're currently shipping. We need to address that before landing this. ::: toolkit/locales/en-US/chrome/global/findbar.properties @@ +7,5 @@ > WrappedToTop=Reached end of page, continued from top > WrappedToBottom=Reached top of page, continued from bottom > +NormalFindLabel2=Find in page > +FastFindLabel2=Quick Find > +FastFindLinksLabel2=Quick Find (links only) Could we just rename these to NormalFind, FastFind, and FastFindLinks, respectively? ::: toolkit/themes/osx/global/findBar.css @@ +141,1 @@ > -moz-padding-start: 19px; Could you add `positive: relative;` here, so the box-shadow for the textbox appears above the previous/next buttons? This shadow still looks a bit wonky. I don't have time to investigate how to make it match the mockup more closely.
Attachment #739628 - Flags: ui-review?(fyan) → ui-review-
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Hardware: x86 → All
Frank: Thanks for reviewing this! I'll take a look at the styling...
Could we have a screenshot of what it currently looks like on various platforms ?
* using -moz-box-ordinal-group for the close button now (sweet feature!) * impl. Franks' suggestions, basically * improved show/ hide perf by tuning down the anim duration. Since it's on top now, we don't really need an anim anyway. As a reference: the perf regression is caused by the find bar pushing the page's content down, causing many reflows/ repaints because it's animated. Chrome solved this by making their search bar behave as an interstitial, but that's much uglier IMHO. Search bar on bottom didn't have this issue, because it only increased the scroll-height, so no reflows were necessary. Frank: I honestly tried using '50%' for border-radius, but for the life of me, I couldn't get it to work... It presented me with the oddest bunch of radii I've seen yet.
Attachment #739628 - Attachment is obsolete: true
Attachment #741308 - Flags: ui-review?(fyan)
Attached image screenshot OSX (obsolete) —
Attachment #645092 - Attachment is obsolete: true
Looks very nice. I don't know which styling you used for the buttons, but it could be nice to apply Australis one (especially on Windows). Only question left is : how will this look on Linux ?
(In reply to Mike de Boer [:mikedeboer] from comment #40) > Created attachment 741308 [details] [diff] [review] > Change the findBar appearance to be on top and improve its UX > > * using -moz-box-ordinal-group for the close button now (sweet feature!) > * impl. Franks' suggestions, basically > * improved show/ hide perf by tuning down the anim duration. Since it's on > top now, we don't really need an anim anyway. As a reference: the perf > regression is caused by the find bar pushing the page's content down, > causing many reflows/ repaints because it's animated. Chrome solved this by > making their search bar behave as an interstitial, but that's much uglier > IMHO. Search bar on bottom didn't have this issue, because it only increased > the scroll-height, so no reflows were necessary FWIW Opera animates it even with the bar pushing the content down.
(In reply to Mike de Boer [:mikedeboer] from comment #40) > * improved show/ hide perf by tuning down the anim duration. Since it's on > top now, we don't really need an anim anyway. As a reference: the perf > regression is caused by the find bar pushing the page's content down, > causing many reflows/ repaints because it's animated. Chrome solved this by > making their search bar behave as an interstitial, but that's much uglier > IMHO. Search bar on bottom didn't have this issue, because it only increased > the scroll-height, so no reflows were necessary. Maybe we should revisit whether moving the bar to the top is a good tradeoff. This change also seems pretty much unrelated to the visual appearance improvements.
(In reply to Dão Gottwald [:dao] from comment #45) > (In reply to Mike de Boer [:mikedeboer] from comment #40) > > * improved show/ hide perf by tuning down the anim duration. Since it's on > > top now, we don't really need an anim anyway. As a reference: the perf > > regression is caused by the find bar pushing the page's content down, > > causing many reflows/ repaints because it's animated. Chrome solved this by > > making their search bar behave as an interstitial, but that's much uglier > > IMHO. Search bar on bottom didn't have this issue, because it only increased > > the scroll-height, so no reflows were necessary. > > Maybe we should revisit whether moving the bar to the top is a good > tradeoff. This change also seems pretty much unrelated to the visual > appearance improvements. Moving it to the top is the main point of the visual redesign. It has been wanted for ages.
(In reply to Guillaume C. [:ge3k0s] from comment #46) > Moving it to the top is the main point of the visual redesign. Is it? So how exactly are the many visual design changes related to the bar's position? > It has been wanted for ages. I'm not sure what you're trying to tell me. Even if it was wanted for ages, that wouldn't mean that we couldn't revisit whether it's a good tradeoff.
(In reply to Dão Gottwald [:dao] from comment #47) > (In reply to Guillaume C. [:ge3k0s] from comment #46) > > Moving it to the top is the main point of the visual redesign. > > Is it? So how exactly are the many visual design changes related to the > bar's position? It's true that there are two aspects of the visual redesign : styling and moving the bar to the top. It hasn't been specified though that this bug would cover only the buttons or only the bar's position. It's a complete redesign that cover both. > > It has been wanted for ages. > > I'm not sure what you're trying to tell me. Even if it was wanted for ages, > that wouldn't mean that we couldn't revisit whether it's a good tradeoff. It has also been discussed for a long time (especially in bug 565552). You can also find why it would be better to having it on top according to the UX team here : https://wiki.mozilla.org/Improve_find-in-page One of many argument : Zhenshuo Fang (:fang) - Firefox UX Team from comment #27) > I did talk to Limi about this. The reason of placing the find bar on top is > that the user always read from top to bottom. After typing the keyword on > find box on top of the page, they can move their focus down to look at the > result. This flow feels more natural. I don't say that we shouldn't discuss more, even if the answer seems obvious to me.
I was saying "tradeoff", meaning that there are pros and cons. That's different from neglecting pros, which I didn't. https://wiki.mozilla.org/Improve_find-in-page clearly says nothing about content being pushed down and how this affects the animation.
Blocks: 257061
(In reply to Mike de Boer [:mikedeboer] from comment #40) > * using -moz-box-ordinal-group for the close button now (sweet feature!) Yeah, it's pretty cool. :) > * improved show/ hide perf by tuning down the anim duration. Since it's on > top now, we don't really need an anim anyway. Good point. Let's move the find bar, removing the opening/closing animation, and spin off a followup bug to get the animation right. Getting the find bar in the right location is more important than the animation. (:fang, :limi, and shorlander can corroborate this point.) > Frank: I honestly tried using '50%' for border-radius, but for the life of > me, I couldn't get it to work... It presented me with the oddest bunch of > radii I've seen yet. Huh. I'll look into it. (In reply to Dão Gottwald [:dao] from comment #49) > https://wiki.mozilla.org/Improve_find-in-page clearly says nothing about > content being pushed down and how this affects the animation. Wiki page is just out-of-date.
(In reply to Frank Yan (:fryn) from comment #50) > > Since it's on top now, we don't really need an anim anyway. > > Good point. Let's move the find bar, removing the opening/closing animation, > and spin off a followup bug to get the animation right. Getting the find bar > in the right location is more important than the animation. To be clear, the animation is still important, but you're right that it's less important than the find bar's location. We should make sure that we fix the followup bug for the animation and not leave it hanging there. It just doesn't need to land at the same time.
Comment on attachment 741308 [details] [diff] [review] Change the findBar appearance to be on top and improve its UX Review of attachment 741308 [details] [diff] [review]: ----------------------------------------------------------------- I just realized the border-radius: 50% doesn't have the desired appearance in this case, because our containers are square, and we want circular corners, not elliptical ones. The box-shadow on OS X isn't very pretty still, but that's a problem that should be solved in a general "Improve OS X focus box-shadows"-type bug.
Attachment #741308 - Flags: ui-review?(fyan)
Attachment #741308 - Flags: ui-review+
Attachment #741308 - Flags: feedback+
(In reply to Dão Gottwald [:dao] from comment #49) > https://wiki.mozilla.org/Improve_find-in-page clearly says nothing about > content being pushed down and how this affects the animation. It seems pretty settled that the bar should be at the top (apart from anything else, most web site search functions are located in the header, and usually on the right in LTR text environments), but perhaps the bar doesn't need to move the content. If it comes down, simply overlaying the page without pushing anything down, there's content that it obscures, but if it also allows scrolling the page to see that content, is that a problem? Technically, that might take the form of moving the top of the viewport down with the find bar's bottom edge: if the page is at the top when the user invokes find, the page then seems to have been scrolled by the size of the find bar. To the user, there's no visual change, but moving to the top match might scroll the top of the page into view.
Attachment #741308 - Flags: review?(dao)
Comment on attachment 741308 [details] [diff] [review] Change the findBar appearance to be on top and improve its UX >--- a/toolkit/themes/windows/global/findBar.css >+++ b/toolkit/themes/windows/global/findBar.css >+findbar { >+ padding: 4px 8px; >+ background: -moz-linear-gradient(#ebf1f5, #f5f8fa); >+ border-bottom: 1px solid #cad9e5; These hardcoded colors seem to assume that something like the Windows 7 default theme is in use, which is of course not going to be the case for all users. Before this patch, the findbar used a semi-transparent gradient that would pick up whatever toolbar color was underneath it: >-findbar { >- padding-top: 1px; >- background-image: linear-gradient(rgba(0,0,0,.15) 1px, rgba(255,255,255,.15) 1px); >+.findbar-textbox { >+ -moz-appearance: none; >+ border: 1px solid rgba(23,50,77,.3); >+ border-radius: 2px 0 0 2px; >+ margin: 0; >+ padding: 1px 5px; >+ width: 210px; > } Please use an 'em' value for the width. This patch still lacks styling on Linux. Most visibly, the bar has a top border but no bottom border, which is backwards when its moved to the top. I'd actually prefer if you spun the position change off to a separate bug, such that it's easier to track and to back out in case we run into issues. This would make the above point moot for now, i.e. you wouldn't have to worry about it in this bug. Note that a bottom-located findbar needs to be styled properly either way, as long as that remains a supported configuration. We use it in the view source window, for instance.
Attachment #741308 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #54) > Comment on attachment 741308 [details] [diff] [review] > Change the findBar appearance to be on top and improve its UX > > >--- a/toolkit/themes/windows/global/findBar.css > >+++ b/toolkit/themes/windows/global/findBar.css > > >+findbar { > >+ padding: 4px 8px; > >+ background: -moz-linear-gradient(#ebf1f5, #f5f8fa); > >+ border-bottom: 1px solid #cad9e5; > > These hardcoded colors seem to assume that something like the Windows 7 > default theme is in use, which is of course not going to be the case for all > users. Before this patch, the findbar used a semi-transparent gradient that > would pick up whatever toolbar color was underneath it: This styling could still be applied from Vista to Win 8. On XP another styling could be needed indeed. > I'd actually prefer if you spun the position change off to a separate bug, > such that it's easier to track and to back out in case we run into issues. Could use bug 254687.
(In reply to Dão Gottwald [:dao] from comment #54) > Note that a bottom-located findbar needs to be > styled properly either way, as long as that remains a supported > configuration. We use it in the view source window, for instance. I don't see why the find bar couldn't be on top everywhere (in view source too).
(In reply to Guillaume C. [:ge3k0s] from comment #56) > (In reply to Dão Gottwald [:dao] from comment #54) > > Note that a bottom-located findbar needs to be > > styled properly either way, as long as that remains a supported > > configuration. We use it in the view source window, for instance. > > I don't see why the find bar couldn't be on top everywhere (in view source > too). Because that's beyond this bug's scope. View source would just be the easiest consumer to update, there's also Thunderbird, SeaMonkey, random other applications and add-ons that might use toolkit's findbar widget. Also, the reasons for moving it to the top might not apply to other consumers quite as well as to Firefox.
(In reply to Dão Gottwald [:dao] from comment #57) > (In reply to Guillaume C. [:ge3k0s] from comment #56) > > (In reply to Dão Gottwald [:dao] from comment #54) > > > Note that a bottom-located findbar needs to be > > > styled properly either way, as long as that remains a supported > > > configuration. We use it in the view source window, for instance. > > > > I don't see why the find bar couldn't be on top everywhere (in view source > > too). > > Because that's beyond this bug's scope. View source would just be the > easiest consumer to update, there's also Thunderbird, SeaMonkey, random > other applications and add-ons that might use toolkit's findbar widget. > Also, the reasons for moving it to the top might not apply to other > consumers quite as well as to Firefox. I agree concerning all other products, but the find bar's location should be the same everywhere in Firefox mainly for consistency.
Mike, let's just keep the border-top in findbar.css and override it specifically for the Firefox "find in page" bar. I don't want to widen the scope of this bug and get stuck in more edge cases and arguments. The most important thing is to improve the most prominent <findbar>, and that is Firefox's "find in page" bar. (In reply to Guillaume C. [:ge3k0s] from comment #58) Guillaume, your comments aren't especially helpful here. We have all information we need to do the design and implementation. More information is distracting and dilutes the message.
Fixed/ implemented comment from Dao and added the required changes for Linux. A screenshot is coming up! I also removed the feature that moves the findbar to the top from this patch. I will attach a separate patch for that to this bug.
Attachment #741308 - Attachment is obsolete: true
Attachment #746306 - Flags: ui-review+
Attachment #746306 - Flags: review?(dao)
Attached image Screenshot Linux (Ubuntu 13.04) (obsolete) —
Forgots a few things... carrying over ui-r+=fryn
Attachment #746306 - Attachment is obsolete: true
Attachment #746306 - Flags: review?(dao)
Attachment #746324 - Flags: ui-review+
Attachment #746324 - Flags: review?(dao)
Attached patch Part 2: Move findbar to the top (obsolete) — — Splinter Review
The purpose of this patch is to move the findbar widget to the top of the window, in both the browser(-tab) and view-source window. It should be applied AFTER attachment 746324 [details] [diff] [review]
Attachment #746369 - Flags: review?(fyan)
Attachment #746324 - Attachment description: Change the findBar appearance and improve its UX → Part 1: Change the findBar appearance and improve its UX
Frank: I think I also fixed the performance regression that was there first when I moved the findbar to the top. Can you confirm that?
Comment on attachment 746369 [details] [diff] [review] Part 2: Move findbar to the top Please move this to a separate bug (that you can make dependent on this one). Bugs are cheap.
Attachment #746369 - Attachment is obsolete: true
Attachment #746369 - Flags: review?(fyan)
(In reply to Mike de Boer [:mikedeboer] from comment #63) > Created attachment 746369 [details] [diff] [review] > Part 2: Move findbar to the top > > The purpose of this patch is to move the findbar widget to the top of the > window, in both the browser(-tab) and view-source window. > It should be applied AFTER attachment 746324 [details] [diff] [review] (In reply to Dão Gottwald [:dao] from comment #65) Mike, I agree with you on the original purpose of the bug, but if Dão would prefer to review them independently, let's do that. This is a situation where both pieces are improvements even without the other, so it's okay if they land at different times. Thanks for working on this. :)
Blocks: 869543
Comment on attachment 746324 [details] [diff] [review] Part 1: Change the findBar appearance and improve its UX Review of attachment 746324 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/themes/osx/global/findBar.css @@ +10,5 @@ > border-top: @scopeBarSeparatorBorder@; > min-width: 1px; > padding: 4px 2px; > transition-property: margin-bottom, opacity, visibility; > + transition-duration: 150ms, 150ms, 0s Nit: you accidentally removed the semicolon here.
Comment on attachment 746324 [details] [diff] [review] Part 1: Change the findBar appearance and improve its UX >+NormalFind=Find in page >+FastFind=Quick Find >+FastFindLinks=Quick Find (links only) Looks like you capitalized "Find" in "Quick Find" as if it was a proper name. I think it should be lowercase like "Find in page" is. > <!ENTITY findCloseButton.tooltip "Close Find bar"> Could also fix the sentence case here while you're at it. >-<!ENTITY highlight.label "Highlight all"> >+<!ENTITY highlight2.label "Highlight All"> > <!ENTITY highlight.accesskey "a"> > <!ENTITY highlight.tooltiptext "Highlight all occurrences of the phrase">. nit: please rename these to highlightAll.* >--- a/toolkit/themes/linux/global/findBar.css >+++ b/toolkit/themes/linux/global/findBar.css > findbar { >+ padding: 4px 8px; > border-top: 2px solid; > -moz-border-top-colors: ThreeDShadow ThreeDHighlight; >- padding-bottom: 1px; >+ padding-bottom: 2px; > min-width: 1px; > transition-property: margin-bottom, opacity, visibility; > transition-duration: 150ms, 150ms, 0s; > transition-timing-function: ease-in-out, ease-in-out, linear; > } The vertical padding seems uneven. (Your screenshot shows this.) >+.findbar-closebutton { >+ -moz-margin-start: 4px; >+ border: none; >+ padding: 0; >+ list-style-image: url("chrome://global/skin/icons/close.png"); >+ -moz-appearance: none; >+ -moz-image-region: rect(0, 16px, 16px, 0); > } This is actually close.png inherited from toolkit/themes/windows/global/icons/close.png. It's not clear to me that we'd want to use exactly that image on Linux. In any case, this would be the only place using it so far, whereas we use moz-icon://stock/gtk-close?size=menu pretty much everywhere else. So this would be inconsistent; you should probably leave the close button alone for the time being. >+.findbar-textbox { >+ -moz-appearance: none; >+ border: 1px solid GrayText; >+ border-radius: 3px 0 0 3px; >+ box-shadow: 0 0 1px 0 GrayText inset; >+ margin: 0; >+ padding: 5px; >+ width: 14em; > } The border-radius is wrong for RTL locales. GrayText is supposed to be used as a text color. Can you use ThreeDShadow? >+.findbar-find-previous, >+.findbar-find-next { >+ -moz-margin-start: 0; >+ -moz-appearance: none; >+ background: -moz-linear-gradient(rgba(255,255,255,.9), rgba(255,255,255,.2)); >+ border: 1px solid GrayText; >+ border-radius: 2px; >+ box-shadow: 0 1px #fff inset; >+ list-style-image: url("chrome://global/skin/icons/find-arrows.png"); >+ padding: 9px; >+} ditto for the border color These buttons need to stretch vertically such that they scale with the textbox in case the system font size is larger than what you tested this with. You also need to re-implement some kind of a focus ring as these buttons can be tabbed to. >+.findbar-highlight, >+.findbar-case-sensitive { >+ -moz-appearance: none; >+ -moz-margin-start: 5px; >+ border: 1px solid transparent; >+ border-radius: 2px; >+ padding: 5px; >+} This feels rather alien on Linux and Windows. Toolbar buttons on Linux and Windows are supposed to change on hover. Can you restrict the custom styling to findbar-find-previous and findbar-find-next and keep the native toolbar button appearance for the other buttons? >+.findbar-find-status { >+ color: #6c7680; >+ margin: 0 !important; >+ -moz-margin-start: 12px !important; >+} I bet there are OS themes where #6c7680 is barely visible as a text color...
Attachment #746324 - Flags: review?(dao) → review-
Comment on attachment 746324 [details] [diff] [review] Part 1: Change the findBar appearance and improve its UX >+.findbar-find-previous > .toolbarbutton-text, >+.findbar-find-next > .toolbarbutton-text { >+ display: none; >+} This prevents the access keys from being exposed even though they still exist and work. That's not a good state to be in.
Dao: thanks for the super-thorough review! I'd love to meet up on IRC with you to effectively discuss how to move forward from here!
(In reply to Dão Gottwald [:dao] from comment #69) > > This prevents the access keys from being exposed even though they still > exist and work. That's not a good state to be in. Is there a way to keep the button styling this way and still have it accessible? The captions were removed intentionally (replaced by arrow graphics), so it would be nice to not having to change that back or ask UX for feedback. To have properly themed buttons on Linux (GTK, KDE, Metacity, etc), it would be nice to be able to modify the native styling; I hope to avoid using -moz-appearance: none and still have good looking button toggle state. With this I mean that the highlight and case buttons behave like checkboxes and should look like being pressed when toggled. When in the toggled state, the hover states should also change. Do you have any ideas regarding this? Thanks!
(In reply to Mike de Boer [:mikedeboer] from comment #71) > (In reply to Dão Gottwald [:dao] from comment #69) > > > > This prevents the access keys from being exposed even though they still > > exist and work. That's not a good state to be in. > > Is there a way to keep the button styling this way and still have it > accessible? The captions were removed intentionally (replaced by arrow > graphics), so it would be nice to not having to change that back or ask UX > for feedback. We could expose the label and accesskey in a tooltip.
(In reply to Jared Wein [:jaws] from comment #72) > (In reply to Mike de Boer [:mikedeboer] from comment #71) > > (In reply to Dão Gottwald [:dao] from comment #69) > > > > > > This prevents the access keys from being exposed even though they still > > > exist and work. That's not a good state to be in. > > > > Is there a way to keep the button styling this way and still have it > > accessible? The captions were removed intentionally (replaced by arrow > > graphics), so it would be nice to not having to change that back or ask UX > > for feedback. > > We could expose the label and accesskey in a tooltip. That wouldn't be keyboard-accessible though. :-(
I don't see why the tooltip needs to be keyboard accessible. The buttons themselves are keyboard accessible. The user can still tab to the buttons. The accesskey is just a handy shortcut, and isn't required to use the feature. Finding the accesskey so that the user can navigate quicker is a nice-to-have in this situation, so the tooltip should suffice.
(In reply to Jared Wein [:jaws] from comment #74) > I don't see why the tooltip needs to be keyboard accessible. I meant to say, I don't see why the accesskey needs to be visible without any extra interaction.
(In reply to Mike de Boer [:mikedeboer] from comment #71) > (In reply to Dão Gottwald [:dao] from comment #69) > > > > This prevents the access keys from being exposed even though they still > > exist and work. That's not a good state to be in. > > Is there a way to keep the button styling this way and still have it > accessible? The captions were removed intentionally (replaced by arrow > graphics), so it would be nice to not having to change that back or ask UX > for feedback. The buttons can be tabbed to, so you can just drop the access keys altogether. > To have properly themed buttons on Linux (GTK, KDE, Metacity, etc), it would > be nice to be able to modify the native styling; I hope to avoid using > -moz-appearance: none and still have good looking button toggle state. With > this I mean that the highlight and case buttons behave like checkboxes and > should look like being pressed when toggled. When in the toggled state, the > hover states should also change. Do you have any ideas regarding this? This should "just work" for <toolbarbutton type="checkbox"/> without you doing anything special.
Addressed comments from Dão & Jared for all platforms
Attachment #746324 - Attachment is obsolete: true
Attachment #753747 - Flags: ui-review+
Attachment #753747 - Flags: review?(dao)
Attached image Screenshot Linux (Ubuntu 13.04) (obsolete) —
improved styling on Linux desktop
Attachment #746318 - Attachment is obsolete: true
Comment on attachment 753747 [details] [diff] [review] Part 1: Change the findBar appearance and improve its UX >+.findbar-find-previous:not([disabled]):active, >+.findbar-find-next:not([disabled]):active, >+.findbar-highlight:not([disabled]):hover:active, >+.findbar-case-sensitive:not([disabled]):hover:active, >+.findbar-highlight:not([disabled])[checked="true"], >+.findbar-case-sensitive:not([disabled])[checked="true"] { >+ background: rgba(23,50,76,.2); >+ border: 1px solid ThreeDShadow; >+ box-shadow: 0 1px 2px rgba(10,31,51,.2) inset; >+} Why are findbar-highlight / findbar-case-sensitive part of this? >+.findbar-case-sensitive[disabled], >+.findbar-highlight[disabled] { >+ opacity: .5; >+} This shouldn't be necessary, the text should already be faded automatically based on the default toolbarbutton.css styling.
(In reply to Dão Gottwald [:dao] from comment #79) > > Why are findbar-highlight / findbar-case-sensitive part of this? That's because these buttons don't look OK with the default (native) styling of a toobarbutton on OSX. Please see the screenshot I will post next in comment 81.
FWIW the latest Australis OSX mockups use the Australis button styling : http://people.mozilla.com/~shorlander/files/australis-design-specs/images-osx/06-mainWindow-findBar-UI@2x.png
Addressed issues raised in comment 79 and will match the mockup as posted in comment 82 (except for bar on top).
Attachment #753747 - Attachment is obsolete: true
Attachment #753747 - Flags: review?(dao)
Attachment #754459 - Flags: review?(dao)
Attached image screenshot OSX —
Attachment #741311 - Attachment is obsolete: true
(In reply to Mike de Boer [:mikedeboer] from comment #84) > Created attachment 754463 [details] > screenshot OSX Looks good. The find bar looks a bit whiter on the mockup though. I also see a discrepancy between OSX and other platform on the screenshots. The magnifying glass in the search box should appear on every platform.
Comment on attachment 754459 [details] [diff] [review] Change the findBar appearance and improve its UX (In reply to Mike de Boer [:mikedeboer] from comment #80) > (In reply to Dão Gottwald [:dao] from comment #79) > > > > Why are findbar-highlight / findbar-case-sensitive part of this? > > That's because these buttons don't look OK with the default (native) styling > of a toobarbutton on OSX. But this doesn't make any sense on Linux. You can actually see the bogus inset shadow in attachment 753749 [details].
Attachment #754459 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #86) > But this doesn't make any sense on Linux. You can actually see the bogus > inset shadow in attachment 753749 [details]. You're right! I forgot to remove the additional styling on Linux. Followup coming soon...
(In reply to Guillaume C. [:ge3k0s] from comment #85) > > Looks good. The find bar looks a bit whiter on the mockup though. > > I also see a discrepancy between OSX and other platform on the screenshots. > The magnifying glass in the search box should appear on every platform. Guillaume, the toolbar background color is a toolkit-wide setting, which might be slightly different for Australis. Where did you find the info that the magnifying glass needs to be present on all platforms? AFAIK that's very OSX specific and would look alien on Linux and Windows for example.
(In reply to Mike de Boer [:mikedeboer] from comment #88) > (In reply to Guillaume C. [:ge3k0s] from comment #85) > > > > Looks good. The find bar looks a bit whiter on the mockup though. > > > > I also see a discrepancy between OSX and other platform on the screenshots. > > The magnifying glass in the search box should appear on every platform. > > Guillaume, the toolbar background color is a toolkit-wide setting, which > might be slightly different for Australis. Ok thanks for the info. ;-) > Where did you find the info that the magnifying glass needs to be present on > all platforms? AFAIK that's very OSX specific and would look alien on Linux > and Windows for example. On this outdated but most recent mockup for Windows AFAIK : http://people.mozilla.com/~shorlander/files/australis-design-specs/images/Australis-i01-DesignSpec-MainWindow-%28FindBar%29.jpg
Attachment #753749 - Attachment is obsolete: true
Attachment #754459 - Attachment is obsolete: true
Attachment #754508 - Flags: review?(dao)
Dão, I think we're pretty close with this one... don't you agree? It would be awesome if we could give it another push!
Personal note: I think the un-styled toggle buttons (as applied in the latest patch) look quite ugly on OSX, compared to the style as originally implemented by Chris Lee. It's technically more consistent with the implementation on other platforms, but still...
The best solution is to use the Australis styling on every platform (to be consistent with the toolbar icons styling).
(In reply to Mike de Boer [:mikedeboer] from comment #93) > Personal note: I think the un-styled toggle buttons (as applied in the > latest patch) look quite ugly on OSX, compared to the style as originally > implemented by Chris Lee. It's technically more consistent with the > implementation on other platforms, but still... This wasn't clear in comment 79, but I was referring specifically to Linux since you updated this in comment 78. I wasn't referring to OS X and you should probably revert that change indeed.
Reverted the OSX button styling
Attachment #754508 - Attachment is obsolete: true
Attachment #754508 - Flags: review?(dao)
Attachment #761159 - Flags: review?(dao)
Comment on attachment 761159 [details] [diff] [review] Change the findBar appearance and improve its UX (In reply to Mike de Boer [:mikedeboer] from comment #96) > Created attachment 761159 [details] [diff] [review] > Change the findBar appearance and improve its UX > > Reverted the OSX button styling *sigh* You didn't only revert the OS X styling. We're back to comment 86.
Attachment #761159 - Flags: review?(dao) → review-
Linux, behave!
Attachment #761159 - Attachment is obsolete: true
Attachment #762612 - Flags: review?(dao)
ok ok ok, that padding-bottom was kinda ambiguous...
Attachment #762612 - Attachment is obsolete: true
Attachment #762612 - Flags: review?(dao)
Attachment #762619 - Flags: review?(dao)
Comment on attachment 762619 [details] [diff] [review] Change the findBar appearance and improve its UX >+ <xul:toolbarbutton anonid="find-previous" >+ class="findbar-find-previous tabbable" >+ tooltip="&previous.label;" >+ tooltiptext="&previous.tooltip;" >+ label=" " >+ oncommand="onFindAgainCommand(true);" >+ disabled="true" >+ xbl:inherits="accesskey=findpreviousaccesskey"/> >+ <xul:toolbarbutton anonid="find-next" >+ class="findbar-find-next tabbable" >+ tooltip="&next.label;" >+ tooltiptext="&next.tooltip;" >+ label=" " >+ oncommand="onFindAgainCommand(false);" >+ disabled="true" >+ xbl:inherits="accesskey=findnextaccesskey"/> Looks like you're trying to set two tooltips for each button. You can only have one tooltip at a time. Note that the tooltip attribute takes the id of a tooltip element, which you don't want here. Only the tooltiptext attribute takes a string to be used in the tooltip. What's the point of label=" "? >--- a/toolkit/themes/windows/global/findBar.css >+++ b/toolkit/themes/windows/global/findBar.css >+.findbar-textbox { >+ -moz-appearance: none; >+ border: 1px solid rgba(23,50,77,.3); >+ border-radius: 2px 0 0 2px; >+ margin: 0; >+ padding: 1px 5px; >+ width: 14em; > } >+.findbar-textbox[focused="true"] { >+ border-color: rgba(51,167,255,.5); > } >+.findbar-find-previous, >+.findbar-find-next { >+ -moz-margin-start: 0; >+ -moz-appearance: none; >+ background: -moz-linear-gradient(rgba(255,255,255,.9), rgba(255,255,255,.2)); >+ border: 1px solid rgba(23,50,77,.2); >+ border-radius: 2px; >+ box-shadow: 0 1px #fff inset; >+ list-style-image: url("chrome://global/skin/icons/find-arrows.png"); >+ padding: 1px 5px; >+ line-height: 1em; >+} >+ >+.findbar-find-previous:not([disabled]):active, >+.findbar-find-next:not([disabled]):active { >+ background: rgba(23,50,76,.2); >+ border: 1px solid rgba(23,50,76,.3); >+ box-shadow: 0 1px 2px rgba(10,31,51,.2) inset; >+} Use native border colors like you did on Linux. Getting close...
Attachment #762619 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #100) > > >+ <xul:toolbarbutton anonid="find-previous" > >+ class="findbar-find-previous tabbable" > >+ tooltip="&previous.label;" > >+ tooltiptext="&previous.tooltip;" > >+ label=" " > >+ oncommand="onFindAgainCommand(true);" > >+ disabled="true" > >+ xbl:inherits="accesskey=findpreviousaccesskey"/> > >+ <xul:toolbarbutton anonid="find-next" > >+ class="findbar-find-next tabbable" > >+ tooltip="&next.label;" > >+ tooltiptext="&next.tooltip;" > >+ label=" " > >+ oncommand="onFindAgainCommand(false);" > >+ disabled="true" > >+ xbl:inherits="accesskey=findnextaccesskey"/> > > What's the point of label=" "? I used this 'hack' (if you will) to make sure that the next/ previous buttons would scale the exact same way as the textbox; using 'em', growing in size when the text size increases and vice-versa. Re-thinking this, I *might* be able to use `height: calc(1em + 8px);` or something like that.
Can you put them in a hbox with align="stretch"?
Attachment #762619 - Attachment is obsolete: true
Attachment #764715 - Flags: review?(dao)
Comment on attachment 764715 [details] [diff] [review] Change the findBar appearance and improve its UX >--- a/toolkit/locales/en-US/chrome/global/findbar.dtd >+++ b/toolkit/locales/en-US/chrome/global/findbar.dtd >@@ -2,19 +2,18 @@ > - 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/. --> > > <!-- LOCALIZATION NOTE : FILE This file contains the entities needed to --> > <!-- LOCALIZATION NOTE : FILE use the Find Bar. --> > > <!-- entities split out from browser.dtd --> > <!ENTITY next.label "Next"> >-<!ENTITY next.accesskey "N"> > <!ENTITY next.tooltip "Find the next occurrence of the phrase"> > <!ENTITY previous.label "Previous"> >-<!ENTITY previous.accesskey "P"> remove next.label and previous.label. I guess the "entities split out from browser.dtd" doesn't make sense anymore either. >+.findbar-textbox:-moz-locale-dir(rtl) { >+ border-radius: 3px 3px 0 0; This needs to be 0 3px 3px 0. Please recheck the RTL styling for the previous/next buttons. It appears broken on Linux, I haven't tested Windows / OS X.
Attachment #764715 - Flags: review?(dao) → review-
RTL mode for win & lin.
Attachment #764715 - Attachment is obsolete: true
Attachment #770953 - Flags: review?(dao)
Comment on attachment 770953 [details] [diff] [review] Change the findBar appearance and improve its UX >--- a/toolkit/themes/linux/global/findBar.css >+++ b/toolkit/themes/linux/global/findBar.css >+.findbar-find-previous, >+.findbar-find-next { >+ -moz-margin-start: 0; >+ -moz-appearance: none; >+ background: -moz-linear-gradient(rgba(255,255,255,.9), rgba(255,255,255,.2)); >+ border: 1px solid ThreeDShadow; >+ border-radius: 2px; >+ box-shadow: 0 1px #fff inset; >+ list-style-image: url("chrome://global/skin/icons/find-arrows.png"); >+ padding: 5px 9px; >+ line-height: 1em; >+} remove border-radius: 2px; >+.findbar-find-previous { >+ -moz-image-region: rect(0, 12px, 9px, 0); >+ border-radius: 0; >+} remove border-radius: 0; >+.findbar-find-next:-moz-locale-dir(ltr) { >+ border-top-left-radius: 0; >+ border-bottom-left-radius: 0; >+} remove border-top-left-radius / border-bottom-left-radius and set border-top-right-radius / border-bottom-right-radius to 2px instead. >+.findbar-find-next:-moz-locale-dir(rtl) { >+ border-top-right-radius: 0; >+ border-bottom-right-radius: 0; >+} remove border-top-right-radius / border-bottom-right-radius and set border-top-left-radius / border-bottom-left-radius to 2px instead. >+.findbar-find-previous:-moz-locale-dir(ltr), >+.findbar-find-previous:focus:-moz-locale-dir(rtl) + .findbar-find-next { >+ border-right-width: 0; >+} >+ >+.findbar-find-previous:-moz-locale-dir(rtl), >+.findbar-find-previous:focus:-moz-locale-dir(ltr) + .findbar-find-next { >+ border-left-width: 0; >+} >+ >+.findbar-find-previous:focus:-moz-locale-dir(ltr) { >+ border-right-width: 1px; >+} >+ >+.findbar-find-previous:focus:-moz-locale-dir(rtl) { >+ border-left-width: 1px; >+} use -moz-border-end-width / -moz-border-start-width These comments may or may not apply to Windows and OS X too. Please check this yourself.
Attachment #770953 - Flags: review?(dao) → review-
reviewed and updated border CSS
Attachment #770953 - Attachment is obsolete: true
Attachment #772090 - Flags: review?(dao)
Comment on attachment 772090 [details] [diff] [review] Change the findBar appearance and improve its UX >diff --git a/toolkit/themes/windows/global/icons/search-textbox.png b/toolkit/themes/windows/global/icons/search-textbox.png >new file mode 100644 Don't add this image since you're not using it. r=me with that!
Attachment #772090 - Flags: review?(dao) → review+
Removed unused image. Carrying over r=dao.
Attachment #772090 - Attachment is obsolete: true
Attachment #772203 - Flags: review+
Attachment #772203 - Attachment description: Part 1: Change the findBar appearance and improve its UX → Patch: Change the findBar appearance and improve its UX
Removed image from jar file too. Carrying over r=dao
Attachment #772203 - Attachment is obsolete: true
Attachment #772208 - Flags: review+
Keywords: checkin-needed
Depends on: 891258
No longer depends on: 891258
https://hg.mozilla.org/mozilla-central/rev/52f8868edc1d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
relnote-firefox: --- → +
Keywords: addon-compat
Depends on: 891948
Depends on: 893566
Depends on: 893672
No longer depends on: 893672
Depends on: 894285
Depends on: 892499
No longer depends on: 893566
With this new change ALT+N and ALT+P hotkeys no longer work. This is very bad and inconvenient to me :( I was told that it is possible to use Shift+Enter and Shift+F3 to navigate backwards, but I would have never found it out myself. Besides, these hotkeys are much less natural than ALT+P or ALT+N. I have posted a bug 933011 about this.
Depends on: 937208
Depends on: 939539
Depends on: 911876
Depends on: 944170
Depends on: 934539
Depends on: 935519
Could someone fix the close button please, it can vanishes after a page wrap during search, if the browser window has a smaller width than approx. 1080px. Suggestion: Move close button to the far left. Please see this bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=1222710 Only slightly related, but same suggestion though: https://bugzilla.mozilla.org/show_bug.cgi?id=998364
Or prevent the close button from vanishing.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: