1387624 - No datalists in WebExtensions inline options

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P3)

Description

[Martin Giger [:freaktechnik]]

Attachment: datalist-aom-wip.patch

Datalists can't be displayed in WebExtensions options, as the necessary popup and support scripts are not present.

From what I understand this would require an import of browser/content/content.js, which also adds some other features. Further a modification to AutoCompletePopup.jsm would be required to convince it, that the options browser is in the foreground.

I've attached a WIP patch that kind of makes datalists work in inline options, however it seems to use the URL bar auto complete popup style instead of the simple one that just displays the label of the option.

Not sure if this bug should be against the Add-ons manager, since the code handling the browser lives there, but I couldn't find a component for it.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8894001 [details] [diff] [review]
datalist-aom-wip.patch

>diff --git a/toolkit/components/satchel/AutoCompletePopup.jsm b/toolkit/components/satchel/AutoCompletePopup.jsm
>--- a/toolkit/components/satchel/AutoCompletePopup.jsm
>+++ b/toolkit/components/satchel/AutoCompletePopup.jsm
>@@ -141,21 +141,21 @@ this.AutoCompletePopup = {
>       // We shouldn't ever be showing an empty popup, and if we
>       // already have a popup open, the old one needs to close before
>       // we consider opening a new one.
>       return;
>     }
> 
>     let window = browser.ownerGlobal;
>     // Also check window top in case this is a sidebar.
>-    if (Services.focus.activeWindow !== window.top) {
>+    /*if (Services.focus.activeWindow !== window.top) {

I was the last to touch that line.  It used to be:

if (Services.focus.activeWindow != window ||
         tabbrowser.selectedBrowser != browser) {

Which of course wouldn't work for the sidebar.  Maybe it needs to be:

if (Services.focus.activeWindow !== window &&
    Services.focus.activeWindow !== window.top)

But I haven't tried it.

The rest of the patch looks reasonable though the popup issue would need to be figured out.

Comment 3

[Martin Giger [:freaktechnik]]

The styling issue is solved by adding chrome://browser/content/browser.css to the Add-on Manager (and it doesn't seem to break anything, but I haven't looked at every little bit of it).

However the focus check is harder, as the activeWindow is the top level ChromeWindow, while window.top is just the Window for the add-on manager, I think,it has gViewDefault set to "addons://discover/". I don't know if it's possible to get the ChromeWindow from the Add-on Manager window, or if it'd have to check against the current tab of the ChromeWindow instead.

Comment 4

[Shane Caraveo (:mixedpuppy)]

aswan might have some thoughts on the css issue.

I'm wondering if we can check the focusedElement instead of the window comparison, something like:

Services.focus.focusedElement.ownerDocument == browser.document

Comment 5

[Andrew Swan [:aswan]]

I don't have thoughts on the css issue, redirect to mark

Comment 6

[Mark Striemer [:mstriemer]]

I don't think I know enough about the browser CSS file to comment on this.

It seems like that file should be unrelated to how an HTML element is rendered on page but I could very well be misunderstanding the issue.

Comment 7

[Martin Giger [:freaktechnik]]

Attachment: Bug 1387624 - Fix datalist popups in extension options

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

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8925252 [details]
Bug 1387624 - Fix datalist popups in extension options

https://reviewboard.mozilla.org/r/196462/#review202038

::: toolkit/mozapps/extensions/content/extensions.xul:10
(Diff revision 1)
>  
>  <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
>  <?xml-stylesheet href="chrome://mozapps/content/extensions/extensions.css"?>
>  <?xml-stylesheet href="chrome://mozapps/skin/extensions/extensions.css"?>
> +<?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>

I'm fairly certain that adding these css files is ok, but I haven't really touched this file so I'd be more comfortable with a second look.  Given the hot potatoe comments on it, lets see of Mossop knows any issue.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 8925252 [details]
Bug 1387624 - Fix datalist popups in extension options

https://reviewboard.mozilla.org/r/196462/#review202044

There are a number of problems with doing this. The first is that the files you're including simpkly don't exist outside of Firefox. That is less of a concern than the fact that no-one making changes to those files is going to check that add-on options render correctly because it is extremely unobvious that they would have any effect there.

What is it specifically about these files that makes this all work?

Comment 10

[Martin Giger [:freaktechnik]]

Comment on attachment 8925252 [details]
Bug 1387624 - Fix datalist popups in extension options

https://reviewboard.mozilla.org/r/196462/#review202044

browser.css makes the popup look like a normal list, instead of like an awesomebar suggestion box. browser/skin adds hover styles for items etc.

Comment 11

[Dave Townsend [:mossop]]

(In reply to Martin Giger [:freaktechnik] from comment #10)
> Comment on attachment 8925252 [details]
> Bug 1387624 - Fix datalist popups in extension options
> 
> https://reviewboard.mozilla.org/r/196462/#review202044
> 
> browser.css makes the popup look like a normal list, instead of like an
> awesomebar suggestion box. browser/skin adds hover styles for items etc.

What in those files does that? Can it be split out?

Comment 12

[Martin Giger [:freaktechnik]]

(In reply to Dave Townsend [:mossop] from comment #11)
> (In reply to Martin Giger [:freaktechnik] from comment #10)
> > browser.css makes the popup look like a normal list, instead of like an
> > awesomebar suggestion box. browser/skin adds hover styles for items etc.
> 
> What in those files does that? Can it be split out?

I think the styles required from browser.css are limited to a single rule for hiding elements (https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/browser/base/content/browser.css#621-627 ).

The browser/skin one may have system dependent styles, though honestly we could skip those with the downside of not having mouse hover styles and potential other adjustments platform styles make to the autocomplete popup. I may be missing rules here, as I don't have a build with the patch applied right now (and also can't look for the specific files that have any effect).

Comment 13

[Shane Caraveo (:mixedpuppy)]

Sigh.  I totally overlooked the browser files in toolkit. :(

Anyway, I'm surprised that toolkit doesn't have the base datalist/autocomplete styling, given the changes from bug 1296638 affect toolkit.

Looking at https://hg.mozilla.org/mozilla-central/rev/e4e0419afca7 should look into whether the changes in shared/autocomplete.inc.css may also be handy to have in toolkit.

Comment 14

[Martin Giger [:freaktechnik]]

Comment on attachment 8925252 [details]
Bug 1387624 - Fix datalist popups in extension options

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

Comment 15

[Martin Giger [:freaktechnik]]

I've added the one critical bit of CSS from browser to the extensions.inc.css and removed the stylesheets from browser. This means the styling does not match other datalists, however the functionality is there.

I'd suggest opening a follow-up for improving the styles of these datalists, as I am not familiar with what that would entail.

Shane, does this need re-review?

Comment 16

[Martin Giger [:freaktechnik]]

Comment on attachment 8925252 [details]
Bug 1387624 - Fix datalist popups in extension options

https://reviewboard.mozilla.org/r/196462/#review216472

::: toolkit/themes/shared/extensions/extensions.inc.css:1075
(Diff revision 2)
> +#PopupAutoComplete richlistbox,
> +#PopupAutoComplete listbox {
> +  -moz-appearance: initial;
> +  margin-inline-start: 0;
> +  border: none;
> +  border-radius: none;

As per try run (https://treeherder.mozilla.org/#/jobs?repo=try&revision=743cbbc8b6cba60d39d552b832930ba558fb35af&selectedJob=154646337) this is not valid syntax.

Comment 17

[Shane Caraveo (:mixedpuppy)]

css addition is fine, carry forward the r+, given fixing the issue in comment 16.

Comment 18

[Martin Giger [:freaktechnik]]

Comment on attachment 8925252 [details]
Bug 1387624 - Fix datalist popups in extension options

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

Comment 19

[Martin Giger [:freaktechnik]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e4ed7978a1a32e9cb9965c413b89c31b49b85db&selectedJob=155426462

Comment 20

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

The issue has to be set as fixed to be able to land it with Review Board.

Comment 21

[Martin Giger [:freaktechnik]]

Oops, thank you Aryx.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c4243ce6d43
Fix datalist popups in extension options r=mixedpuppy

Comment 23

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/1c4243ce6d43

Comment 24

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 25

[Martin Giger [:freaktechnik]]

(In reply to marius.santa from comment #24)
> Is manual testing required on this bug? If Yes, please provide some STR and
> the proper webextension(if required), if No set the “qe-verify-“ flag.

I've tested this only on Linux and there are no automated tests, so I think this may be a good idea to verify.

 - Extension: https://addons.mozilla.org/en-US/firefox/addon/notification-sound/
 - Open options
 - Double click on the "Extension ID" field, or start typing "Live Stream Notifier"
 - Auto compelte suggestions should show up
 - When selecting a suggestion it should be replaced with an extension ID (mapping as listed below the field)

Comment 26

[marius.santa]

Attachment: nodatalist.gif

I can reproduce this issue on Firefox 57.0a1 (20170804193726) under Wind 10 64-bit. 
This issue is verified as fixed on Firefox 59.0a1 (20180122100120) under Wind 10 64-bit and Mac OS X 10.13.