1294366 - Filter editor tooltip: filter select can only be opened once

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P1)

Description

[Julian Descottes [:jdescottes]]

STRs :

- (Windows only)
- open any page, open the devtools, go to inspector
- add rule "filter: unset" on any element
- open the filter editor tooltip (click on the filter button in the rule)
- expand the select "Select a Filter"
- (the select's list is displayed)
- collapse the select
- expand the select "Select a Filter"

ER: The select's list should be displayed again
AR: The list can no longer be displayed

Regression from bug 1267414. Since the migration of the filter editor tooltip to use HTML, the tooltip's HTML content is directly embedded in a XUL Panel (since the filter editor tooltip uses the HTMLTooltip xulPanelWrapper option).

A quick look at the XUL Panel limitations [1], it looks like HTML <select> elements are not working properly in panels. Our options for fixing this are either to :
- 1: encapsulate the editor tooltip in an iframe
- 2: stop using a XUL panel wrapper for this tooltip

More in favor of 2, since the XUL panel wrapper is just supposed to be a temporary measure, and I think the filter editor is the one that benefits the less from the increased space provided by the XUL panel wrapper. 

[1] https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/panel#Panel_limitations

Comment 1

[Andrew Overholt [:overholt]]

This is tagged as a regression in 50. Is someone able to take a look?

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294366 - fix filter widget select opening only once, force focus on window on click;

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

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294366 - css filter tootlip: shorten empty presets helptext;

Use a shorter text that will have more chance to fit in the tooltip even
if the height is reduced. Using a separate commit here so that we can
easily uplift the first part to aurora without having to embed a string
change.

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

Comment 4

[Julian Descottes [:jdescottes]]

Having a look.

Comment 5

[Alexandre Poirot [:ochameau]]

> More in favor of 2, since the XUL panel wrapper is just supposed to be a
> temporary measure,

Are you sure about that? Aren't we going to keep using it whenever we are running on Firefox for real?

> and I think the filter editor is the one that benefits
> the less from the increased space provided by the XUL panel wrapper. 

I wouldn't say so. This tooltip is still big and doesn't easily fit in a toolbox with a default size.

What about a third option, very common when it comes to xul, a workaround?

It looks like there is a focus issue, the following thing brings the <select> alive on Windows on the given test case. Could you give that a try?

+    this.filterSelect.onmousedown = () => {
+      this.filterSelect.ownerDocument.defaultView.focus();
+    };

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> > More in favor of 2, since the XUL panel wrapper is just supposed to be a
> > temporary measure,
> 
> Are you sure about that? Aren't we going to keep using it whenever we are
> running on Firefox for real?

Due to the various limitations we found (mainly Bug 1285261), we cannot rely on XUL wrappers for all tooltips even when running in Firefox. We want to use them for the ruleview editor tooltips (which are quite big) while we are waiting for the inline editors to replace them. 

In the beginning I wanted to use them for all tooltips when running in Firefox, but since we can't do that the plan is to use XUL panels only to help us migrate quickly tooltips which are too big for the toolbox. Let me know if you want to discuss this in more details!

> 
> > and I think the filter editor is the one that benefits
> > the less from the increased space provided by the XUL panel wrapper. 
> 
> I wouldn't say so. This tooltip is still big and doesn't easily fit in a
> toolbox with a default size.

Since the tooltip is displaying a list of filters and presets, it can easily be resized vertically, which is not the case for the 2 other tooltips. That's why I think it doesn't need a XUL wrapper as much as the others.

Are you more concerned with the width or with the height of the tooltip? Since it has a fixed width of 510px, I guess this can be an issue for devtools docked to the side with a small width. I could rework the tooltip to no longer use a fixed-width? 

(Maybe it's worst on Linux because of the increased font-size?)

> 
> What about a third option, very common when it comes to xul, a workaround?
> 
> It looks like there is a focus issue, the following thing brings the
> <select> alive on Windows on the given test case. Could you give that a try?
> 
> +    this.filterSelect.onmousedown = () => {
> +      this.filterSelect.ownerDocument.defaultView.focus();
> +    };

Indeed this allows to re-open the select, but the panel still behaves strangely:
- clicking on the opened select makes the select flicker and reopen, instead of closing it
- if you close the select by clicking somewhere else in the tooltip, the focus is lost. You can still interact with inputs in the tooltip, but the :focus styling will not be applied, the caret will not be visible etc...

I managed to "fix" the second problem by attaching the listener on the whole tooltip (this.el). However for the first issue, unless I resort to very ugly hacks using setTimeout, I can't find a way to fix it for now. Let me know if you have any idea here.

As I said, we want to remove tooltips using the wrapper so I would prefer to go in this direction. But such a change is too much here, considering this needs to be uplifted. 

=> What about having first patch based on your workaround that we uplift here, and open another to redesign the filter widget to fit in a regular tooltip?

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #6)
> (In reply to Alexandre Poirot [:ochameau] from comment #5)
> > > More in favor of 2, since the XUL panel wrapper is just supposed to be a
> > > temporary measure,
> > 
> > Are you sure about that? Aren't we going to keep using it whenever we are
> > running on Firefox for real?
> 
> Due to the various limitations we found (mainly Bug 1285261), we cannot rely
> on XUL wrappers for all tooltips even when running in Firefox.

I wasn't aware of the other limitations.

> We want to
> use them for the ruleview editor tooltips (which are quite big) while we are
> waiting for the inline editors to replace them. 

It looks like it would be better time spent converting them as inline rather that doing all these tweaks.

> > 
> > > and I think the filter editor is the one that benefits
> > > the less from the increased space provided by the XUL panel wrapper. 
> > 
> > I wouldn't say so. This tooltip is still big and doesn't easily fit in a
> > toolbox with a default size.
> 
> Since the tooltip is displaying a list of filters and presets, it can easily
> be resized vertically, which is not the case for the 2 other tooltips.
> That's why I think it doesn't need a XUL wrapper as much as the others.
> 
> Are you more concerned with the width or with the height of the tooltip?

More about the height. That works fine for one or two filters, after that it starts to be painful as the scrollbar are small and scroll very fast.

> > 
> > What about a third option, very common when it comes to xul, a workaround?
> > 
> > It looks like there is a focus issue, the following thing brings the
> > <select> alive on Windows on the given test case. Could you give that a try?
> > 
> > +    this.filterSelect.onmousedown = () => {
> > +      this.filterSelect.ownerDocument.defaultView.focus();
> > +    };
> 
> Indeed this allows to re-open the select, but the panel still behaves
> strangely:

Here is a slightly better version, but there is still issues if you keep opening and closing the select by clicking on it:
  this.el.onmousedown = (event) => {
    if (event.target != this.filterSelect) {
      this.filterSelect.ownerDocument.defaultView.focus();
    }
  };

> => What about having first patch based on your workaround that we uplift
> here, and open another to redesign the filter widget to fit in a regular
> tooltip?

I would either keep the tooltip as-is and use cheap workarounds, or just convert them to be inline.
I don't see a lot of value in redesigning them while we know it would be better as inline.
Same thing applies to workarounds, if that takes too much time finding them, it would be better time spent converting them to inline.

I agree on uplifting quick fixes like my workaround or your tweaks.

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8781690 [details]
Bug 1294366 - fix filter widget select opening only once, force focus on window on click;

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

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8781690 [details]
Bug 1294366 - fix filter widget select opening only once, force focus on window on click;

https://reviewboard.mozilla.org/r/72042/#review71436

Thanks, works fine on linux and windows.

Comment 10

[Julian Descottes [:jdescottes]]

Thanks for the review! Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56be001f6378

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4c8109af287
fix filter widget select opening only once, force focus on window on click;r=ochameau

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b4c8109af287

Comment 13

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using Nightly 51.0a1 2016-08-29 under Win 10 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.9.5.

I noticed that under Win 10, clicking twice on the dropdown doesn't close it. This is not reproducible on other platforms and it happens on 48RC as well, so it's not a recent regression. Should I log this separately? Thank you!

Comment 14

[Julian Descottes [:jdescottes]]

(In reply to Petruta Rasa [QA] [:petruta] from comment #13)
> Verified as fixed using Nightly 51.0a1 2016-08-29 under Win 10 64-bit,
> Ubuntu 14.04 64-bit and Mac OS X 10.9.5.
> 
> I noticed that under Win 10, clicking twice on the dropdown doesn't close
> it. This is not reproducible on other platforms and it happens on 48RC as
> well, so it's not a recent regression. Should I log this separately? Thank
> you!

Thanks! Please log a separate bug here.
Also it doesn't seem related to devtools-html, so no need to tag it for devtools-html triage.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Julian, could you please nominate a patch for uplift to Fx50? Thanks!

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: bug1294366.aurora.patch

Sure! Here is the patch rebased on top of aurora.

Approval Request Comment
[Feature/regressing bug #]: 1267414
[User impact if declined]: Devtools tooltip to edit CSS filters is buggy for windows users
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37c5a0b386f6
[Risks and why]: small JS change: moving the focus when capturing a mousedown event, has been on central for some time, no issue raised.
[String/UUID change made/needed]: N/A

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8790459 [details] [diff] [review]
bug1294366.aurora.patch

Fixes a recent regression, was verified on Nightly, Aurora50+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8fe577bafb78