Bug 75454 – Web Inspector: Add utility changes for Spectrum colorpicker

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Brian Grinstead]

Patch to follow

[Brian Grinstead]

Created attachment 120902 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation

[Alexander Pavlov (apavlov)]

Comment on attachment 120902 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation

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

looks good but for the comments

> Source/WebCore/inspector/front-end/Popover.js:61
> +    

odd change

> Source/WebCore/inspector/front-end/Popover.js:93
> +        this._parentElement = parentElement;

Should this re-parent the popover when it is visible?

> Source/WebCore/inspector/front-end/utilities.js:916
> +function stopPropagation(e) {

Not sure if this is too useful. Pavel can comment on this

[Brian Grinstead]

(In reply to comment #2)

> > Source/WebCore/inspector/front-end/utilities.js:916
> > +function stopPropagation(e) {
> 
> Not sure if this is too useful. Pavel can comment on this

This is here because I was told to not use anonymous functions.  I had a need to stopPropagation on an event and I think it makes sense to have it as a utility function.  For example, this same function could be used inside of StylesSidebarPane.js instead of _handleSelectorClick.  I could always move this into a more local scope for the spectrum feature if needed.

[Timothy Hatcher]

Comment on attachment 120902 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation

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

>> Source/WebCore/inspector/front-end/utilities.js:916
>> +function stopPropagation(e) {
> 
> Not sure if this is too useful. Pavel can comment on this

I don't even see you calling this in your patch. What does this have to do with anonymous functions? Just locally call event.stopPropagation(). This is useless, please remove.

[Brian Grinstead]

(In reply to comment #4)
> (From update of attachment 120902 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120902&action=review
> 
> >> Source/WebCore/inspector/front-end/utilities.js:916
> >> +function stopPropagation(e) {
> > 
> > Not sure if this is too useful. Pavel can comment on this
> 
> I don't even see you calling this in your patch. What does this have to do with anonymous functions? Just locally call event.stopPropagation(). This is useless, please remove.

Please see this comment by Pavel Feldman: https://bugs.webkit.org/show_bug.cgi?id=71262#c31.  I don't call any of these changes in the patch.  He wanted me to include the utility functions as one patch and the code using it as another patch.  This is to make it easier to land all the changes.

I think it is useful to be able to replace this anonymous function:

element.addEventListener("click", function(e) {
   e.stopPropagation();
}, false);

With this:

element.addEventListener("click", stopPropagation, false);

I was told to not use anonymous functions in the web inspector code, that is why I created the helper function.  As I said, I can move this to a local scope and land it with the Spectrum changes, but there is at least one other place in code where this exact thing is done, so this could help with that as well.

[Pavel Feldman]

Comment on attachment 120902 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation

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

This change looks good to me. Please refactor totalOffsetTop/Left to use the new function and mark subsequent change as r?. Otherwise it does not end up on my to-review dashboard.

>> Source/WebCore/inspector/front-end/Popover.js:93
>> +        this._parentElement = parentElement;
> 
> Should this re-parent the popover when it is visible?

I don't think it should re-parent anything. This setter look good to me.

> Source/WebCore/inspector/front-end/utilities.js:284
> +Element.prototype.totalOffset = function(parent) 

You should change Element.prototype.totalOffsetTop and Element.prototype.totalOffsetLeft above to return Element.prototype.totalOffset.top and Element.prototype.totalOffset.left respectively.

>>>> Source/WebCore/inspector/front-end/utilities.js:916
>>>> +function stopPropagation(e) {
>>> 
>>> Not sure if this is too useful. Pavel can comment on this
>> 
>> I don't even see you calling this in your patch. What does this have to do with anonymous functions? Just locally call event.stopPropagation(). This is useless, please remove.
> 
> Please see this comment by Pavel Feldman: https://bugs.webkit.org/show_bug.cgi?id=71262#c31.  I don't call any of these changes in the patch.  He wanted me to include the utility functions as one patch and the code using it as another patch.  This is to make it easier to land all the changes.
> 
> I think it is useful to be able to replace this anonymous function:
> 
> element.addEventListener("click", function(e) {
>    e.stopPropagation();
> }, false);
> 
> With this:
> 
> element.addEventListener("click", stopPropagation, false);
> 
> I was told to not use anonymous functions in the web inspector code, that is why I created the helper function.  As I said, I can move this to a local scope and land it with the Spectrum changes, but there is at least one other place in code where this exact thing is done, so this could help with that as well.

I've seen it used in the spectrum component multiple times. I'd rather have it here in utilities.js than in Spectrum only.

[Pavel Feldman]

Comment on attachment 120902 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation

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

> Source/WebCore/inspector/front-end/Popover.js:35
> +WebInspector.Popover = function(popoverHelper, hideOverflow)

You should also make hideOverflow as optional:

* @param {boolean=} hideOverflow

above.

[Brian Grinstead]

Created attachment 121132 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 1)

[Pavel Feldman]

Comment on attachment 121132 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 1)

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

> Source/WebCore/inspector/front-end/Popover.js:117
> +        const scrollerWidth = this._hideOverflow ? 0 : 11;

Nit: contants should not be conditional: scrollerWidth is always 11, you should use ? in the call site.

> Source/WebCore/inspector/front-end/Popover.js:124
> +        const totalWidth = this._parentElement ? this._parentElement.clientWidth : window.innerWidth;

this._parentElement is always be there, no need to check.

> Source/WebCore/inspector/front-end/Popover.js:130
> +        // If a parent element has been specified, position within that element instead

Now that I looked at the runtime, I think this is wrong. We never want popovers to be limited with the ancestor elements. When there is not enough real estate, we'd like them to be rendered say "to the left" of the anchor, which is outside the styles sidebar. So you should not need this change. Sorry for bringing it up so late.

> Source/WebCore/inspector/front-end/Popover.js:135
> +            console.log(relativeOffset, anchorBox.x, anchorBox.y)

No logging should be used in the production.

> Source/WebCore/inspector/front-end/utilities.js:911
> +function stopPropagation(e) {

{ should be on the next line

[Brian Grinstead]

(In reply to comment #9)

> Now that I looked at the runtime, I think this is wrong. We never want popovers to be limited with the ancestor elements. When there is not enough real estate, we'd like them to be rendered say "to the left" of the anchor, which is outside the styles sidebar. So you should not need this change. Sorry for bringing it up so late.

I thought about this as well when first implementing it.  The issue that comes up though is that the popover 'sticks' to the location it was opened, even if the sidebar pane is scrolled or resized.  This didn't feel right to me.  The original popover component wasn't designed to be used in a scrolling panel as far as I can tell.  That, combined with the small constant size of the colorpicker I think makes sense to put it in the styles sidebar - essentially inserting it in the DOM next to the swatch it 'belongs' to.

Can you try commenting out the line:
'this._popover.parentElement = container;'
from https://bugs.webkit.org/show_bug.cgi?id=71262 and tell me if you still think it is preferable?  Make sure you scroll after opening a colorpicker.

[Pavel Feldman]

> I thought about this as well when first implementing it.  The issue that comes up though is that the popover 'sticks' to the location it was opened, even if the sidebar pane is scrolled or resized.  This didn't feel right to me.  The original popover component wasn't designed to be used in a scrolling panel as far as I can tell. 

We can change that. In fact, on-hover evaluations are available on source frame, timing popup on the network panel. Both scroll, it is just that they act more like tooltips. I'd expect them to listen to onscroll and override View's wasHidden to clean up. I'd definitely prefer this way of operation.

[Brian Grinstead]

Created attachment 125057 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 2_

Doesn't need the changes to Popover.js anymore so this is a smaller change

[Alexander Pavlov (apavlov)]

Comment on attachment 125057 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 2_

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

Looks good

> Source/WebCore/ChangeLog:1
> [phone]  bgrins  <[e-mail]>

WebKit endorses the use of real names

[Pavel Feldman]

Comment on attachment 125057 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 2_

Please fix the ChangeLog prior to landing.

[Brian Grinstead]

Created attachment 125833 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 3)

Same as Update 2, just with full name in ChangeLog

[Alexander Pavlov (apavlov)]

Comment on attachment 125833 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 3)

Looks good. One thing is that you should never set "r+" on your own patches. Rather, a request for review is "r?"

[Alexander Pavlov (apavlov)]

Committed r107079: <http://trac.webkit.org/changeset/107079>