1450473 - Webextensions: Date input element does not work in desktop for browser_action, page_action and sidebar_action

Status: NEW

(WebExtensions :: Frontend, enhancement, P3)

Description

[ramkumar.kr94]

Attachment: date_works_-1.0.0.zip

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180326161258

Steps to reproduce:

Have a date input field on various views of a webextension namely
- browser_action
- page_action
- options_ui
- sidebar_action

A sample webextension is attached


Actual results:

On Firefox desktop:
The datepicker UI was not displayed when the date input was brought into focus in the following views
- browser_action
- page_action
- sidebar_action

However, the datepicker UI was visible in the options_ui page.

On  Firefox for Android:
All the views triggered the native Android datepicker UI.



Expected results:

The datepicker UI should have been displayed when the date input was brought into focus in the following views on firefox desktop
- browser_action
- page_action
- sidebar_action

Comment 1

[ramkumar.kr94]

Related discussion - https://discourse.mozilla.org/t/webextensions-date-input-element-in-browser-action-does-not-work-in-desktop/27114

Comment 2

[u554753]

Please correct if this is not the right component. Thanks.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1450473 - WIP - support datetimepicker in extension panels

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

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8965495 [details]
Bug 1450473 - WIP - support datetimepicker in extension panels

Getting it working in sidebar was easy enough, but I'm uncertain that we can have a panel in a panel.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8965495 [details]
Bug 1450473 - WIP - support datetimepicker in extension panels

https://reviewboard.mozilla.org/r/234266/#review240214

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> Comment on attachment 8965495 [details]
> Bug 1450473 - WIP - support datetimepicker in extension panels
> 
> Getting it working in sidebar was easy enough, but I'm uncertain that we can
> have a panel in a panel.


In theory, we can. Think also of how submenus for `<menupopup>` are basically just... more popups. So we can display more than 1. And we can display 1 on top of another 1. Whether it's easy to make this specific case work, that's a different question... Does it work with this patch? Maybe at least on Windows/Mac ? I think on Linux things may be more complicated...

::: browser/base/content/webext-panels.xul:8
(Diff revision 1)
> -<?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/content/usercontext/usercontext.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>

Reordering these sheets doesn't strike me as correct.

::: browser/base/content/webext-panels.xul:12
(Diff revision 1)
>  <!DOCTYPE page [
> -<!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd">
> +#include browser-doctype.inc
> -%browserDTD;
> -<!ENTITY % textcontextDTD SYSTEM "chrome://global/locale/textcontext.dtd">
> -%textcontextDTD;

Eep. why is this necessary?

::: browser/base/content/webext-panels.xul:59
(Diff revision 1)
> +   <!-- for date/time picker. consumeoutsideclicks is set to never, so that
> +        clicks on the anchored input box are never consumed. -->
> +    <panel id="DateTimePickerPanel"
> +           type="arrow"
> +           hidden="true"
> +           orient="vertical"
> +           noautofocus="true"
> +           norolluponanchor="true"
> +           consumeoutsideclicks="never"
> +           level="parent"
> +           tabspecific="true">
> +    </panel>
> +

Probably need this in the web panel as well?

::: browser/base/content/webext-panels.xul:69
(Diff revision 1)
> +           orient="vertical"
> +           noautofocus="true"
> +           norolluponanchor="true"
> +           consumeoutsideclicks="never"
> +           level="parent"
> +           tabspecific="true">

I don't think tabspecific is correct here?

::: toolkit/modules/DateTimePickerHelper.jsm:147
(Diff revision 1)
> -    if (Services.focus.activeWindow != window ||
> +    if ((Services.focus.activeWindow !== window.top &&
> +        Services.focus.focusedWindow.top !== window.top) ||

Why focusedWindow rather than activeWindow?

Comment 6

[:Gijs (he/him)]

Comment on attachment 8965495 [details]
Bug 1450473 - WIP - support datetimepicker in extension panels

We should definitely do this for sidebars. We should do it for panels if we can - maybe in a follow-up bug if it's more complex.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8965495 [details]
Bug 1450473 - WIP - support datetimepicker in extension panels

https://reviewboard.mozilla.org/r/234266/#review240214

> Reordering these sheets doesn't strike me as correct.

I reordered to how it was used elsewhere when I was trying to figure out why the panel was not binding.  It may not be necessary.

> Eep. why is this necessary?

There is some other dtd that is needed to make this work, I can narrow down later.

> Probably need this in the web panel as well?

It has not been made remote, so it might work already, not sure.

> I don't think tabspecific is correct here?

I haven't dug into the code using that to understand it yet, but it looked like it was being used to decide whether to close panels.  In that case we might actually want it.

> Why focusedWindow rather than activeWindow?

webext sidebar is in another xul window, this was a fix I applied elsewhere at some time for the same reason.  iirc activeWindow is the top level browser window but webext will be focused window.

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> Comment on attachment 8965495 [details]

> > Eep. why is this necessary?
> 
> There is some other dtd that is needed to make this work, I can narrow down
> later.

Verified that it is actually not necessary.

> > I don't think tabspecific is correct here?
> 
> I haven't dug into the code using that to understand it yet, but it looked
> like it was being used to decide whether to close panels.  In that case we
> might actually want it.

Without tabspecific, once datetime is opened in a regular tab, you can no longer open it in the sidebar.  e.g. open in sidebar, close, open in tab, close, open in sidebar fails.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8965495 [details]
Bug 1450473 - WIP - support datetimepicker in extension panels

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

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8965495 [details]
Bug 1450473 - WIP - support datetimepicker in extension panels

I was able to get this working in extension popups.

- I had to use level=top on the picker panel
- The panel will not close if I click outside it but in the parent panel (clicking outside both panels closes both)
- Had to add an exception for extension popups in datetimepicker.jsm
- Added a new binding rather than trying to manage changing "level" on the existing binding.

Some of these are not ideal, not sure if we should separate that part out and only land the sidebar, or if we can live with some of the off behavior.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8965495 [details]
Bug 1450473 - WIP - support datetimepicker in extension panels

https://reviewboard.mozilla.org/r/234266/#review241858

It might be worth splitting the panel fix into a separate commit. For the rest, I wonder if someone like mconley (who IIRC reviewed some of the datetimepicker stuff) or enndeakin (who knows all there is to know about popups) would have other ideas about how to handle the nested panel stuff.

> - The panel will not close if I click outside it but in the parent panel (clicking outside both panels closes both)

Maybe this is fixable by just listening for mousedown on the parent panel and check that it's not from inside the timepicker panel, or something?

Alternatively, I *think* it may be possible to avoid the level=top hack by making the datetimepicker panel itself a child of the popup in which you want to show it. I know that we've had to do this with context menu popups for some other panels, which wouldn't work right otherwise.

::: browser/base/content/webext-panels.xul:63
(Diff revision 2)
>  
>    <popupset id="mainPopupSet">
>      <tooltip id="aHTMLTooltip" page="true"/>
>  
> +   <!-- for date/time picker. consumeoutsideclicks is set to never, so that
> +        clicks on the anchored input box are never consumed. -->

I'm a little confused about why we need the consumeoutsideclicks thing here but not in the other popups. Can you elaborate?

::: browser/themes/shared/browser.inc.css:147
(Diff revision 2)
>  
>  /* Force background for datepicker popup to white so themes don't override it */
>  #DateTimePickerPanel[active="true"] > .panel-arrowcontainer > .panel-arrowbox,
> -#DateTimePickerPanel[active="true"] > .panel-arrowcontainer > .panel-arrowcontent {
> +#DateTimePickerPanel[active="true"] > .panel-arrowcontainer > .panel-arrowcontent,
> +#DateTimePickerSubPanel[active="true"] > .panel-arrowcontainer > .panel-arrowbox,
> +#DateTimePickerSubPanel[active="true"] > .panel-arrowcontainer > .panel-arrowcontent {

I wonder if this should be a class-based style instead.

::: toolkit/modules/DateTimePickerHelper.jsm:151
(Diff revision 2)
>      let tabbrowser = window.gBrowser;
> -    if (Services.focus.activeWindow != window ||
> -        tabbrowser.selectedBrowser != aBrowser) {
> +    // If the browser is an extension browser, let it through.
> +    let isExtensionPopup = !!aBrowser.getAttribute("webextension-view-type");
> +    if ((Services.focus.activeWindow !== window.top &&
> +        Services.focus.focusedWindow.top !== window.top) ||
> +        (!isExtensionPopup && tabbrowser.selectedBrowser != aBrowser)) {

So before, this only worked with tabbed browsers. I think for browsers that are really tabbed browsers, this check makes sense, but not for the other ones.

Instead of using `tabbrowser = window.gBrowser`, can we use `aBrowser.getTabBrowser()`, and just not check for the selected browser if there's no tabbrowser?

That seems like it would work better also for other browsers that aren't webextension browsers. For instance, AFAICT the current code will never work for non-webext sidebar browsers, because they are never the tabbrowser's selected browser anyway. It also avoids leaking webextension details into the datetimepicker implementation. How does that sound?

Comment 12

[:Gijs (he/him)]

Comment on attachment 8965495 [details]
Bug 1450473 - WIP - support datetimepicker in extension panels

See review comments. I think doing both in this bug is fine, but might be good to have 2 commits also for things like regression hunting. `hg commit --interactive` and such might help splitting it up.

Re:
(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> > > I don't think tabspecific is correct here?
> > 
> > I haven't dug into the code using that to understand it yet, but it looked
> > like it was being used to decide whether to close panels.  In that case we
> > might actually want it.
> 
> Without tabspecific, once datetime is opened in a regular tab, you can no
> longer open it in the sidebar.  e.g. open in sidebar, close, open in tab,
> close, open in sidebar fails.

I'm pretty confused by this. My understanding is that we close tabspecific panels when switching between tabs, and that otherwise the attribute does nothing. This is also why I didn't think we'd want it in the sidebar - if you switch tabs with ctrl/cmd-tab or whatever, the picker in the sidebar / add-on panel can in principle stay open. So I don't understand why that would make a difference to the STR you cite, but I also haven't had time to try it out myself. :-(

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs (he/him) from comment #11)

> Alternatively, I *think* it may be possible to avoid the level=top hack by
> making the datetimepicker panel itself a child of the popup in which you
> want to show it. I know that we've had to do this with context menu popups
> for some other panels, which wouldn't work right otherwise.

Currently the other popups are all defined in browser.xul and work fine in the page/browser action popups.  But I'm curious if there is something behind the scene that is reparenting those into panels.  If so, probably need to just do the same for datetimepicker.