935521 - Findbar Previous/Next buttons don't have an inactive state

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P2)

Description

[Stephen Horlander [:shorlander]]

The previous and next buttons on the Findbar appear clickable even when there is no search term or a match has not been found.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 935521 - properly disable the find next/ previous buttons on the find toolbar and update their appearance when disabled.

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

Comment 2

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f89791b2f76d

Comment 3

[:Gijs (he/him)]

Comment on attachment 8808667 [details]
Bug 935521 - properly disable the find next/ previous buttons on the find toolbar and update their appearance when disabled.

https://reviewboard.mozilla.org/r/91434/#review91366

So I have some comments below, but beyond that, r- because this should get some tests. :-)

::: toolkit/content/widgets/findbar.xml:1000
(Diff revision 1)
>  
>            // We have to carry around an explicit version of this,
>            // because finder.searchString doesn't update on failed
>            // searches.
>            this.browser._lastSearchString = val;
> +          this._enableFindButtons(val);

`val` is a string and this takes a bool, so please use `!!` to avoid confusion.

Does this actually make sense? Why are we enabling the buttons when the value changes, only to then disable them again when we get results? Feels like we should only update the state when we get results, and they should actually start *disabled* rather than enabled (can't go forward/backward when the find bar contains 0 values).

Also, why move this out of the if condition?

::: toolkit/themes/windows/global/findBar.css:107
(Diff revision 1)
>  .findbar-find-next {
> -  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#glyph-find-next);
> +  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#next);
> +}
> +
> +.findbar-find-previous[disabled] {
> +  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#previous-disabled);

Have you checked GrayText works for the devedition hardcoded colours?

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8808667 [details]
Bug 935521 - properly disable the find next/ previous buttons on the find toolbar and update their appearance when disabled.

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

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #3)
> So I have some comments below, but beyond that, r- because this should get
> some tests. :-)

Very true! Thanks.

> Does this actually make sense? Why are we enabling the buttons when the
> value changes, only to then disable them again when we get results? Feels
> like we should only update the state when we get results, and they should
> actually start *disabled* rather than enabled (can't go forward/backward
> when the find bar contains 0 values).
> 
> Also, why move this out of the if condition?

Thanks for the thoughts here, there's indeed no good reason to have it here. The findbar code is oooooooold.

> Have you checked GrayText works for the devedition hardcoded colours?

DevEdition doesn't style the prev/ next buttons.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8808667 [details]
Bug 935521 - properly disable the find next/ previous buttons on the find toolbar and update their appearance when disabled.

https://reviewboard.mozilla.org/r/91434/#review92656

Comment 7

[:Gijs (he/him)]

Oh, one more thing: do the buttons start out disabled right now when the binding is constructed? I believe they should.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8808667 [details]
Bug 935521 - properly disable the find next/ previous buttons on the find toolbar and update their appearance when disabled.

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

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #7)
> Oh, one more thing: do the buttons start out disabled right now when the
> binding is constructed? I believe they should.

Nope. I agree that they should, so I pushed that change to MozReview.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89101092aea1
properly disable the find next/ previous buttons on the find toolbar and update their appearance when disabled. r=Gijs

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Follow-up patch: fix browser_bug537013.js mochitest

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f3b0ffe948d
fix the browser_bug537013.js mochitest to know about the default disabled buttons state and that the Highlight All state does not switch on reload anymore. r=Gijs

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89101092aea1
https://hg.mozilla.org/mozilla-central/rev/4f3b0ffe948d