Closed Bug 1352481 Opened 2 years ago Closed 2 years ago

Search should find matched strings within popups and subdialogs

Categories

(Firefox :: Preferences, enhancement, P1)

52 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox55 --- fixed
firefox57 --- verified

People

(Reporter: jaws, Assigned: evanxd)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference] )

Attachments

(3 files, 2 obsolete files)

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.
Attachment #8853797 - Flags: review?(mconley) → review-
Assignee: nobody → manotejmeka
Status: NEW → ASSIGNED
Attachment #8853797 - Attachment is obsolete: true
Attachment #8853797 - Flags: review?(mconley)
Attachment #8857783 - Attachment is obsolete: true
Blocks: 1357285
Whiteboard: [photon-preference]
Target Milestone: --- → Firefox 55
QA Contact: hani.yacoub
Flags: qe-verify+
Blocks: 1357130
No longer blocks: 1357285
Hi Manotej,

Are you still active to work on this bug?

Thanks.
Flags: needinfo?(manotejmeka)
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!
Assignee: manotejmeka → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(manotejmeka)
Priority: -- → P2
Target Milestone: Firefox 55 → Firefox 56
Neeinfo myself, I will work on this once I have time.
Flags: needinfo?(evan)
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;" />
```
Assignee: nobody → evan
Status: NEW → ASSIGNED
Flags: needinfo?(evan)
Priority: P2 → P1
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.
Attachment #8866054 - Flags: feedback?(jaws)
Attachment #8866054 - Flags: feedback?(jaws)
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.
Attachment #8866054 - Flags: feedback?(jaws) → feedback+
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.
Duplicate of this bug: 1364835
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.
Attachment #8866054 - Flags: review?(jaws)
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%.
Attachment #8866054 - Flags: review?(jaws) → review+
Blocks: 1357285
No longer blocks: 1357130
Blocks: 1367322
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
Blocks: 1365847
The timeout issue still occurs. Splitted up the test to four test files. Let's if it will be OK.
Attached image review-board-bug.png
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
Keywords: checkin-needed
smacleod, can you fix this review request?
Flags: needinfo?(smacleod)
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
Keywords: checkin-needed
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
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.
Flags: needinfo?(smacleod)
Should the highlight be applied also on the pop-ups and subdialogs or just the tooltip pointing where the keyword could appear?
Flags: needinfo?(evan)
The hightlight and tooltip things for subdialogs will be fixed at Bug 1363721.
Flags: needinfo?(evan)
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.