Bug 152369 – Web Inspector: REGRESSION (r193913): Popover covers completion suggestions

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Created attachment 267514 [details]
[Screenshot] Hidden completion suggestions

* SUMMARY
Popover covers completion suggestions. This occurs because --z-index-popover is higher than --z-index-tooltip, which is used by the popover's completion suggestion view.

Regressed in http://trac.webkit.org/changeset/193913.

[Matt Baker]

Note that the fix should bump the z-index only for CompletionSuggestionsViews that are created from a popover.

[Matt Baker]

Created attachment 267519 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Comment on attachment 267519 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=267519&action=review

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:50
> +                this._suggestionsView.element.classList.add("popover");

I suspect this can be inferred from element hierarchy in the CSS selector.

.popover .completion-suggestions

[Timothy Hatcher]

Comment on attachment 267519 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=267519&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:50
>> +                this._suggestionsView.element.classList.add("popover");
> 
> I suspect this can be inferred from element hierarchy in the CSS selector.
> 
> .popover .completion-suggestions

Also .popover also likely conflicts and inherits styles form the real popover class.

[Nikita Vasilyev]

Would anything break if we simply swap --z-index-tooltip and --z-index-popover, e.g.:

    --z-index-popover: 512;
    --z-index-tooltip: 1024;

?

[Matt Baker]

Created attachment 267521 [details]
[Patch] Proposed Fix

[Nikita Vasilyev]

Comment on attachment 267521 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=267521&action=review

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.css:33
> +    z-index: calc(var(--z-index-popover) + var(--z-index-tooltip));

This is the only place where --z-index-tooltip is used. It would make more sense to simply swap --z-index-popover and --z-index-tooltip in Variables.css.

[Matt Baker]

(In reply to comment #7)
> Comment on attachment 267521 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267521&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.css:33
> > +    z-index: calc(var(--z-index-popover) + var(--z-index-tooltip));
> 
> This is the only place where --z-index-tooltip is used. It would make more
> sense to simply swap --z-index-popover and --z-index-tooltip in
> Variables.css.

Nice and simple! A reordering of the z-index variables sounds like the best solution. Since only one tooltip is visible at a time, and popups are modal views that can have tooltips, I suggest the following order:

--z-index-glass-pane-for-drag: 512;
--z-index-uncaught-exception-sheet: 1024;
--z-index-popover: 2048;
--z-index-tooltip: 4096;

As an aside, is there a reason why --z-index-uncaught-exception-sheet has a higher priority than --z-index-glass-pane-for-drag?

[Nikita Vasilyev]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 267521 [details]
> > [Patch] Proposed Fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=267521&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.css:33
> > > +    z-index: calc(var(--z-index-popover) + var(--z-index-tooltip));
> > 
> > This is the only place where --z-index-tooltip is used. It would make more
> > sense to simply swap --z-index-popover and --z-index-tooltip in
> > Variables.css.
> 
> Nice and simple! A reordering of the z-index variables sounds like the best
> solution. Since only one tooltip is visible at a time, and popups are modal
> views that can have tooltips, I suggest the following order:
> 
> --z-index-glass-pane-for-drag: 512;
> --z-index-uncaught-exception-sheet: 1024;
> --z-index-popover: 2048;
> --z-index-tooltip: 4096;
> 
> As an aside, is there a reason why --z-index-uncaught-exception-sheet has a
> higher priority than --z-index-glass-pane-for-drag?

--z-index-uncaught-exception-sheet is for the error page Brian recently added. It should have the highest priority as it can show up on an exception at any state.

--z-index-glass-pane-for-drag is for an invisible element that covers the whole inspector. It gets added to the DOM when dragging (e.g. sidebar resizing) starts. The whole point of it is to silence all other mouse events that could have fired on other elements while resizing. It should be on top of --z-index-popover and --z-index-tooltip.

I suggest to only swap --z-index-popover and --z-index-tooltip, and keep the rest untouched.

[Matt Baker]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Comment on attachment 267521 [details]
> > > [Patch] Proposed Fix
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=267521&action=review
> > > 
> > > > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.css:33
> > > > +    z-index: calc(var(--z-index-popover) + var(--z-index-tooltip));
> > > 
> > > This is the only place where --z-index-tooltip is used. It would make more
> > > sense to simply swap --z-index-popover and --z-index-tooltip in
> > > Variables.css.
> > 
> > Nice and simple! A reordering of the z-index variables sounds like the best
> > solution. Since only one tooltip is visible at a time, and popups are modal
> > views that can have tooltips, I suggest the following order:
> > 
> > --z-index-glass-pane-for-drag: 512;
> > --z-index-uncaught-exception-sheet: 1024;
> > --z-index-popover: 2048;
> > --z-index-tooltip: 4096;
> > 
> > As an aside, is there a reason why --z-index-uncaught-exception-sheet has a
> > higher priority than --z-index-glass-pane-for-drag?
> 
> --z-index-uncaught-exception-sheet is for the error page Brian recently
> added. It should have the highest priority as it can show up on an exception
> at any state.
> 
> --z-index-glass-pane-for-drag is for an invisible element that covers the
> whole inspector. It gets added to the DOM when dragging (e.g. sidebar
> resizing) starts. The whole point of it is to silence all other mouse events
> that could have fired on other elements while resizing. It should be on top
> of --z-index-popover and --z-index-tooltip.
> 
> I suggest to only swap --z-index-popover and --z-index-tooltip, and keep the
> rest untouched.

Sounds good!

[Matt Baker]

Created attachment 267539 [details]
[Patch] Proposed Fix

[Nikita Vasilyev]

(In reply to comment #11)
> Created attachment 267539 [details]
> [Patch] Proposed Fix

Looks good to me, but I'm not a reviewer.

[WebKit Commit Bot]

Comment on attachment 267539 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 267539

Committed r194214: <http://trac.webkit.org/changeset/194214>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.