Bug 166935 - Web Inspector: popovers shouldn't be dismissed when Web Inspector window is dragged
Summary: Web Inspector: popovers shouldn't be dismissed when Web Inspector window is d...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-11 10:52 PST by BJ Burg
Modified: 2017-02-16 02:19 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.23 KB, patch)
2017-02-15 15:14 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (7.08 KB, patch)
2017-02-15 17:07 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-01-11 10:52:21 PST
This is supremely annoying if you are using a CSS property editor and lose your changes.
Comment 1 Radar WebKit Bug Importer 2017-01-11 11:19:03 PST
<rdar://problem/29972986>
Comment 2 Devin Rousso 2017-02-15 15:14:54 PST
Created attachment 301661 [details]
Patch
Comment 3 Devin Rousso 2017-02-15 15:23:49 PST
(In reply to comment #2)
> Created attachment 301661 [details]
> Patch

I tested this on <https://webkit.org>.  For each of the following, find a color inline swatch somewhere in the CSS - Rules sidebar and open it before doing what's listed:

- Click one of the dock configuration buttons (detach, dock right, dock bottom)
- When undocked, resize the inspector window from any side
- When undocked, drag the inspector window from empty space in the toolbar area
- When docked right, drag the resizer on the left side of the inspector window
- When docked bottom, drag the resizer on the top side of the inspector window
- Pressing ⌘⇧D to toggle to the last docked configuration

None of the above should dismiss the color picker popover.
Comment 4 Devin Rousso 2017-02-15 17:07:06 PST
Created attachment 301676 [details]
Patch
Comment 5 Joseph Pecoraro 2017-02-15 18:22:11 PST
Comment on attachment 301676 [details]
Patch

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

r=me

You mentioned:

- When docked right, drag the resizer on the left side of the inspector window
- When docked bottom, drag the resizer on the top side of the inspector window
- Pressing ⌘⇧D to toggle to the last docked configuration

I'd actually expect resizes to dismiss or maybe redraw / reposition the popover. Is that still the case?

> Source/WebInspectorUI/UserInterface/Views/Popover.js:173
> -            if (!this._element.contains(event.target) && !event.target.enclosingNodeOrSelfWithClass(WebInspector.Popover.IgnoreAutoDismissClassName))
> +            if (!this._element.contains(event.target) && !event.target.enclosingNodeOrSelfWithClass(WebInspector.Popover.IgnoreAutoDismissClassName)
> +                && !event[WebInspector.Popover.EventPreventDismissSymbol]) {
>                  this.dismiss();
> +            }

Style: I think all on one line is fine, or breaking it up on multiple lines:

    if (this._element.contains(event.target))
        break;
    if (event.target.enclosingNodeOrSelfWithClass(WebInspector.Popover.IgnoreAutoDismissClassName))
        break;
    if (event[WebInspector.Popover.EventPreventDismissSymbol])
        break;
    this.dismiss();
    break;
Comment 6 BJ Burg 2017-02-15 22:38:06 PST
(In reply to comment #5)
> Comment on attachment 301676 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301676&action=review
> 
> r=me
> 
> You mentioned:
> 
> - When docked right, drag the resizer on the left side of the inspector
> window
> - When docked bottom, drag the resizer on the top side of the inspector
> window
> - Pressing ⌘⇧D to toggle to the last docked configuration
> 
> I'd actually expect resizes to dismiss or maybe redraw / reposition the
> popover. Is that still the case?

Note that this bug (per the title) is about dragging. For resizing, I think dismissal is fine.
Comment 7 Joseph Pecoraro 2017-02-15 22:41:08 PST
We should match system behavior. System behavior doesn't dismiss popovers (though it does allow repositioning them to more optimal positions). That was our existing behavior and it seems that is what Devin says still happens. I just wanted to make sure that was still the case.
Comment 8 Devin Rousso 2017-02-15 23:56:18 PST
Comment on attachment 301676 [details]
Patch

(In reply to comment #7)
> We should match system behavior. System behavior doesn't dismiss popovers
> (though it does allow repositioning them to more optimal positions). That
> was our existing behavior and it seems that is what Devin says still
> happens. I just wanted to make sure that was still the case.

So it was already the case that resizing when undocked would not dismiss, but instead just reposition the popover.  I have extended this to also work when docked (both right and bottom), meaning that whenever the window is moved (when undocked) or resized the popover just recalculates its position.
Comment 9 WebKit Commit Bot 2017-02-16 02:19:39 PST
Comment on attachment 301676 [details]
Patch

Clearing flags on attachment: 301676

Committed r212427: <http://trac.webkit.org/changeset/212427>
Comment 10 WebKit Commit Bot 2017-02-16 02:19:45 PST
All reviewed patches have been landed.  Closing bug.