Closed Bug 1363743 Opened 8 years ago Closed 7 years ago

Redirecting searchResult to general if search field is empty

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- verified

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference] )

Attachments

(1 file)

Bug 1363743 - Redirect to general pane if search result is empty
59 bytes, text/x-review-board-request
mconley
: review+
Details
Preferences should be able to memorize search query string by url, so everyone can share and pass url link such as about:preferences#searchResults?query=history to memorize a "history" query has input in search field. The query string on url bar will be changed accordingly with search input. Using two way data binding mechanism to ensure input change will be updated accordingly.
Flags: qe-verify+
QA Contact: hani.yacoub
> about:preferences#searchResults?query=history The part after question mark is not technically a valid search string, which should go before the hash. Sorry for being late to the party, but I would tend to think |about:preferences#search=history| might be a better URL.
Flags: needinfo?(rchien)
about:preferences#search=history, it is reasonable. I also noticed there is an existing subcategories using "-" for separating the url. Patch has updated!
Flags: needinfo?(rchien)
Comment on attachment 8866451 [details] Bug 1363743 - Redirect to general pane if search result is empty https://reviewboard.mozilla.org/r/138084/#review145162 I'm not sure this is necessary right now. We have our way of creating pre-determined search queries that we can use in documentation, but the preferences are not linkable from other web pages so these types of memorizable query URLs will only be useful in-product where we won't have to support infinite search values. Most users struggle with typing regular URLs in to the address bar by hand, and asking them to type a URL with a different scheme, hash character and hyphen will be asking too much.
Attachment #8866451 - Flags: review?(jaws) → review-
Jared, the purpose of the memorizing query url isn't aiming at telling user to type and search their string through URL. I can see this could be very helpful when working on preferences, for example I can reload the same search result or run ./mach run about:preferences#searchResults=history to bring me to a particular search result page which I'm working on. But there are other benefits beyond development as well. * Chrome also has the same feature to memorize the query string (try chrome://settings/search#history in your Chrome) * Without this feature, relaunching browser or resuming from crash will bring to the empty search result for about:preferences tab, because now we only keep the state of url without the query string. * It's really useful for copying and pasting url to share the search result. A linkable preferences page would be more helpful in documentation. * It would be error-prone without memorizing query url like you will lost your search result after reloading preferences page How do you think?
Flags: needinfo?(jaws)
(In reply to Ricky Chien [:rickychien] from comment #11) Of the list of reasons, this is the only one that seems worth supporting: > * Without this feature, relaunching browser or resuming from crash will > bring to the empty search result for about:preferences tab, because now we > only keep the state of url without the query string. How well does the search within about:addons resume after restarts? Same for the find-in-page? > * It's really useful for copying and pasting url to share the search result. > A linkable preferences page would be more helpful in documentation. A linkable preferences page would be better than asking users to copy/pate the URL but as of now we can't do that due to security concerns.
Flags: needinfo?(jaws)
Jared, I'd like to know what's sort of security concerns that could happen? If it does matter, the only workaround we can do is to redirect #searchResult to #general if the search field is empty in order to avoid empty search result.
Flags: needinfo?(jaws)
In AboutRedirector.cpp, "preferences" is not listed as nsIAboutModule::MAKE_LINKABLE. Traditionally we have not allowed pages with chrome privileges to be linkable. See also how "newtab" is not linkable because it also requires chrome privileges. Redirecting #searchResult to #general would be preferred.
Flags: needinfo?(jaws)
Summary: Memorize the search result by url query string → Redirecting searchResult to general if search field is empty
Version: 52 Branch → Trunk
Mike, Jared is away from 27 May - 5 June, could you help review the patch? Thanks
Comment on attachment 8866451 [details] Bug 1363743 - Redirect to general pane if search result is empty https://reviewboard.mozilla.org/r/138084/#review148924 ::: browser/components/preferences/in-content-new/preferences.js:165 (Diff revision 9) > if (category != "paneSearchResults") { > gSearchResultsPane.searchInput.value = ""; > gSearchResultsPane.searchResultsCategory.hidden = true; > gSearchResultsPane.findSelection.removeAllRanges(); > + } else if (gSearchResultsPane.searchInput.value == "") { > + category = kDefaultCategoryInternalName > + document.location.hash = "general"; > } I wonder if it'd be clearer like: ```js const kDefaultCategoryInternalName = "paneGeneral"; const kDefaultCategory = "general"; // ... if (category == "paneSearchResults" && !gSearchResultsPane.searchInput.value) { // Something tried to send us to the search results pane without // a query string. Default to the General pane instead. category = kDefaultCategoryInternalName; document.location.hash = kDefaultCategory; } else { gSearchResultsPane.searchInput.value = ""; // ... etc } ``` I know it seems small, but for me, it reads more clearly if we merge the two conditions and avoid the else-if. I'm pretty sure this still gives us the behaviour that we want. ::: browser/components/preferences/in-content-new/preferences.js:170 (Diff revision 9) > if (category != "paneSearchResults") { > gSearchResultsPane.searchInput.value = ""; > gSearchResultsPane.searchResultsCategory.hidden = true; > gSearchResultsPane.findSelection.removeAllRanges(); > + } else if (gSearchResultsPane.searchInput.value == "") { > + category = kDefaultCategoryInternalName Nit: missing semi-colon
Attachment #8866451 - Flags: review?(mconley) → review+
Comment on attachment 8866451 [details] Bug 1363743 - Redirect to general pane if search result is empty https://reviewboard.mozilla.org/r/138084/#review148924 > I wonder if it'd be clearer like: > > ```js > > const kDefaultCategoryInternalName = "paneGeneral"; > const kDefaultCategory = "general"; > > // ... > > if (category == "paneSearchResults" && > !gSearchResultsPane.searchInput.value) { > // Something tried to send us to the search results pane without > // a query string. Default to the General pane instead. > category = kDefaultCategoryInternalName; > document.location.hash = kDefaultCategory; > } else { > gSearchResultsPane.searchInput.value = ""; > // ... etc > } > > ``` > > I know it seems small, but for me, it reads more clearly if we merge the two conditions and avoid the else-if. I'm pretty sure this still gives us the behaviour that we want. We still need to check `} else if (category != "paneSearchResults") {` instead of `} else {` to avoid search result page being redirected unexpectedly if search input has value but category is in paneSearchResults.
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2a51ed1fc840 Redirect to general pane if search result is empty r=mconley
https://hg.mozilla.org/mozilla-central/rev/2a51ed1fc840
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Build ID: 20170613030203 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0 Verified as fixed on Firefox Nightly 56.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → ---
status-firefox56: --- → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: