Closed
Bug 1366148
Opened 8 years ago
Closed 8 years ago
xul:menulist provide the ability to get access to the textNode that displays the resulting text
Categories
(Toolkit :: XUL Widgets, defect, P1)
Toolkit
XUL Widgets
Tracking
()
VERIFIED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | verified |
People
(Reporter: rickychien, Assigned: rickychien)
References
Details
(Whiteboard: [photon-preference] )
Attachments
(4 files)
As same as bug 1340643, we should provide the same ability to make xul:menulist highlightable.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: P2 → P1
Updated•8 years ago
|
QA Contact: hani.yacoub
Updated•8 years ago
|
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Target Milestone: --- → mozilla55
Comment 2•8 years ago
|
||
mozreview-review |
Comment on attachment 8870368 [details]
Bug 1366148 - Making menulist and menupopup highlightable
https://reviewboard.mozilla.org/r/141818/#review146000
You must request review from Neal Deakin for the changes to xul.css (see top of that file). All other changes look fine to me.
::: browser/components/preferences/in-content/findInPage.js:273
(Diff revision 1)
> * @returns boolean
> * Returns true when found in at least one childNode, false otherwise
> */
> searchWithinNode(nodeObject, searchPhrase) {
> let matchesFound = false;
> - if (nodeObject.childElementCount == 0) {
> + if (nodeObject.childElementCount === 0 || nodeObject.tagName === "menulist") {
Please leave these as double-equals. We don't need to use triple-equals here as childElementCount will always return a Number and tagName will always return a String.
::: browser/components/preferences/in-content/findInPage.js:313
(Diff revision 1)
> // Searching some elements, such as xul:label, store their user-visible text in a "value" attribute.
> if (nodeObject.getAttribute("value")) {
> valueResult = this.stringMatchesFilters(nodeObject.getAttribute("value"), searchPhrase);
> }
>
> - if (nodeObject.tagName == "button" && (labelResult || valueResult)) {
> + if ((nodeObject.tagName === "button" || nodeObject.tagName === "menulist" || nodeObject.tagName === "menuitem") &&
double-equals here instead of triple-equals
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8870368 [details]
Bug 1366148 - Making menulist and menupopup highlightable
Jared, thanks for your review feedback! I think r? you is the best answer.
Btw, I cannot ni, f?, r? Neal Deakin (not available until Aug 9). Is there any person is able to review xul.css change? The change in xul.css is pretty similar to bug 1340643, hopefully it's safe enough and won't cause unexpected behavior.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
Mossop, as mentioned on comment 4. Neal Deakin is unavailable to review xul.css [1] during this time. Could you help us find a reviewer for reviewing the changes in xul.css?
Thanks!
[1] http://searchfox.org/mozilla-central/source/toolkit/content/xul.css#18-19
Flags: needinfo?(dtownsend)
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8870368 [details]
Bug 1366148 - Making menulist and menupopup highlightable
https://reviewboard.mozilla.org/r/141818/#review146488
Please do a try-push with and without your patch to request all Talos jobs and that they run 5 times so we get enough samples to compare them. You can use the following try syntax:
> try: -b do -p linux,macosx64,win32 -u none -t all --rebuild-talos 5
After both have finished running, you should use https://treeherder.mozilla.org/perf.html#/comparechooser to compare the before and after try pushes to make sure that we don't have any notable performance regressions since this will add another DOM element to all menulists and menuitems. You can see that a similar run was done in bug 1340643.
Attachment #8870368 -
Flags: review?(jaws) → review+
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8870368 [details]
Bug 1366148 - Making menulist and menupopup highlightable
https://reviewboard.mozilla.org/r/141818/#review146558
::: toolkit/content/xul.css:1228
(Diff revision 3)
> +.menulist-label:not([highlightable="true"]),
> +.menulist-label[highlightable="true"],
Doesn't this always hide the menulist-label?
Assignee | ||
Comment 9•8 years ago
|
||
Talos on Try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=312ea98f8d7f7152acee2d496717869c9b34650e
Comparison between this patch and without applying the patch
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=35d1bf2c7180&newProject=try&newRevision=312ea98f8d7f&framework=1&showOnlyImportant=0
I can't see any yellow or red in the perf report.
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8870368 [details]
Bug 1366148 - Making menulist and menupopup highlightable
https://reviewboard.mozilla.org/r/141818/#review147002
r+ for the xul.css change.
Attachment #8870368 -
Flags: review?(dtownsend) → review+
Updated•8 years ago
|
Flags: needinfo?(dtownsend)
Comment 12•8 years ago
|
||
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf1c34370ad8
Making menulist and menupopup highlightable r=jaws,mossop
Comment 13•8 years ago
|
||
bugherder |
Comment 14•8 years ago
|
||
Build ID: 20170530030204
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 15•7 years ago
|
||
Hani,
Could you help us confirm again menulist should be able to highlight?
e.g. search "yahoo". "Yahoo" keyword in Default Search Engine's menulist should be highlighted as expected. I'm afraid that there is a regression recently because I saw highlight has gone.
If it's a regression, please don't be hesitated to report this bug. Thanks
Flags: needinfo?(hani.yacoub)
Comment 16•7 years ago
|
||
I thought the highlight should be as in the attached screenshot, but from what you wrote in the previous comment, I understood that also "yahoo" keyword should be highlighted in the menu list which I did not verify this bug based on that.
Can you please tell me which one of the scenarios is the expected one?
Flags: needinfo?(hani.yacoub) → needinfo?(rchien)
Assignee | ||
Comment 17•7 years ago
|
||
Flags: needinfo?(rchien)
Assignee | ||
Comment 18•7 years ago
|
||
Assignee | ||
Comment 19•7 years ago
|
||
Ah, your attachment was wrong result that we unexpected :(
Please see my attachment. Every single inner item within menulist should be highlightable (search-yahoo.png) and even the default selected item should be highlighted as well (search-google.png).
Comment 20•7 years ago
|
||
Sorry for that, I totally misunderstood what the fix should look like.
Should I log a bug to report this problem? or it could be fixed in this bug?
Flags: needinfo?(rchien)
You need to log in
before you can comment on or make changes to this bug.
Description
•