Search form reset button

[xmorave2]

There are still some issues to discuss I think (like: should this be configured to show?). But I would like to have some feedback.

[demiankatz]

@xmorave2, your styling seems to be having a side effect on the language control (at least in the bootprint3 theme; I haven't looked at others yet).

Before:

After:

[demiankatz]

Thanks, @xmorave2! I'd be interested in input from others (like @crhallberg), but from my perspective, this seems like a pretty clear and unintrusive addition that probably does not need to be made configurable as long as it passes accessibility review. In addition to the minor problem noted above, I've left a few other comments below.

[xmorave2]

Thank @demiankatz, I (hopefully) addressed all issuses.

[demiankatz]

This is looking better, but I'm seeing a new style issue (at least in Firefox -- I didn't try other browsers). In all three provided themes, the search box and type drop down are no longer aligned with each other. Screen shots are from bootprint3, but the other two are similar.

Before (dev branch):

After (this branch):

See also a few very minor suggestions below.

I've put a 9.1 milestone on this, since I'm pretty confident we can get it polished before that release, and this will encourage discussion on the next Community Call. I'd also like to get a review from someone more UI-oriented than myself before merging it, but I won't request that until we finish fixing the minor issues from my own reviews, to avoid any redundant suggestions.

[xmorave2]

@demiankatz: Another round done ;) I incorporated your suggestions and fixed the visual glitch.

[demiankatz]

Thanks, @xmorave2 -- this is looking much better to me. Just one remaining problem that I can see: the clear icon is nearly invisible in the sandal theme:

We may need some sandal-specific styling to adjust for this problem.

That detail aside, though, I think this is ready for input from others, so I'm going to request a couple more reviews -- I'm interested in @ckaz's thoughts on accessibility and @crhallberg's comments on the specific styles and Javascript approach.

[xmorave2]

I fixed the color in sandal theme

[demiankatz]

Thanks, @xmorave2! Looks good to me now. I also merged dev into this branch to resolve minor conflicts caused by other recent style changes. This has my personal approval, but I'll wait to mark it as approved until we receive feedback from others.

[crhallberg]

I'm not seeing any evidence of this interfering with the clear button that some browsers add to type=search inputs, which is good, but that's my only concern.

[demiankatz]

Thanks, @crhallberg, that's a good point! I'm glad that we're not seeing any problems related to this behavior, but do we understand why? I don't see an obvious reason why the browser wouldn't try to add its own clear button to this input, so why aren't we seeing double in some cases? I notice that Chrome isn't adding a clear button on the existing VuFind search box (e.g. at the public demo). Why not? It might be good to see if we can force the pseudo-element to appear so we can be sure that there is no edge case where it's going to cause problems.

I also found this StackOverflow thread which talks about styling the Webkit pseudo-element. If nothing else, maybe we should explicitly hide that in our styles if we think there's any chance we might encounter it. Or maybe we should try to leverage the native function where available, though I personally prefer not to get into supporting features that only work in certain browsers. @xmorave2, what do you think about all this?

[xmorave2]

It is not interfering with standard reset button in webkit/chrome based browsers, because normalize from bootstrap is hiding it. See https://github.com/twbs/bootstrap/blob/v3.4.1/less/normalize.less#L367

I would be happy to use the standard reset button provided by browser, but Firefox is still not implementing it.

[demiankatz]

Thanks, @xmorave2! Since we have a clear explanation for why the browser-provided button is hidden, I have no concerns about conflicts arising from the new functionality, and I think this is the best option for the present since not all browsers offer a standard implementation. I will merge this now!

[ckaz]

Sorry, I've been away on holiday leave. I have no issues with this PR -- looks perfect to me.