The fix made for bug 1169704 does more troublesome for extension developers to expand the search pane of about:preferences; the details are in bug 1169704, comment 17.

Carlos, would you like to write a patch for this? I can help you out if needed.

I can give to it a try. However I'm reading here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build that it's not possible anymore to build Firefox in a 32-bit system. But, as the expected patch is very short, I guess that I could make it from source without compiling it locally.

With an artifact build you might not need to install a compiler, https://developer.mozilla.org/en-US/docs/Artifact_builds However, I've gone ahead and put up a quick patch for this bug. If you would like to fix some other bugs let me know and I'll see what I can find for you :)

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4) > With an artifact build you might not need to install a compiler, > https://developer.mozilla.org/en-US/docs/Artifact_builds Interesting. > However, I've gone ahead and put up a quick patch for this bug. If you would > like to fix some other bugs let me know and I'll see what I can find for you > :) Sure, I will try in the future.

Comment on attachment 8780736 [details] Bug 1294531 - Use a className instead of an ID to alter the state of the search engine list to allow add-ons to add their own buttons to the page and choose the behavior that they want. https://reviewboard.mozilla.org/r/71376/#review68946 ::: browser/components/preferences/in-content/search.js:114 (Diff revision 1) > - if (aEvent.target.id != "engineChildren" && aEvent.target.id != "removeEngineButton") { > + if (aEvent.target.id != "engineChildren" && > + !aEvent.target.classList.contains("searchEngineAction")) { > let engineList = document.getElementById("engineList"); > // We don't want to toggle off selection while editing keyword > // so proceed only when the input field is hidden > - if (engineList.inputField.hidden) { > + if (engineList.inputField.hidden && engineList.view) { Why do we need the extra check for `engineList.view`, ie when is that falsy/null? ::: browser/components/preferences/in-content/search.js:534 (Diff revision 1) > - } else { > + } else if (orientation == nsITreeView.DROP_AFTER) { > - if (orientation == nsITreeView.DROP_AFTER) Should this have been in the "no lonely ifs" patch?

Comment on attachment 8780736 [details] Bug 1294531 - Use a className instead of an ID to alter the state of the search engine list to allow add-ons to add their own buttons to the page and choose the behavior that they want. https://reviewboard.mozilla.org/r/71376/#review68948

Comment on attachment 8780736 [details] Bug 1294531 - Use a className instead of an ID to alter the state of the search engine list to allow add-ons to add their own buttons to the page and choose the behavior that they want. https://reviewboard.mozilla.org/r/71376/#review68946 > Why do we need the extra check for `engineList.view`, ie when is that falsy/null? These event listeners are added when the Search pane is opened but aren't removed when a different pane is opened. When the search pane is hidden, the XBL binding for the engine gets destroyed and engineList.view is null. Ideally the better fix for this would be to stop treating the panes as separate pages code-wise and hook up event listeners for all widgets in a single place so it is obvious to see how they can get reached. We could also remove event listeners when panes are navigated, but if the long term is to treat the preferences as one page with built-in search than removing event listeners during pane navigation will become obsolete. > Should this have been in the "no lonely ifs" patch? Ideally yes, but it only shows up as an eslint failure by the eslint commit hook, which I believe looks at ignore files. /browser/components/preferences is currently in the ignore list, which is why this doesn't fail on infra.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8) > > Should this have been in the "no lonely ifs" patch? > > Ideally yes, but it only shows up as an eslint failure by the eslint commit > hook, which I believe looks at ignore files. /browser/components/preferences > is currently in the ignore list, which is why this doesn't fail on infra. I filed bug 1294989 to stop ignoring /browser/components/preferences and attached a patch there.

Comment on attachment 8780736 [details] Bug 1294531 - Use a className instead of an ID to alter the state of the search engine list to allow add-ons to add their own buttons to the page and choose the behavior that they want. https://reviewboard.mozilla.org/r/71376/#review68962 OK, thanks for the clarifications, r=me

Comment on attachment 8780736 [details] Bug 1294531 - Use a className instead of an ID to alter the state of the search engine list to allow add-ons to add their own buttons to the page and choose the behavior that they want. https://reviewboard.mozilla.org/r/71376/#review68946 > These event listeners are added when the Search pane is opened but aren't removed when a different pane is opened. When the search pane is hidden, the XBL binding for the engine gets destroyed and engineList.view is null. Ideally the better fix for this would be to stop treating the panes as separate pages code-wise and hook up event listeners for all widgets in a single place so it is obvious to see how they can get reached. > > We could also remove event listeners when panes are navigated, but if the long term is to treat the preferences as one page with built-in search than removing event listeners during pane navigation will become obsolete. Actually, second thought, might be worth a comment saying the listener is on <window> and the view might have been destroyed when the list got hidden? :-)

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4f30550d3edb Use a className instead of an ID to alter the state of the search engine list to allow add-ons to add their own buttons to the page and choose the behavior that they want. r=Gijs

I think that it can be safely pushed to Aurora too, given that bug 1169704 —blocked by it— was done for Fx50.

Jared, it's still possible to move it to current Aurora too? It would be a big relief for me: the add-on that I was talking while asking for this patch in bug 1169704 comment 17 is now in AMO: https://addons.mozilla.org/en-uS/firefox/addon/search-engines-export-import/ In fact the Export button doesn't work in 50.0a2; I have locally a ugly possible workaround but it doesn't work in all the cases yet (triggering the button via keys is bugging me).

Comment on attachment 8780736 [details] Bug 1294531 - Use a className instead of an ID to alter the state of the search engine list to allow add-ons to add their own buttons to the page and choose the behavior that they want. Approval Request Comment [Feature/regressing bug #]: bug 1169704 [User impact if declined]: add-on compat issue with https://addons.mozilla.org/en-uS/firefox/addon/search-engines-export-import/ [Describe test coverage new/current, TreeHerder]: manual testing, simple HTML change to use class attributes instead of ID attributes [Risks and why]: none expected [String/UUID change made/needed]: none

Hello Carlos, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment on attachment 8780736 [details] Bug 1294531 - Use a className instead of an ID to alter the state of the search engine list to allow add-ons to add their own buttons to the page and choose the behavior that they want. Fixes a recent regression (an addon compat issue), Aurora50+

(In reply to Ritu Kothari (:ritu) from comment #18) > Hello Carlos, could you please verify this issue is fixed as expected on a > latest Nightly build? Thanks! Yes, it's working as expected.

(In reply to Carlos [nohamelin] from comment #20) > (In reply to Ritu Kothari (:ritu) from comment #18) > > Hello Carlos, could you please verify this issue is fixed as expected on a > > latest Nightly build? Thanks! > > Yes, it's working as expected. Thanks.