Closed Bug 1294531 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Preferences, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 51
Tracking Status
firefox49 --- unaffected
firefox50 --- fixed
firefox51 --- verified

People

(Reporter: nohamelin, Assigned: jaws)

References

Details

(Keywords: addon-compat)

Attachments

(1 file)

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.
Blocks: 1169704
Keywords: addon-compat
Carlos, would you like to write a patch for this? I can help you out if needed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(nohamelin)
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.
Flags: needinfo?(nohamelin)
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 :)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Summary: Recent changes to about:preferences are unfriendly to extensions → 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
(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
Attachment #8780736 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8780736 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/4f30550d3edb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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).
Flags: needinfo?(jaws)
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
Flags: needinfo?(jaws)
Attachment #8780736 - Flags: approval-mozilla-aurora?
Hello Carlos, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(nohamelin)
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+
Attachment #8780736 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Flags: needinfo?(nohamelin)
(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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.