Bug 75454 - Web Inspector: Add utility changes for Spectrum colorpicker
Summary: Web Inspector: Add utility changes for Spectrum colorpicker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-02 17:21 PST by Brian Grinstead
Modified: 2012-02-08 06:19 PST (History)
10 users (show)

See Also:


Attachments
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (6.96 KB, patch)
2012-01-02 17:54 PST, Brian Grinstead
no flags Details | Formatted Diff | Diff
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 1) (8.01 KB, patch)
2012-01-04 11:59 PST, Brian Grinstead
pfeldman: review-
Details | Formatted Diff | Diff
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 2_ (3.46 KB, patch)
2012-02-01 18:10 PST, Brian Grinstead
pfeldman: review+
Details | Formatted Diff | Diff
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 3) (3.47 KB, patch)
2012-02-07 06:21 PST, Brian Grinstead
pfeldman: review+
pfeldman: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Grinstead 2012-01-02 17:21:49 PST
Patch to follow
Comment 1 Brian Grinstead 2012-01-02 17:54:26 PST
Created attachment 120902 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation
Comment 2 Alexander Pavlov (apavlov) 2012-01-02 23:37:38 PST
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
Comment 3 Brian Grinstead 2012-01-03 07:23:28 PST
(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.
Comment 4 Timothy Hatcher 2012-01-03 09:46:58 PST
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.
Comment 5 Brian Grinstead 2012-01-03 11:24:20 PST
(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.
Comment 6 Pavel Feldman 2012-01-04 03:05:31 PST
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.
Comment 7 Pavel Feldman 2012-01-04 03:42:15 PST
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.
Comment 8 Brian Grinstead 2012-01-04 11:59:23 PST
Created attachment 121132 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 1)
Comment 9 Pavel Feldman 2012-01-05 01:44:28 PST
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
Comment 10 Brian Grinstead 2012-01-05 05:57:29 PST
(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.
Comment 11 Pavel Feldman 2012-01-05 06:04:43 PST
> 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.
Comment 12 Brian Grinstead 2012-02-01 18:10:22 PST
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
Comment 13 Alexander Pavlov (apavlov) 2012-02-07 05:19:36 PST
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
> +2012-02-01  bgrins  <briangrinstead@gmail.com>

WebKit endorses the use of real names
Comment 14 Pavel Feldman 2012-02-07 06:02:19 PST
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.
Comment 15 Brian Grinstead 2012-02-07 06:21:25 PST
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
Comment 16 Alexander Pavlov (apavlov) 2012-02-08 02:21:59 PST
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?"
Comment 17 Alexander Pavlov (apavlov) 2012-02-08 05:22:51 PST
Committed r107079: <http://trac.webkit.org/changeset/107079>