Closed Bug 1363721 Opened 7 years ago Closed 7 years ago

Should highlight the sub-dialog items when do preferences search

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: evanxd, Assigned: rickychien)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(1 file)

Bug 1363721 - Make about:preferences sub-dialog content highlightable
59 bytes, text/x-review-board-request
mconley
: review+
Details 
Should highlight the sub-dialog items when do preferences search.
Flags: qe-verify+
QA Contact: hani.yacoub
Status: NEW → ASSIGNED
Priority: P2 → P1
Assignee: evan → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2
Depends on: 1352481
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Priority: P2 → P1
See Also: → 1368957
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.
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 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-
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.
* 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 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 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-
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 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+
> 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.
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28bddcbef8e1
Make about:preferences sub-dialog content highlightable r=mconley
https://hg.mozilla.org/mozilla-central/rev/28bddcbef8e1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
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
Depends on: 1430718
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: