The search being implemented by bug 1335905 filters elements within the preferences pages but doesn't reach in to subdialogs and various popups to find any matching text there. We should look for string matches within popups, and if we find any we should show a tooltip (bug 1351867) next to the button showing that the text would be revealed by clicking on the button. We may need to either build a string index or have some way to find the strings within a dialog instead of opening each dialog as that will have really bad performance.

Comment on attachment 8853797 [details] Bug 1352481 - Finding content and highlights in popups. https://reviewboard.mozilla.org/r/125848/#review131562 Hey manotej, I'm having a pretty hard time differentiating between "stuff that was hacked in in order to make the demo work", and "stuff you want reviewed and want to consider landing". Can you make those two things more distinct, perhaps with different commits, or perhaps with some comments? ::: browser/components/preferences/in-content/findInPage.js:647 (Diff revision 4) > + getSubDialogContent() { > + console.log("getting Sub dialog Contet") > + let arry = this.dialogBoxs[this.counterDialog]; > + let dialogObj = document.getElementById("dialogBox"); > + //console.log("db",dialogObj,dialogObj.children.length); > + arry[1] = this.leafNodes(dialogObj); > + //console.log("ar",arry[1],this.totalSubDialogs,this.counterDialog); > + if (this.totalSubDialogs != this.counterDialog) { > + this.counterDialog += 1; > + this.openSubDialogBox(); > + } > + document.getElementById("dialogClose").click(); > + }, > + > + openSubDialogBox() { > + let arry = this.dialogBoxs[this.counterDialog]; > + //console.log("arry",arry) > + if (arry) { > + document.getElementById(arry[0]).click(); > + } > + }, > + > + leafNodes(nodeObject) { > + let matchesFound = ""; > + //console.log("fun: ",nodeObject,nodeObject.childNodes.length) > + if (nodeObject.childNodes.length == 0) { > + //console.log("leaf",nodeObject, nodeObject.textContent,nodeObject.innerHTML); > + //console.log("no",nodeObject,nodeObject.textContent, nodeObject.tagName); > + if (nodeObject.textContent.indexOf("chrome://") == -1) { > + matchesFound += nodeObject.textContent.trim(); > + } > + } > + > + for (let i = 0; i < nodeObject.childNodes.length; i++) { > + //console.log("\tchild:",nodeObject.childNodes[i],nodeObject.childNodes[i].childNodes.length); > + // Search only if child node is not hidden > + let result = this.leafNodes(nodeObject.childNodes[i]); > + if (nodeObject.childNodes[i].contentDocument) { > + result += " " + this.leafNodes(nodeObject.childNodes[i].contentDocument); > + } > + //console.log("result",result); > + matchesFound += " " + result; > + } > + return matchesFound; > + }, As a general comment, the formatting is really really important in order to try to do a review, and this is pretty hard to read. You might want to try running your code through ESLint before pushing to review. You can set up the linter like so: ```bash ./mach eslint --setup ./mach eslint <path to file to lint> ``` Can I assume this is the script that you said was just for demonstration purposes? ::: browser/components/preferences/in-content/main.js:124 (Diff revision 4) > + //document.getElementById("useBookmark").setAttribute("subDialog","true"); > setEventListener("restoreDefaultHomePage", "command", > gMainPane.restoreDefaultHomePage); > setEventListener("chooseLanguage", "command", > gMainPane.showLanguages); > + //document.getElementById("chooseLanguage").setAttribute("subDialog","true"); > setEventListener("translationAttributionImage", "click", > gMainPane.openTranslationProviderAttribution); > setEventListener("translateButton", "command", > gMainPane.showTranslationExceptions); > + //document.getElementById("translateButton").setAttribute("subDialog","true"); > setEventListener("font.language.group", "change", > gMainPane._rebuildFonts); > setEventListener("advancedFonts", "command", > gMainPane.configureFonts); > + //document.getElementById("advancedFonts").setAttribute("subDialog","true"); > setEventListener("colors", "command", > gMainPane.configureColors); > + //document.getElementById("colors").setAttribute("subDialog","true"); Please remove this dead, commented out code. ::: browser/components/preferences/in-content/subdialogs.js:298 (Diff revision 4) > + console.log("open",gSearchResultsPane.dialogeInit); > + console.log("numb",gSearchResultsPane.totalSubDialogs,gSearchResultsPane.counterDialog); Please remove the logging. ::: browser/components/preferences/in-content/subdialogs.js:302 (Diff revision 4) > > + console.log("open",gSearchResultsPane.dialogeInit); > + console.log("numb",gSearchResultsPane.totalSubDialogs,gSearchResultsPane.counterDialog); > + if (gSearchResultsPane.totalSubDialogs == gSearchResultsPane.counterDialog){ > + gSearchResultsPane.highlightDialog(); > + } else if (gSearchResultsPane.dialogeInit) { This bool is never flipped to false, fwiw.

Hi Manotej, Are you still active to work on this bug? Thanks.

Hey Manotej - I'm going to unassign you from this one. A full-timer will drive this one through to a resolution. Thanks for your work!

Neeinfo myself, I will work on this once I have time.

After discussed with Jared and Mike, we will add a new element attribute for the buttons could open subdialogs to store keywords. Then we could search for the keywords. For example, ``` <button id="id" searchKeywords="&some.label;" /> ```

Comment on attachment 8866054 [details] Bug 1352481 - Add searchkeywords attributes for subdialog buttons and search the keywords in the searchWithinNode method. Hi Jared, This is a WIP patch. It can help us search for the color sub-dialog. Could you give feedbacks for it? If the approach is good, I'll continue to work on the rest of work to add the "searchKeywords" for all sub-dialog buttons. Thanks.

Comment on attachment 8866054 [details] Bug 1352481 - Add searchkeywords attributes for subdialog buttons and search the keywords in the searchWithinNode method. https://reviewboard.mozilla.org/r/137646/#review141100 Looks good, thanks for sending the WIP patch. ::: browser/components/preferences/in-content/findInPage.js:285 (Diff revision 2) > - if (nodeObject.getAttribute("label")) { > - labelResult = this.stringMatchesFilters(nodeObject.getAttribute("label"), searchPhrase); > + nodeKeywords = nodeObject.getAttribute("label"); > + if (nodeKeywords) { It looks like you made these changes to prevent us from calling getAttribute for the same value twice. This is not expensive so we shouldn't need to make this change for performance reasons. If you wanted to make this change to make the code easier to read, then we should probably change stringMatchesFilter to just return false if the string that is passed in is empty. ::: browser/components/preferences/in-content/findInPage.js:299 (Diff revision 2) > + } > + > + // Searching some elements, such as xul:button, buttons to open subdialogs. > + nodeKeywords = nodeObject.getAttribute("searchkeywords"); > + if (nodeKeywords) { > + valueResult = this.stringMatchesFilters(nodeKeywords, searchPhrase); This should not overwrite valueResult. ::: browser/components/preferences/in-content/main.xul:545 (Diff revision 2) > + searchkeywords="&overrideDefaultPageColors.label;, > + &overrideDefaultPageColors.always.label;, This looks correct but we should not have commas inserted here.

Comment on attachment 8866054 [details] Bug 1352481 - Add searchkeywords attributes for subdialog buttons and search the keywords in the searchWithinNode method. https://reviewboard.mozilla.org/r/137646/#review141256 ::: browser/components/preferences/in-content/findInPage.js:285 (Diff revision 2) > - if (nodeObject.getAttribute("label")) { > - labelResult = this.stringMatchesFilters(nodeObject.getAttribute("label"), searchPhrase); > + nodeKeywords = nodeObject.getAttribute("label"); > + if (nodeKeywords) { Sure, let's fix it. ::: browser/components/preferences/in-content/findInPage.js:299 (Diff revision 2) > + } > + > + // Searching some elements, such as xul:button, buttons to open subdialogs. > + nodeKeywords = nodeObject.getAttribute("searchkeywords"); > + if (nodeKeywords) { > + valueResult = this.stringMatchesFilters(nodeKeywords, searchPhrase); Sure, let's fix it. ::: browser/components/preferences/in-content/main.xul:545 (Diff revision 2) > + searchkeywords="&overrideDefaultPageColors.label;, > + &overrideDefaultPageColors.always.label;, Sure.

Comment on attachment 8866054 [details] Bug 1352481 - Add searchkeywords attributes for subdialog buttons and search the keywords in the searchWithinNode method. Hi Jared, Could you help review the patch? Thanks.

Comment on attachment 8866054 [details] Bug 1352481 - Add searchkeywords attributes for subdialog buttons and search the keywords in the searchWithinNode method. https://reviewboard.mozilla.org/r/137646/#review145190 r=me with the following issues fixed. ::: browser/components/preferences/in-content/findInPage.js:271 (Diff revision 17) > let labelResult = false; > let valueResult = false; > + let keywordsResult = false; Now that you have removed the if-blocks surrounding the calls to stringMatchesFilter, please move these variable declarations to be inline with the assignment of these variables. For example, let keywordsResult = this.stringMatchesFilter(...); ::: browser/components/preferences/in-content/privacy.js:153 (Diff revision 17) > function setEventListener(aId, aEventType, aCallback) { > document.getElementById(aId) > .addEventListener(aEventType, aCallback.bind(gPrivacyPane)); > - } > + }; > + > + function appendSearchKeywors(aId, keywords) { spelling error, appendSearchKeywords ::: browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences.js:5 (Diff revision 17) > +/* > +* This file contains tests for the Preferences search bar. > +*/ > + > +requestLongerTimeout(2); We should not be creating a new test that requires a longer timeout. Please split this test in to multiple tests so this longer timeout is not necessary. ::: browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences.js:16 (Diff revision 17) > + await openPreferencesViaOpenPreferencesAPI("paneGeneral", {leaveOpen: true}); > + > + evaluateSearchResults("Set Home Page", "startupGroup"); > + > + await BrowserTestUtils.removeTab(gBrowser.selectedTab); Please remove the blank lines between these since they are only one line each and the extra blank lines expand each function by 67%.

Comment on attachment 8866054 [details] Bug 1352481 - Add searchkeywords attributes for subdialog buttons and search the keywords in the searchWithinNode method. https://reviewboard.mozilla.org/r/137646/#review145864 ::: browser/components/preferences/in-content/findInPage.js:271 (Diff revision 17) > let labelResult = false; > let valueResult = false; > + let keywordsResult = false; Sure, let's do it. ::: browser/components/preferences/in-content/privacy.js:153 (Diff revision 17) > function setEventListener(aId, aEventType, aCallback) { > document.getElementById(aId) > .addEventListener(aEventType, aCallback.bind(gPrivacyPane)); > - } > + }; > + > + function appendSearchKeywors(aId, keywords) { Thanks. Updated it. ::: browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences.js:5 (Diff revision 17) > +/* > +* This file contains tests for the Preferences search bar. > +*/ > + > +requestLongerTimeout(2); Sure. Let's remove it. ::: browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences.js:16 (Diff revision 17) > + await openPreferencesViaOpenPreferencesAPI("paneGeneral", {leaveOpen: true}); > + > + evaluateSearchResults("Set Home Page", "startupGroup"); > + > + await BrowserTestUtils.removeTab(gBrowser.selectedTab); Sure, let's do it.

Updated patch for nits and review comments. Let's land the patch after the try[1] is good. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66d9d0da15ee

The try is failed because a timeout issue. > browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. Let's split up the test.

Finished spliting the test up. Let's land the patch after the try[1] is good. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee25c49589d9

The timeout issue still occurs. Splitted up the test to four test files. Let's if it will be OK.

Hi Sheriff, I would like to land the patch[1]. But there is an issue with it, which is that it still shows two open issues not fixed yet even though I already close all issues I can close. It shows two open issues at "Commit" section of the page[3] and shows no open issues at the "Issues" section. It's weird and conflicted. I would say it might be a bug of the review board. You can see the situation in the screenshot[2] or the review board page[3]. Could you help land the patch? Thanks. [1]: https://reviewboard.mozilla.org/r/137646/diff/20 [2]: https://bugzilla.mozilla.org/attachment.cgi?id=8871780 [3]: https://reviewboard.mozilla.org/r/137646/#issue-summary

smacleod, can you fix this review request?

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9239079ba11d Add searchkeywords attributes for subdialog buttons and search the keywords in the searchWithinNode method. r=jaws

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9bc84b9b18d8 Fix a few eslint errors related to unnecessary semicolon usage and incorrect quotes (should have used double quotes for string). r=me

https://hg.mozilla.org/mozilla-central/rev/9239079ba11d https://hg.mozilla.org/mozilla-central/rev/9bc84b9b18d8

Pushed by philringnalda@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ffbc221accd followup, set the preferences to enable the UI the tests search for, even on beta and release

(In reply to Mark Côté [:mcote] from comment #47) > smacleod, can you fix this review request? fixed.

Should the highlight be applied also on the pop-ups and subdialogs or just the tooltip pointing where the keyword could appear?

The hightlight and tooltip things for subdialogs will be fixed at Bug 1363721.

Build ID; 20170807113452 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 Verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.