Filter editor tooltip: filter select can only be opened once

VERIFIED FIXED in Firefox 50

Status

defect
P1
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

({regression})

Trunk
Firefox 51
Unspecified
Windows
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed, firefox51 verified)

Details

(Whiteboard: [reserve-html])

Attachments

(2 attachments, 1 obsolete attachment)

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
Keywords: regression
Priority: -- → P2
Whiteboard: [devtools-html] [triage]
Flags: qe-verify?
Whiteboard: [devtools-html] [triage] → [reserve-html]
This is tagged as a regression in 50. Is someone able to take a look?
Flags: needinfo?(jdescottes)
Having a look.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Flags: needinfo?(jdescottes)
> 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();
+    };
(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?
Flags: needinfo?(poirot.alex)
(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.
Flags: needinfo?(poirot.alex)
Attachment #8781692 - Attachment is obsolete: true
Attachment #8781692 - Flags: review?(poirot.alex)
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.
Attachment #8781690 - Flags: review?(poirot.alex) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4c8109af287
fix filter widget select opening only once, force focus on window on click;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/b4c8109af287
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Iteration: --- → 51.2 - Aug 29
Flags: qe-verify? → qe-verify+
Priority: P2 → P1
QA Contact: petruta.rasa
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!
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+ → needinfo?(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.
Flags: needinfo?(jdescottes)
Hi Julian, could you please nominate a patch for uplift to Fx50? Thanks!
Flags: needinfo?(jdescottes)
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
Flags: needinfo?(jdescottes)
Attachment #8790459 - Flags: review+
Attachment #8790459 - Flags: approval-mozilla-aurora?
Comment on attachment 8790459 [details] [diff] [review]
bug1294366.aurora.patch

Fixes a recent regression, was verified on Nightly, Aurora50+
Attachment #8790459 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.