Closed Bug 935521 Opened 6 years ago Closed 3 years ago

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

Categories

(Toolkit :: Find Toolbar, defect, P2)

All
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: shorlander, Assigned: mikedeboer)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

The previous and next buttons on the Findbar appear clickable even when there is no search term or a match has not been found.
Component: General → Find Toolbar
Product: Firefox → Toolkit
Priority: -- → P2
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
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?
Attachment #8808667 - Flags: review?(gijskruitbosch+bugs) → review-
(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 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
Attachment #8808667 - Flags: review?(gijskruitbosch+bugs) → review+
Oh, one more thing: do the buttons start out disabled right now when the binding is constructed? I believe they should.
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mdeboer)
Pushed by mdeboer@mozilla.com:
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
Attachment #8810368 - Flags: review?(gijskruitbosch+bugs)
Attachment #8810368 - Flags: checkin?(cbook)
Attachment #8810368 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by cbook@mozilla.com:
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
Attachment #8810368 - Flags: checkin?(cbook) → checkin+
https://hg.mozilla.org/mozilla-central/rev/89101092aea1
https://hg.mozilla.org/mozilla-central/rev/4f3b0ffe948d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1326993
Depends on: 1327212
Depends on: 1328035
QA Whiteboard: [good first verify]
Depends on: 1358500
You need to log in before you can comment on or make changes to this bug.