1366148 - xul:menulist provide the ability to get access to the textNode that displays the resulting text

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

As same as bug 1340643, we should provide the same ability to make xul:menulist highlightable.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1366148 - Making menulist and menupopup highlightable

Review commit: https://reviewboard.mozilla.org/r/141818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141818/

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8870368 [details]
Bug 1366148 - Making menulist and menupopup highlightable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141818/diff/1-2/

Comment 4

[Ricky Chien [:rickychien] (inactive)]

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 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8870368 [details]
Bug 1366148 - Making menulist and menupopup highlightable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141818/diff/2-3/

Comment 6

[Ricky Chien [:rickychien] (inactive)]

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

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 8

[Dave Townsend [:mossop]]

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?

Comment 9

[Ricky Chien [:rickychien] (inactive)]

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 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8870368 [details]
Bug 1366148 - Making menulist and menupopup highlightable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141818/diff/3-4/

Comment 11

[Dave Townsend [:mossop]]

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf1c34370ad8
Making menulist and menupopup highlightable r=jaws,mossop

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/bf1c34370ad8

Comment 14

[Hani Yacoub, Desktop QA]

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.

Comment 15

[Ricky Chien [:rickychien] (inactive)]

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

Comment 16

[Hani Yacoub, Desktop QA]

Attachment: menulist.png

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?

Comment 17

[Ricky Chien [:rickychien] (inactive)]

Attachment: search-yahoo.png

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Attachment: search-google.png

Comment 19

[Ricky Chien [:rickychien] (inactive)]

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

[Hani Yacoub, Desktop QA]

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?

Comment 21

[Ricky Chien [:rickychien] (inactive)]

You should go report a bug. Thanks