1324171 - Search fields should always have "Search" as the placeholder text instead of appearing in-front of the Search field

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

From slide 8 and 9 of https://bugzilla.mozilla.org/attachment.cgi?id=8819509

Comment 1

[Marco Bonardo [:mak]]

whoops, I misread all of it :(

Comment 2

[Julianne Adams]

I'm willing to take this bug if no one else is working on it already.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Cool, it's yours! Let me know if you have questions.

Comment 4

[Julianne Adams]

Attachment: Bug 1324171 - Use 'Search' as placeholder text instead of as a label in-front of search fields.

Review commit: https://reviewboard.mozilla.org/r/99952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99952/

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8820451 [details]
Bug 1324171 - Use 'Search' as placeholder text instead of as a label in-front of search fields.

https://reviewboard.mozilla.org/r/99952/#review100712

::: browser/components/preferences/cookies.xul:38
(Diff revision 1)
>      <hbox align="center">
> -      <label accesskey="&filter.accesskey;" control="filter">&filter.label;</label>
>        <textbox type="search" id="filter" flex="1"
>                 aria-controls="cookiesList"
> -               oncommand="gCookiesWindow.filter();"/>
> +               oncommand="gCookiesWindow.filter();"
> +               placeholder="Search"/>

We need to use the localizable strings here so that the placeholder text matches the language of the application.

This should have placeholder="&filter.label;" and also accesskey="&filter.accesskey;".

The same should be done for passwordManager.xul.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

You can test that the accesskey works by pressing Shift+Alt+accesskey when the field isn't focused. Pressing the combination should move focus to the field.

Previously the accesskey was visible to users by showing the underline on the accesskey character. This won't be visible anymore now that we have switched to the placeholder text, but we shouldn't break the functionality for users that are used to this working.

Comment 7

[Julianne Adams]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> We need to use the localizable strings here so that the placeholder text
> matches the language of the application.

Thanks for the tip. I'll try to keep things like that in mind in the future.

I removed the colon from the labels to match the example in the slides. Should the entities' keys be modified so all the locales see the change (like in Bug 1324173)?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jalen Adams from comment #7)
> I removed the colon from the labels to match the example in the slides.
> Should the entities' keys be modified so all the locales see the change
> (like in Bug 1324173)?

Yeah, good call. We should update the entities' keys. We wouldn't want some of the locales to still have "Search:" as the placeholder.

Comment 9

[Julianne Adams]

Comment on attachment 8820451 [details]
Bug 1324171 - Use 'Search' as placeholder text instead of as a label in-front of search fields.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99952/diff/1-2/

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8820451 [details]
Bug 1324171 - Use 'Search' as placeholder text instead of as a label in-front of search fields.

https://reviewboard.mozilla.org/r/99952/#review101056

Perfect!

Comment 11

[Mike Conley (:mconley) (:⚙️)]

It looks like autoland on this failed. It might need to be rebased.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Something got screwed up with the revisions of the patch on this bug. The first revision has the labels being removed and "Search" hardcoded as the placeholder. The second revision shows "Search" being changed to use the locale string, but the history doesn't connect the two.

I've fixed it on my machine and am building with it right now. Once the build is done, if it looks good I'll get it landed.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1324171 - Use 'Search' as placeholder text instead of as a label in-front of search fields.

Review commit: https://reviewboard.mozilla.org/r/100542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100542/

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8821205 [details]
Bug 1324171 - Use 'Search' as placeholder text instead of as a label in-front of search fields.

https://reviewboard.mozilla.org/r/100542/#review101084

Fixed up the patch and tested that it worked.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d13ec3eac00
Use 'Search' as placeholder text instead of as a label in-front of search fields. r=jaws

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9d13ec3eac00

Comment 17

[Fahima Zulfath]

 [testday-20170317] 
Bug 1324171 - Use 'Search' as placeholder text instead of as a label in-front of search fields is verified.
BROWSER: Firefox Beta 52.0
OS: Windows 10.0

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

This was never uplifted to Firefox 52 so it can't be verified there.