No datalists in WebExtensions inline options

VERIFIED FIXED in Firefox 59

Status

P3
normal
VERIFIED FIXED
2 years ago
9 months ago

People

(Reporter: freaktechnik, Assigned: freaktechnik)

Tracking

(Blocks: 1 bug)

unspecified
mozilla59

Firefox Tracking Flags

(firefox59 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Posted patch datalist-aom-wip.patch (obsolete) — Splinter Review
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 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.
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Priority: -- → P3
Duplicate of this bug: 1396640
(Assignee)

Comment 3

2 years ago
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.
Flags: needinfo?(mixedpuppy)
(Assignee)

Updated

2 years ago
Assignee: nobody → martin
Status: NEW → ASSIGNED
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
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(aswan)
I don't have thoughts on the css issue, redirect to mark
Flags: needinfo?(aswan) → needinfo?(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.
Flags: needinfo?(mstriemer)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8894001 - Attachment is obsolete: true

Comment 8

a year ago
mozreview-review
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.
Attachment #8925252 - Flags: review?(mixedpuppy) → review+
Attachment #8925252 - Flags: review?(dtownsend)

Comment 9

a year ago
mozreview-review
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?
Attachment #8925252 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 10

a year ago
mozreview-review-reply
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.
(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?
(Assignee)

Comment 12

a year ago
(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).
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 hidden (mozreview-request)
(Assignee)

Comment 15

a year ago
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?
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 16

a year ago
mozreview-review
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.
css addition is fine, carry forward the r+, given fixing the issue in comment 16.
Flags: needinfo?(mixedpuppy)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Keywords: checkin-needed
The issue has to be set as fixed to be able to land it with Review Board.
Flags: needinfo?(martin)
Keywords: checkin-needed
(Assignee)

Comment 21

a year ago
Oops, thank you Aryx.
Flags: needinfo?(martin)
Keywords: checkin-needed

Comment 22

a year ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/1c4243ce6d43
Fix datalist popups in extension options r=mixedpuppy
Keywords: checkin-needed

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1c4243ce6d43
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(Assignee)

Updated

a year ago
Blocks: 1430648

Comment 24

a year ago
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.
Flags: needinfo?(martin)
(Assignee)

Comment 25

a year ago
(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)
Flags: needinfo?(martin)

Comment 26

a year ago
Posted image 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.

Updated

a year ago
Status: RESOLVED → VERIFIED
status-firefox59: fixed → verified

Updated

9 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.