Bug 140664 - Web Inspector: Make collapsed sidebars undraggable
Summary: Web Inspector: Make collapsed sidebars undraggable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-01-19 21:22 PST by Nikita Vasilyev
Modified: 2015-02-10 15:27 PST (History)
9 users (show)

See Also:


Attachments
Animated GIF (205.51 KB, image/gif)
2015-01-19 21:22 PST, Nikita Vasilyev
no flags Details
Patch (2.94 KB, patch)
2015-02-03 16:00 PST, Nikita Vasilyev
timothy: review+
Details | Formatted Diff | Diff
Animated GIF with the patch applied (2.05 MB, image/gif)
2015-02-03 16:02 PST, Nikita Vasilyev
no flags Details
Animated GIF: Bug with `display:none` (311.38 KB, image/gif)
2015-02-08 07:28 PST, Nikita Vasilyev
no flags Details
Patch (using display: none) (4.72 KB, patch)
2015-02-08 07:43 PST, Nikita Vasilyev
timothy: review+
Details | Formatted Diff | Diff
Patch (using display: none) (3.97 KB, patch)
2015-02-08 19:11 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2015-01-19 21:22:30 PST
Created attachment 244966 [details]
Animated GIF

It's currently possible to grip on the invisible gutter on the right edge of the console and drag in an empty sidebar.
Comment 1 Radar WebKit Bug Importer 2015-01-19 21:22:41 PST
<rdar://problem/19528505>
Comment 2 Nikita Vasilyev 2015-02-03 16:00:40 PST
Created attachment 245983 [details]
Patch
Comment 3 Nikita Vasilyev 2015-02-03 16:02:04 PST
Created attachment 245984 [details]
Animated GIF with the patch applied
Comment 4 Timothy Hatcher 2015-02-04 13:47:44 PST
Comment on attachment 245983 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/Sidebar.css:83
> +    visibility: hidden;

Why visibility: hidden? display: none will prevent mouse events and other work.
Comment 5 Nikita Vasilyev 2015-02-08 07:28:37 PST
Created attachment 246241 [details]
Animated GIF: Bug with `display:none`

`.sidebar.collapsed {display: none}` introduces a bug, see the attached GIF.

offsetWidth and totalOffsetLeft properties are 0 while resizing:

    _resizerMouseMoved: function(event)
    {
        if (this._side === WebInspector.Sidebar.Sides.Left)
            var newWidth = event.pageX - this._element.totalOffsetLeft;
        else
            var newWidth = this._element.totalOffsetLeft + this._element.offsetWidth - event.pageX;
Comment 6 Nikita Vasilyev 2015-02-08 07:43:22 PST
Created attachment 246242 [details]
Patch (using display: none)
Comment 7 Timothy Hatcher 2015-02-08 09:37:45 PST
Comment on attachment 246242 [details]
Patch (using display: none)

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

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:54
> +    this._widthBeforeResize = 0;
> +    this._resizerMouseDownX = 0;

No need to create the properties here.

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:274
> +        this._widthBeforeResize = this.width;
> +        this._resizerMouseDownX = event.pageX;

They will be create as needed here.

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:305
> +        this._widthBeforeResize = 0;
> +        this._resizerMouseDownX = 0;

Nit: No need to reset them, they will be initialized again next time. I just like to delete ephemeral properties when done.
Comment 8 Nikita Vasilyev 2015-02-08 19:05:46 PST
(In reply to comment #7)
> Comment on attachment 246242 [details]
> Patch (using display: none)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246242&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:54
> > +    this._widthBeforeResize = 0;
> > +    this._resizerMouseDownX = 0;

A long while ago I read somewhere that it’s better to define all instance properties in constructor since it will allocate a memory during allocation and also make property access a tad faster. I don’t know if it’s still relevant or applicable to JavaScriptCore.
Comment 9 Nikita Vasilyev 2015-02-08 19:11:02 PST
Created attachment 246254 [details]
Patch (using display: none)
Comment 10 Nikita Vasilyev 2015-02-08 19:12:50 PST
(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 246242 [details]
> > Patch (using display: none)
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=246242&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:54
> > > +    this._widthBeforeResize = 0;
> > > +    this._resizerMouseDownX = 0;
> 
> A long while ago I read somewhere that it’s better to define all instance
> properties in constructor since it will allocate a memory during allocation
> and also make property access a tad faster. I don’t know if it’s still
> relevant or applicable to JavaScriptCore.

s/during allocation/during instantiation/
Comment 11 Brian Burg 2015-02-10 10:37:03 PST
(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Comment on attachment 246242 [details]
> > > Patch (using display: none)
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=246242&action=review
> > > 
> > > > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:54
> > > > +    this._widthBeforeResize = 0;
> > > > +    this._resizerMouseDownX = 0;
> > 
> > A long while ago I read somewhere that it’s better to define all instance
> > properties in constructor since it will allocate a memory during allocation
> > and also make property access a tad faster. I don’t know if it’s still
> > relevant or applicable to JavaScriptCore.
> 
> s/during allocation/during instantiation/

I think the rationale for current style is that it's easier to read. It may be marginally faster to unconditionally define the property, but the inspector's performance is 99% dominated by layout and painting. So I wouldn't clutter the constructors unless there's evidence it's a performance problem.
Comment 12 WebKit Commit Bot 2015-02-10 15:27:30 PST
Comment on attachment 246254 [details]
Patch (using display: none)

Clearing flags on attachment: 246254

Committed r179889: <http://trac.webkit.org/changeset/179889>
Comment 13 WebKit Commit Bot 2015-02-10 15:27:35 PST
All reviewed patches have been landed.  Closing bug.