Last Comment Bug 776708 - Improve the visual appearance of the "find in page" bar
: Improve the visual appearance of the "find in page" bar
Status: RESOLVED FIXED
: addon-compat
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla25
Assigned To: Mike de Boer [:mikedeboer]
:
Mentors:
https://www.dropbox.com/s/ijg3cmw00z4...
: 755735 (view as bug list)
Depends on: 894285 932643 891948 892499 911876 934539 935519 937208 939539 944170
Blocks: 565552 869543 257061 891842
  Show dependency treegraph
 
Reported: 2012-07-23 14:30 PDT by Chris Lee (:cleer)
Modified: 2016-03-15 07:23 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Mac OS X Patch v3 (21.49 KB, patch)
2012-07-23 14:30 PDT, Chris Lee (:cleer)
fryn: review-
Details | Diff | Splinter Review
screenshot of patch v3 (15.26 KB, image/png)
2012-07-23 15:12 PDT, Frank Yan (:fryn)
no flags Details
WIP Mac OS X patch v4 (21.74 KB, patch)
2012-07-23 17:23 PDT, Chris Lee (:cleer)
no flags Details | Diff | Splinter Review
Mac OS X Patch v5 (21.68 KB, patch)
2012-07-25 14:39 PDT, Chris Lee (:cleer)
fryn: review-
Details | Diff | Splinter Review
Mac OS X Patch v6 (19.38 KB, patch)
2012-07-30 12:40 PDT, Chris Lee (:cleer)
no flags Details | Diff | Splinter Review
Mac OS X Patch v6.1 (1.19 KB, patch)
2012-07-30 12:48 PDT, Chris Lee (:cleer)
no flags Details | Diff | Splinter Review
Mac OS X Patch v6.2 (20.62 KB, patch)
2012-07-30 12:50 PDT, Chris Lee (:cleer)
fryn: review-
Details | Diff | Splinter Review
Mac OS X Patch v7 (19.90 KB, patch)
2012-08-08 15:11 PDT, Chris Lee (:cleer)
fryn: review+
dao+bmo: review-
Details | Diff | Splinter Review
Mac OS X Patch v8 (19.55 KB, patch)
2012-08-14 14:06 PDT, Chris Lee (:cleer)
dao+bmo: review-
Details | Diff | Splinter Review
Mac OS X Patch v9 (20.72 KB, patch)
2012-08-14 17:07 PDT, Chris Lee (:cleer)
no flags Details | Diff | Splinter Review
Mac OS X Patch v9.1 (21.93 KB, patch)
2012-08-15 14:49 PDT, Chris Lee (:cleer)
dao+bmo: review-
Details | Diff | Splinter Review
Mac OS X Patch v10 (21.82 KB, patch)
2012-08-20 14:37 PDT, Chris Lee (:cleer)
no flags Details | Diff | Splinter Review
Mac OS X Patch v11 (21.30 KB, patch)
2012-08-21 14:35 PDT, Chris Lee (:cleer)
no flags Details | Diff | Splinter Review
Windows and Mac OS X Patch v1 (27.77 KB, patch)
2012-08-22 18:25 PDT, Chris Lee (:cleer)
no flags Details | Diff | Splinter Review
Change the findBar appearance to be on top and improve its UX (36.39 KB, patch)
2013-04-19 09:06 PDT, Mike de Boer [:mikedeboer]
fryn: ui‑review-
Details | Diff | Splinter Review
Change the findBar appearance to be on top and improve its UX (37.29 KB, patch)
2013-04-24 07:38 PDT, Mike de Boer [:mikedeboer]
dao+bmo: review-
fryn: ui‑review+
fryn: feedback+
Details | Diff | Splinter Review
screenshot OSX (383.83 KB, image/png)
2013-04-24 07:41 PDT, Mike de Boer [:mikedeboer]
no flags Details
Screenshot Win8 (desktop) (69.56 KB, image/png)
2013-04-24 08:00 PDT, Mike de Boer [:mikedeboer]
no flags Details
Change the findBar appearance and improve its UX (44.57 KB, patch)
2013-05-07 02:55 PDT, Mike de Boer [:mikedeboer]
mdeboer: ui‑review+
Details | Diff | Splinter Review
Screenshot Linux (Ubuntu 13.04) (29.68 KB, image/png)
2013-05-07 03:19 PDT, Mike de Boer [:mikedeboer]
no flags Details
Part 1: Change the findBar appearance and improve its UX (42.02 KB, patch)
2013-05-07 03:40 PDT, Mike de Boer [:mikedeboer]
dao+bmo: review-
mdeboer: ui‑review+
Details | Diff | Splinter Review
Part 2: Move findbar to the top (8.75 KB, patch)
2013-05-07 06:10 PDT, Mike de Boer [:mikedeboer]
no flags Details | Diff | Splinter Review
Part 1: Change the findBar appearance and improve its UX (43.28 KB, patch)
2013-05-24 05:48 PDT, Mike de Boer [:mikedeboer]
mdeboer: ui‑review+
Details | Diff | Splinter Review
Screenshot Linux (Ubuntu 13.04) (10.71 KB, image/png)
2013-05-24 05:51 PDT, Mike de Boer [:mikedeboer]
no flags Details
Findbar showing buttons without additional styling (86.65 KB, image/png)
2013-05-27 07:08 PDT, Mike de Boer [:mikedeboer]
no flags Details
Change the findBar appearance and improve its UX (42.99 KB, patch)
2013-05-27 07:26 PDT, Mike de Boer [:mikedeboer]
dao+bmo: review-
Details | Diff | Splinter Review
screenshot OSX (34.46 KB, image/png)
2013-05-27 07:28 PDT, Mike de Boer [:mikedeboer]
no flags Details
Screenshot Linux (Ubuntu 13.04) (25.57 KB, image/png)
2013-05-27 10:02 PDT, Mike de Boer [:mikedeboer]
no flags Details
Change the findBar appearance and improve its UX (42.30 KB, patch)
2013-05-27 10:04 PDT, Mike de Boer [:mikedeboer]
no flags Details | Diff | Splinter Review
Change the findBar appearance and improve its UX (43.23 KB, patch)
2013-06-11 14:25 PDT, Mike de Boer [:mikedeboer]
dao+bmo: review-
Details | Diff | Splinter Review
Change the findBar appearance and improve its UX (42.46 KB, patch)
2013-06-14 04:34 PDT, Mike de Boer [:mikedeboer]
no flags Details | Diff | Splinter Review
Change the findBar appearance and improve its UX (42.44 KB, patch)
2013-06-14 04:44 PDT, Mike de Boer [:mikedeboer]
dao+bmo: review-
Details | Diff | Splinter Review
Change the findBar appearance and improve its UX (43.02 KB, patch)
2013-06-19 06:16 PDT, Mike de Boer [:mikedeboer]
dao+bmo: review-
Details | Diff | Splinter Review
Change the findBar appearance and improve its UX (42.88 KB, patch)
2013-07-03 12:17 PDT, Mike de Boer [:mikedeboer]
dao+bmo: review-
Details | Diff | Splinter Review
Change the findBar appearance and improve its UX (42.41 KB, patch)
2013-07-08 07:55 PDT, Mike de Boer [:mikedeboer]
dao+bmo: review+
Details | Diff | Splinter Review
Patch: Change the findBar appearance and improve its UX (41.80 KB, patch)
2013-07-08 11:45 PDT, Mike de Boer [:mikedeboer]
mdeboer: review+
Details | Diff | Splinter Review
Patch: Change the findBar appearance and improve its UX (38.55 KB, patch)
2013-07-08 11:49 PDT, Mike de Boer [:mikedeboer]
mdeboer: review+
Details | Diff | Splinter Review

Description Chris Lee (:cleer) 2012-07-23 14:30:15 PDT
Created attachment 645077 [details] [diff] [review]
Mac OS X Patch v3

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.
Comment 1 Chris Lee (:cleer) 2012-07-23 14:57:50 PDT
(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.
Comment 2 Frank Yan (:fryn) 2012-07-23 15:12:03 PDT
Created attachment 645092 [details]
screenshot of patch v3

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 3 Frank Yan (:fryn) 2012-07-23 15:14:23 PDT
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. :)
Comment 4 Chris Lee (:cleer) 2012-07-23 17:23:59 PDT
Created attachment 645137 [details] [diff] [review]
WIP Mac OS X patch v4

Fixes RTL styling and a bug with how the quick find class was being added.
Comment 5 Chris Lee (:cleer) 2012-07-25 14:39:58 PDT
Created attachment 645891 [details] [diff] [review]
Mac OS X Patch v5

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.
Comment 6 Chris Lee (:cleer) 2012-07-25 15:02:18 PDT
*** Bug 755735 has been marked as a duplicate of this bug. ***
Comment 7 Frank Yan (:fryn) 2012-07-26 04:06:15 PDT
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.
Comment 8 Chris Lee (:cleer) 2012-07-30 12:40:09 PDT
Created attachment 647252 [details] [diff] [review]
Mac OS X Patch v6

Should fix both issues.
Comment 9 Chris Lee (:cleer) 2012-07-30 12:48:50 PDT
Created attachment 647258 [details] [diff] [review]
Mac OS X Patch v6.1

Oops, an image somehow went missing in the previous patch.
Comment 10 Chris Lee (:cleer) 2012-07-30 12:50:30 PDT
Created attachment 647259 [details] [diff] [review]
Mac OS X Patch v6.2

Double oops, I uploaded the wrong patch...
Comment 11 Frank Yan (:fryn) 2012-08-07 23:54:54 PDT
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. :/
Comment 12 Chris Lee (:cleer) 2012-08-08 15:11:11 PDT
Created attachment 650335 [details] [diff] [review]
Mac OS X Patch v7

Fixed the issue mentioned above.
Comment 13 Frank Yan (:fryn) 2012-08-09 02:17:45 PDT
Comment on attachment 650335 [details] [diff] [review]
Mac OS X Patch v7

\o/
Comment 14 Dão Gottwald [:dao] 2012-08-10 10:26:11 PDT
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/.
Comment 15 Chris Lee (:cleer) 2012-08-13 14:51:56 PDT
Any suggestions for who would be an appropriate "toolkit peer"?
Comment 16 Dão Gottwald [:dao] 2012-08-13 15:10:26 PDT
(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.
Comment 17 Chris Lee (:cleer) 2012-08-14 14:06:45 PDT
Created attachment 651877 [details] [diff] [review]
Mac OS X Patch v8

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!
Comment 18 Dão Gottwald [:dao] 2012-08-14 14:50:27 PDT
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?
Comment 19 Chris Lee (:cleer) 2012-08-14 17:07:16 PDT
Created attachment 651942 [details] [diff] [review]
Mac OS X Patch v9

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.
Comment 20 Chris Lee (:cleer) 2012-08-15 14:49:09 PDT
Created attachment 652244 [details] [diff] [review]
Mac OS X Patch v9.1

Forgot an image.
Comment 21 Dão Gottwald [:dao] 2012-08-16 01:43:42 PDT
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.
Comment 22 Chris Lee (:cleer) 2012-08-16 13:37:59 PDT
(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.
Comment 23 Chris Lee (:cleer) 2012-08-20 14:37:19 PDT
Created attachment 653525 [details] [diff] [review]
Mac OS X Patch v10

- Edited entity names.
- Using shared values for button/toolbar styles. Might file a separate bug to update these styles.
Comment 24 Chris Lee (:cleer) 2012-08-21 14:35:10 PDT
Created attachment 653957 [details] [diff] [review]
Mac OS X Patch v11

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.
Comment 25 Chris Lee (:cleer) 2012-08-22 18:25:53 PDT
Created attachment 654483 [details] [diff] [review]
Windows and Mac OS X Patch v1

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.
Comment 26 Guillaume C. [:ge3k0s] 2012-12-18 06:03:06 PST
This should be one of top Australis priorities now. Current patch could be even launched on UX at least on OSX
Comment 27 Alfred Kayser 2013-02-06 05:29:35 PST
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}
Comment 28 Alfred Kayser 2013-02-06 05:32:56 PST
(In reply to Alfred Kayser from comment #27)
Oops, wrong bug
Comment 29 Guillaume C. [:ge3k0s] 2013-04-06 03:16:08 PDT
Nice to have for Australis part one.
Comment 30 Mike de Boer [:mikedeboer] 2013-04-16 08:06:14 PDT
nice! what's needed still to land this? If this is known, I'd be happy to take this bug.
Comment 31 Guillaume C. [:ge3k0s] 2013-04-16 08:12:06 PDT
(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 32 Dão Gottwald [:dao] 2013-04-16 08:16:12 PDT
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.
Comment 33 Guillaume C. [:ge3k0s] 2013-04-16 08:19:20 PDT
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.
Comment 34 Mike de Boer [:mikedeboer] 2013-04-19 09:06:48 PDT
Created attachment 739628 [details] [diff] [review]
Change the findBar appearance to be on top and improve its UX

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?
Comment 35 Frank Yan (:fryn) 2013-04-19 10:52:31 PDT
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.
Comment 36 Chris Lee (:cleer) 2013-04-19 23:01:55 PDT
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 37 Frank Yan (:fryn) 2013-04-22 13:25:56 PDT
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.
Comment 38 Mike de Boer [:mikedeboer] 2013-04-24 06:01:49 PDT
Frank: Thanks for reviewing this! I'll take a look at the styling...
Comment 39 Guillaume C. [:ge3k0s] 2013-04-24 06:46:33 PDT
Could we have a screenshot of what it currently looks like on various platforms ?
Comment 40 Mike de Boer [:mikedeboer] 2013-04-24 07:38:22 PDT
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.

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.
Comment 41 Mike de Boer [:mikedeboer] 2013-04-24 07:41:42 PDT
Created attachment 741311 [details]
screenshot OSX
Comment 42 Mike de Boer [:mikedeboer] 2013-04-24 08:00:16 PDT
Created attachment 741327 [details]
Screenshot Win8 (desktop)
Comment 43 Guillaume C. [:ge3k0s] 2013-04-24 09:05:22 PDT
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 ?
Comment 44 Guillaume C. [:ge3k0s] 2013-04-24 09:29:46 PDT
(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.
Comment 45 Dão Gottwald [:dao] 2013-04-24 09:53:22 PDT
(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.
Comment 46 Guillaume C. [:ge3k0s] 2013-04-24 10:01:09 PDT
(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.
Comment 47 Dão Gottwald [:dao] 2013-04-24 10:08:48 PDT
(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.
Comment 48 Guillaume C. [:ge3k0s] 2013-04-24 10:24:56 PDT
(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.
Comment 49 Dão Gottwald [:dao] 2013-04-24 11:05:19 PDT
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.
Comment 50 Frank Yan (:fryn) 2013-04-25 12:45:33 PDT
(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.
Comment 51 Frank Yan (:fryn) 2013-04-25 12:56:51 PDT
(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 52 Frank Yan (:fryn) 2013-04-25 17:27:54 PDT
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.
Comment 53 Iain Hallam 2013-04-26 02:03:36 PDT
(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.
Comment 54 Dão Gottwald [:dao] 2013-04-26 08:18:08 PDT
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.
Comment 55 Guillaume C. [:ge3k0s] 2013-04-26 08:26:52 PDT
(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.
Comment 56 Guillaume C. [:ge3k0s] 2013-04-26 08:29:26 PDT
(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).
Comment 57 Dão Gottwald [:dao] 2013-04-26 11:06:59 PDT
(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.
Comment 58 Guillaume C. [:ge3k0s] 2013-04-26 11:13:56 PDT
(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.
Comment 59 Frank Yan (:fryn) 2013-04-26 11:49:20 PDT
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.
Comment 60 Mike de Boer [:mikedeboer] 2013-05-07 02:55:32 PDT
Created attachment 746306 [details] [diff] [review]
Change the findBar appearance and improve its UX

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.
Comment 61 Mike de Boer [:mikedeboer] 2013-05-07 03:19:58 PDT
Created attachment 746318 [details]
Screenshot Linux (Ubuntu 13.04)
Comment 62 Mike de Boer [:mikedeboer] 2013-05-07 03:40:53 PDT
Created attachment 746324 [details] [diff] [review]
Part 1: Change the findBar appearance and improve its UX

Forgots a few things... carrying over ui-r+=fryn
Comment 63 Mike de Boer [:mikedeboer] 2013-05-07 06:10:14 PDT
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]
Comment 64 Mike de Boer [:mikedeboer] 2013-05-07 06:12:10 PDT
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 65 Dão Gottwald [:dao] 2013-05-07 09:52:08 PDT
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.
Comment 66 Frank Yan (:fryn) 2013-05-07 10:15:56 PDT
(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. :)
Comment 67 Frank Yan (:fryn) 2013-05-07 10:45:27 PDT
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 68 Dão Gottwald [:dao] 2013-05-08 02:13:05 PDT
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...
Comment 69 Dão Gottwald [:dao] 2013-05-08 02:57:23 PDT
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.
Comment 70 Mike de Boer [:mikedeboer] 2013-05-08 03:41:07 PDT
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!
Comment 71 Mike de Boer [:mikedeboer] 2013-05-21 09:52:54 PDT
(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!
Comment 72 Jared Wein [:jaws] (please needinfo? me) 2013-05-21 09:57:58 PDT
(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.
Comment 73 :Gijs Kruitbosch 2013-05-21 10:17:15 PDT
(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. :-(
Comment 74 Jared Wein [:jaws] (please needinfo? me) 2013-05-21 10:20:11 PDT
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.
Comment 75 Jared Wein [:jaws] (please needinfo? me) 2013-05-21 10:40:14 PDT
(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.
Comment 76 Dão Gottwald [:dao] 2013-05-22 02:32:27 PDT
(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.
Comment 77 Mike de Boer [:mikedeboer] 2013-05-24 05:48:04 PDT
Created attachment 753747 [details] [diff] [review]
Part 1: Change the findBar appearance and improve its UX

Addressed comments from Dão & Jared for all platforms
Comment 78 Mike de Boer [:mikedeboer] 2013-05-24 05:51:21 PDT
Created attachment 753749 [details]
Screenshot Linux (Ubuntu 13.04)

improved styling on Linux desktop
Comment 79 Dão Gottwald [:dao] 2013-05-24 08:18:27 PDT
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.
Comment 80 Mike de Boer [:mikedeboer] 2013-05-27 07:05:21 PDT
(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.
Comment 81 Mike de Boer [:mikedeboer] 2013-05-27 07:08:50 PDT
Created attachment 754452 [details]
Findbar showing buttons without additional styling
Comment 82 Guillaume C. [:ge3k0s] 2013-05-27 07:12:11 PDT
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
Comment 83 Mike de Boer [:mikedeboer] 2013-05-27 07:26:27 PDT
Created attachment 754459 [details] [diff] [review]
Change the findBar appearance and improve its UX

Addressed issues raised in comment 79 and will match the mockup as posted in comment 82 (except for bar on top).
Comment 84 Mike de Boer [:mikedeboer] 2013-05-27 07:28:08 PDT
Created attachment 754463 [details]
screenshot OSX
Comment 85 Guillaume C. [:ge3k0s] 2013-05-27 07:54:36 PDT
(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 86 Dão Gottwald [:dao] 2013-05-27 07:54:47 PDT
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].
Comment 87 Mike de Boer [:mikedeboer] 2013-05-27 08:04:02 PDT
(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...
Comment 88 Mike de Boer [:mikedeboer] 2013-05-27 08:32:26 PDT
(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.
Comment 89 Guillaume C. [:ge3k0s] 2013-05-27 08:36:27 PDT
(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
Comment 90 Mike de Boer [:mikedeboer] 2013-05-27 10:02:50 PDT
Created attachment 754506 [details]
Screenshot Linux (Ubuntu 13.04)
Comment 91 Mike de Boer [:mikedeboer] 2013-05-27 10:04:57 PDT
Created attachment 754508 [details] [diff] [review]
Change the findBar appearance and improve its UX
Comment 92 Mike de Boer [:mikedeboer] 2013-06-03 09:52:31 PDT
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!
Comment 93 Mike de Boer [:mikedeboer] 2013-06-07 06:29:19 PDT
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...
Comment 94 Guillaume C. [:ge3k0s] 2013-06-07 07:54:59 PDT
The best solution is to use the Australis styling on every platform (to be consistent with the toolbar icons styling).
Comment 95 Dão Gottwald [:dao] 2013-06-11 09:53:27 PDT
(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.
Comment 96 Mike de Boer [:mikedeboer] 2013-06-11 14:25:14 PDT
Created attachment 761159 [details] [diff] [review]
Change the findBar appearance and improve its UX

Reverted the OSX button styling
Comment 97 Dão Gottwald [:dao] 2013-06-14 00:41:02 PDT
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.
Comment 98 Mike de Boer [:mikedeboer] 2013-06-14 04:34:20 PDT
Created attachment 762612 [details] [diff] [review]
Change the findBar appearance and improve its UX

Linux, behave!
Comment 99 Mike de Boer [:mikedeboer] 2013-06-14 04:44:01 PDT
Created attachment 762619 [details] [diff] [review]
Change the findBar appearance and improve its UX

ok ok ok, that padding-bottom was kinda ambiguous...
Comment 100 Dão Gottwald [:dao] 2013-06-15 08:33:35 PDT
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...
Comment 101 Mike de Boer [:mikedeboer] 2013-06-19 01:50:20 PDT
(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.
Comment 102 Dão Gottwald [:dao] 2013-06-19 02:01:24 PDT
Can you put them in a hbox with align="stretch"?
Comment 103 Mike de Boer [:mikedeboer] 2013-06-19 06:16:11 PDT
Created attachment 764715 [details] [diff] [review]
Change the findBar appearance and improve its UX
Comment 104 Dão Gottwald [:dao] 2013-06-21 05:16:48 PDT
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.
Comment 105 Mike de Boer [:mikedeboer] 2013-07-03 12:17:40 PDT
Created attachment 770953 [details] [diff] [review]
Change the findBar appearance and improve its UX

RTL mode for win & lin.
Comment 106 Dão Gottwald [:dao] 2013-07-06 02:53:50 PDT
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.
Comment 107 Mike de Boer [:mikedeboer] 2013-07-08 07:55:58 PDT
Created attachment 772090 [details] [diff] [review]
Change the findBar appearance and improve its UX

reviewed and updated border CSS
Comment 108 Dão Gottwald [:dao] 2013-07-08 10:27:51 PDT
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!
Comment 109 Mike de Boer [:mikedeboer] 2013-07-08 11:45:23 PDT
Created attachment 772203 [details] [diff] [review]
Patch: Change the findBar appearance and improve its UX

Removed unused image. Carrying over r=dao.
Comment 110 Mike de Boer [:mikedeboer] 2013-07-08 11:49:10 PDT
Created attachment 772208 [details] [diff] [review]
Patch: Change the findBar appearance and improve its UX

Removed image from jar file too. Carrying over r=dao
Comment 111 Ryan VanderMeulen [:RyanVM] 2013-07-08 13:43:52 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/52f8868edc1d
Comment 112 Ed Morley [:emorley] 2013-07-09 02:29:54 PDT
https://hg.mozilla.org/mozilla-central/rev/52f8868edc1d
Comment 113 Aleksey Hizhnyak 2013-10-30 15:12:18 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.