| 1234qwer1234qwer4 | |
| Sep 17 2021, 3:45 PM |
| F34646208: Monobook search before.png | |
| Sep 17 2021, 3:45 PM |
| F34646210: Monobook search after.png | |
| Sep 17 2021, 3:45 PM |
List of steps to reproduce (step by step, including full links if applicable):
What happens?:
The "Search" button disappears, from
What should have happened instead?:
Both the "Go" and "Search" buttons should have stayed where they were.
Debugger says that https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/31d560955440aef1aff3adb1a4a2b8ba2aef81a9/resources/src/mediawiki.searchSuggest/searchSuggest.js#402 is removing the line, apparently intentionally. But that code was last changed in 2015, so that doesn't explain why the behavior changed.
rSMNBabe94aa4082d: Monobook Mustache recently added .mw-fallbackSearchButton to the search button, which causes it to be removed by searchSuggest.
While the fallback search button probably shouldn't be there, removing it when the search box is clicked isn't a very good user experience.
Note: functionally these buttons generate different arguments as well:
GO: /index.php?title=Special%3ASearch&search=&go=Go
SEARCH: index.php?title=Special%3ASearch&search=&fulltext=Search
Though the resulting load page appears to be the same
Thanks for the ping!
MonoBook added the class for consistency purposes, so that button can be identified across skins by that class and obviously that's flagged a very odd bit of code.
Presumably, this code can be removed. I don't see any reason why it should exist. On Vector that button is hidden via CSS so perhaps this is an artifact from a different time.
It's different. 'Go' takes you directly to the page (if it exists) else to Special:Search. 'Search' takes you to Special:Search (always)
This is true. It should probably just be removed entirely.
This code looks like a terrible hack to me. It's deleting the button to ensure that hitting enter triggers a "go" query rather than a "fulltext" query.
In Vector, when that button is not deleted, since the "fulltext" search button comes first that gets triggered on enter.
In Monobook: #searchButton, #mw-searchButton
In Vector #mw-searchButton #searchButton
So, either that JavaScript should check the button ordering (with an appropriate sibling selector and a very explicit comment) AND/OR we should change the order inside Vector.
Either way that JavaScript needs a much more detailed inline comment.
The order of buttons in Vector was intended to make the fulltext search accessible for no-JavaScript users. See the commit message in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/82100 for the explanation. Maybe there's a better way to do this, but please don't just jump in and delete everything.
I understand that. I am not suggesting we delete it for non-JavaScript users, I am suggesting that this line of code is specific to Vector for JavaScript users. Perhaps a patch might be clearer. I'll see if I can put one together.
It is not specific to Vector, it applies also to other skins that only want to display one search button (e.g. Timeless). That's why it checks for the class 'mw-fallbackSearchButton', instead of something Vector-specific.
Change 721898 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):
[mediawiki/core@master] Search behaviour should be consistent across skins
not specific to Vector, it applies also to other skins that only want to display one search button (e.g. Timeless).
Sure. I was just giving an example of one skin. I expect there are more skins out in the wild with the exact same problem.
The order of buttons in Vector was intended to make the fulltext search accessible for no-JavaScript users.
However, it seems this was not the case for CologneBlue, Modern and MonoBook. The default behavior there with JS disabled is to do a "Go" search https://en.wikipedia.org/?useskin=monobook - that is also a bug IMO - those should behave consistently going forward.
The motivation for much of the skin work is to get consistency across skins so they are predictable. If different behaviour is warranted, gadgets can be used to alter that behaviour.
The above patch should remedy the above without any problems with cached HTML.
The effects of this are especially bizarre on Commons, where the default submit action is actually Search, not Go (for some reason): selecting the search box removes the (bolded) Search button but not the (unbolded) Go button, and pressing the Return key while the search field is focused actually does nothing whatsoever (on Commons) at the moment.
In any case, being able to decide whether one wants to force full-text search or not is one of the advantages the likes of Monobook have over the likes of Vector, and one reason why a user might opt for Monobook.
@Jdlrobson The behavior of the buttons is not supposed to be consistent between Vector and MonoBook, because Vector has only one button, while MonoBook has two. It's supposed to be consistent between skins that have only one button (including Vector and Timeless), and skins that have two buttons (including CologneBlue, Modern and MonoBook). Trying to make them all behave the same makes no sense to me.
I really don't understand why you're trying to change the behavior for all skins, when we could simply restore the previous, correct behavior on MonoBook, which was changed unintentionally.
Maybe 'mw-fallbackSearchButton' should be renamed to 'mw-noJavaScriptFullTextSearchButton' or something. Buttons with that class are supposed to only be active while you're using a skin with one search button, and have JavaScript turned off so search suggestions don't appear. It's supposed to be a no-JavaScript fallback. Does that explanation make sense?
I think we should strive towards having consistent HTML and behaviour for the search box. The existing code for the search is brittle from my perspective - it makes assumptions about the element being present in the DOM and its visibility. I think https://gerrit.wikimedia.org/r/c/721898 is the right solution here since it is the smallest possible change and leaves the code and is easier to follow as it tests functionality rather than the presence of an arbitary class.
If we do need to retain the search behaviour for MonoBook I'm going to need that core patch anyhow to fix Modern and CologneBlue. I'll provide a MonoBook patch shortly.
Change 722558 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):
[mediawiki/core@master] SkinTemplate: Provide a non-fallback fulltext search button
Change 722559 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):
[mediawiki/skins/MonoBook@master] Use the non-fallback fulltext search button
Change 722646 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):
[mediawiki/core@master] Hiding fallback button depends on HTML order
Change 721898 abandoned by Jdlrobson:
[mediawiki/core@master] Search should be consistent across skins; don't hide button in MonoBook
Reason:
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/722646 is simpler.
Change 722646 merged by jenkins-bot:
[mediawiki/core@master] Hiding fallback button depends on HTML order
Change 722558 abandoned by Bartosz Dziewoński:
[mediawiki/core@master] SkinTemplate: Provide a non-fallback fulltext search button
Reason:
Change 722559 abandoned by Bartosz Dziewoński:
[mediawiki/skins/MonoBook@master] Use the non-fallback fulltext search button
Reason:
Change 722988 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):
[mediawiki/core@wmf/1.38.0-wmf.1] Hiding fallback button depends on HTML order
Change 722989 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):
[mediawiki/core@REL1_37] Hiding fallback button depends on HTML order
Change 722989 merged by jenkins-bot:
[mediawiki/core@REL1_37] Hiding fallback button depends on HTML order
Unfortunately i missed the morning backport window and this afternoon's is cancelled, so this will have to wait until Monday next week.
While deployment windows are the easiest and most straightforward way to get code deployed, major regressions like this can be fixed out of band too. I'll sync it out now.
Change 722988 merged by jenkins-bot:
[mediawiki/core@wmf/1.38.0-wmf.1] Hiding fallback button depends on HTML order
Mentioned in SAL (#wikimedia-operations) [2021-09-24T00:39:04Z] <legoktm@deploy1002> Synchronized php-1.38.0-wmf.1/resources/src/mediawiki.searchSuggest/searchSuggest.js: Hiding fallback button depends on HTML order (T291272) (duration: 00m 57s)