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)
Firefox
Settings UI
Tracking
()
VERIFIED
FIXED
Firefox 56
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)
Should highlight the sub-dialog items when do preferences search.
Reporter | ||
Updated•7 years ago
|
Flags: qe-verify+
QA Contact: hani.yacoub
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → P1
Reporter | ||
Updated•7 years ago
|
Assignee: evan → nobody
Status: ASSIGNED → NEW
Updated•7 years ago
|
Priority: P1 → P2
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years 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•7 years ago
|
||
Patch has rebased and test failures are fixed.
Comment 7•7 years ago
|
||
Sorry for the delay. I'm about half way through a review, should have it finished today.
Comment 8•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/28bddcbef8e1
Comment 22•7 years 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
You need to log in
before you can comment on or make changes to this bug.
Description
•