Summary: | Web Inspector: Add utility changes for Spectrum colorpicker | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Grinstead <briangrinstead> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Brian Grinstead
2012-01-02 17:21:49 PST
Created attachment 120902 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation
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 (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 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. (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 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 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. Created attachment 121132 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 1)
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 (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. > 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.
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 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 on attachment 125057 [details]
Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 2_
Please fix the ChangeLog prior to landing.
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 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?"
Committed r107079: <http://trac.webkit.org/changeset/107079> |