Should highlight the sub-dialog items when do preferences search

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Preferences
P1
normal
VERIFIED FIXED
4 months ago
2 months ago

People

(Reporter: evanxd, Assigned: rickychien)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [photon-preference])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
Should highlight the sub-dialog items when do preferences search.
(Reporter)

Updated

4 months ago
Flags: qe-verify+
QA Contact: hani.yacoub

Updated

4 months ago
Status: NEW → ASSIGNED
Priority: P2 → P1
(Reporter)

Updated

4 months ago
Assignee: evan → nobody
Status: ASSIGNED → NEW

Updated

4 months ago
Priority: P1 → P2
(Assignee)

Updated

4 months ago
Depends on: 1352481
(Assignee)

Updated

3 months ago
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Priority: P2 → P1
(Reporter)

Updated

3 months ago
See Also: → bug 1368957
Depends on: 1369719
Comment hidden (mozreview-request)
(Assignee)

Comment 2

3 months ago
Summarize some changes in this patch:

* Each sub-panel is loaded within a XUL <browser> which is separate frame mechanism, so we have to modify all *.xul and ensure all of these labels are highlightable.

* Trigger searchWithinNode() in _onLoad to search and highlight all query keyword if matched.

* Change the way how we get nsISelectionController. After investigating, I found that nsISelectionController relies on document context. New structure will try to store and update subdialog's selection instance separately.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

3 months ago
Patch has rebased and test failures are fixed.
Sorry for the delay. I'm about half way through a review, should have it finished today.

Comment 8

3 months ago
mozreview-review
Comment on attachment 8874731 [details]
Bug 1363721 - Make about:preferences sub-dialog content highlightable

https://reviewboard.mozilla.org/r/146100/#review151854

Hey rickychien,

This looks really good, but I have a few questions / suggestions. See below! Thanks,

-Mike

::: commit-message-2c628:1
(Diff revision 4)
> +Bug 1363721 - Should highlight the sub-dialog content r?mconley

Can you update this commit message so that it's clear that you're referring to about:preferences search?

::: browser/components/preferences/in-content-new/findInPage.js:8
(Diff revision 4)
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* import-globals-from preferences.js */
>  
>  var gSearchResultsPane = {
> +  selections: {},

This doesn't appear to be used.

::: browser/components/preferences/in-content-new/findInPage.js:163
(Diff revision 4)
> +      if (this.subdialogFindSelection) {
> +        this.subdialogFindSelection.addRange(range);

This subdialogFindSelection thing seems a little awkward. Perhaps we should have `searchWithinNode` take a selection argument? That way, I think we don't have to keep it stashed on the gSearchResultsPane.

::: browser/components/preferences/in-content-new/findInPage.js:199
(Diff revision 4)
>     *
>     * @param String event
>     *    to search for filted query in
>     */
>    searchFunction(event) {
> -    let query = event.target.value.trim().toLowerCase();
> +    let query = this.query = event.target.value.trim().toLowerCase();

Might as well just have this be:

```js
this.query = event.target.value.trim().toLowerCase();
```

and use this.query instead of query down below.

::: browser/components/preferences/in-content-new/subdialogs.js:58
(Diff revision 4)
> +    this._frame = document.getElementById("dialogFrame");
> +    this._overlay = document.getElementById("dialogOverlay");
> +    this._box = document.getElementById("dialogBox");
> +    this._closeButton = document.getElementById("dialogClose");

I'm confused... this already happens in the constructor. Why do we need to do this again?
Attachment #8874731 - Flags: review?(mconley) → review-
(Assignee)

Comment 9

2 months ago
mozreview-review-reply
Comment on attachment 8874731 [details]
Bug 1363721 - Make about:preferences sub-dialog content highlightable

https://reviewboard.mozilla.org/r/146100/#review151854

> This subdialogFindSelection thing seems a little awkward. Perhaps we should have `searchWithinNode` take a selection argument? That way, I think we don't have to keep it stashed on the gSearchResultsPane.

IMO, it's necessary to store `findSelection` and `subdialogFindSelection` in gSearchResultsPane since `findSelection` holds the reference of all highlight selections in preferences.xul scope and `subdialogFindSelection` is for each sub-dialog xul which is loaded by a frame with a separate scope. 

So it make us possible to call `findSelection.removeAllRanges();` to remove highlight keywords in preferences.xul.
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 months ago
* Fixed all review issues.
* Supported showing search tooltip in sub-dialog
* Corrected the improper tooltip position.

In order to make tooltip float on menulist properly, the most easiest way is that to wrap menulist with hbox element so that those appended tooltips are able to layout by `display: -moz-box` and then ensure they float on top of the anchor element itself.
Comment hidden (mozreview-request)

Comment 13

2 months ago
mozreview-review-reply
Comment on attachment 8874731 [details]
Bug 1363721 - Make about:preferences sub-dialog content highlightable

https://reviewboard.mozilla.org/r/146100/#review151854

> IMO, it's necessary to store `findSelection` and `subdialogFindSelection` in gSearchResultsPane since `findSelection` holds the reference of all highlight selections in preferences.xul scope and `subdialogFindSelection` is for each sub-dialog xul which is loaded by a frame with a separate scope. 
> 
> So it make us possible to call `findSelection.removeAllRanges();` to remove highlight keywords in preferences.xul.

Correct me if I'm wrong, but `gSearchResultsPane.getFindSelection(win).getSelection(Ci.nsISelectionController.SELECTION_FIND);` will _always_ find the same Selection for a given window. So we might actually be able to get away with not storing either, but instead modifying `getFindSelection` to do the next step and call `getSelection(Ci.nsISelectionController.SELECTION_FIND)` for the selection controller it finds for the passed window.

Comment 14

2 months ago
mozreview-review
Comment on attachment 8874731 [details]
Bug 1363721 - Make about:preferences sub-dialog content highlightable

https://reviewboard.mozilla.org/r/146100/#review153608

Hey rickychien,

This looks great - I'm still concerned, however, of keeping the find selection for subdialogs on the gSearchResultsPane. I have an alternative - see below.

::: browser/components/preferences/in-content-new/findInPage.js:8
(Diff revision 6)
>    findSelection: null,
> +  subdialogFindSelection: null,

I suspect we can get away with not storing either of these references, but just having `getFindSelection` to return the right Selection for a given window. I think that simplifies things a bit.

::: browser/components/preferences/in-content-new/findInPage.js:166
(Diff revision 6)
> +      if (this.subdialogFindSelection) {
> +        this.subdialogFindSelection.addRange(range);
> +      }

Instead of doing this, what I suggest is to see if there is a `gSubDialog._topDialog`, and if so, get at its selection via `gSubDialog._topDialog._frame.contentWindow`, and add the range there.

Or maybe even better, delegate that to gSubDialog, or SubDialog.
Attachment #8874731 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 months ago
The idea of getting rid of selection references sounds good to me. I've revamped this part to remove selection references and pass window as a parameter in searchWithinNode().

Mike, please kindly check my patch again. Thank you very much!

Comment 17

2 months ago
mozreview-review
Comment on attachment 8874731 [details]
Bug 1363721 - Make about:preferences sub-dialog content highlightable

https://reviewboard.mozilla.org/r/146100/#review154008

Looks great, thanks! Just one little nit below that hopefully you can fix before landing.

::: browser/components/preferences/in-content-new/findInPage.js:356
(Diff revision 7)
> -  createSearchTooltip(currentNode, query) {
> -    let searchTooltip = document.createElement("span");
> +  createSearchTooltip(currentNode, query, doc = document) {
> +    currentNode.hasTooltip = true;

Can we not infer the document based on the currentNode? I think it's a pretty solid constraint that the currentNode and the tooltip will belong to the same document.

So you can probably get this via:

`let doc = currentNode.ownerDocument;`

and drop the extra argument (especially since it looks like that extra argument is not being used anywhere...)
Attachment #8874731 - Flags: review?(mconley) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 19

2 months ago
> So you can probably get this via:
> `let doc = currentNode.ownerDocument;`

This looks great! 

I even got rid of the way that additional `win` parameter in searchWithinNode() in latest amend. Accessing window instance from corresponded frame via node.ownerGlobal is much more straightforward.

Thanks for the review! Let's see what try say.

Comment 20

2 months ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28bddcbef8e1
Make about:preferences sub-dialog content highlightable r=mconley

Comment 21

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/28bddcbef8e1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED

Comment 22

2 months ago
Build ID: 20170618030207
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-firefox56: fixed → verified
You need to log in before you can comment on or make changes to this bug.