Web Inspector: Colorpicker not saving values

[Brian Grinstead]

When working on another case (https://bugs.webkit.org/show_bug.cgi?id=108852) I noticed that the colorpicker is no longer saving values.  If you click on the color swatch and open up the colorpicker, it will let you change the color in the popup, but the value never gets saved.  This is caused by the following line:

Source/WebCore/inspector/front-end/StylesSidebarPane.js

-                var scrollerElement = hasSpectrum ? self._parentPane._computedStylePane.element.parentElement : null;

This fixes it:

+                var scrollerElement = hasSpectrum ? self._parentPane._computedStylePane.bodyElement : null;

I'm working on a patch for this.

[Brian Grinstead]

Created attachment 187572 [details]
Patch to fix colorpicker save issue

Sorry if this is not in the correct format, trying to follow the guide for creating a patch and may not have done it right.

[Alexander Pavlov (apavlov)]

@Vlad:

I believe the issue is related to your recent changes to the sidebar structure. Could you see if anything else is broken in the StylesSidebarPane and MetricsSidebarPane (by plain looking at the code that may use the changed DOM elements)?

The solution suggested by Brian does not fix the broken behavior when the spectrum popup should follow the swatch when the sidebar pane stack is scrolled. Could you advise on the right solution?

[Alexander Pavlov (apavlov)]

Comment on attachment 187572 [details]
Patch to fix colorpicker save issue

Does the spectrum follow the swatch when the sidebar is scrolled?

[Brian Grinstead]

You are right, the popup does not follow the swatch while the sidebar is scrolled with the proposed patch.

(In reply to comment #3)
> (From update of attachment 187572 [details])
> Does the spectrum follow the swatch when the sidebar is scrolled?

[Brian Grinstead]

Not saying that we *should* do this, but this code does cause everything to work: 

    var scrollerElement = hasSpectrum ? self._parentPane._parentView._parentView.element.parentNode : null;

The scroll event seems to now be happening on the div[class=split-view-contents split-view-contents-vertical split-view-sidebar-right].  Surely there is a better way to get that element from the stylessidebarpane than the way above, though.


(In reply to comment #3)
> (From update of attachment 187572 [details])
> Does the spectrum follow the swatch when the sidebar is scrolled?

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> Not saying that we *should* do this, but this code does cause everything to work: 
> 
>     var scrollerElement = hasSpectrum ? self._parentPane._parentView._parentView.element.parentNode : null;
> 
> The scroll event seems to now be happening on the div[class=split-view-contents split-view-contents-vertical split-view-sidebar-right].  Surely there is a better way to get that element from the stylessidebarpane than the way above, though.

Correct. That's why I was asking Vlad for advice. The new structure must have better accessors than ugly constructs like this one :)

[Vladislav Kaznacheev]

Created attachment 188050 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 188050 [details]
Patch

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

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1786
> +                    var scrollerElement = self._parentPane.container().scrollTarget();

Can the scrollerElement change while the color picker is active? (E.g. if the user resizes the web inspector window, which can alter the sidebar pane representation)

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1788
> +                    console.log("detached scroll listener from ", scrollerElement);

Please remove debug logging

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1822
> +                            console.log("attached scroll listener to ", scrollerElement);

Ditto

[Vladislav Kaznacheev]

Created attachment 188052 [details]
Patch

[Vladislav Kaznacheev]

Created attachment 188059 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 188059 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 188059 [details]
Patch

Rejecting attachment 188059 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 188059, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
rce/WebKit/chromium/webkit/media/crypto/ppapi/cdm --revision 173055 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
56>At revision 173055.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/16514150

[Vladislav Kaznacheev]

Created attachment 188071 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188071 [details]
Patch

Clearing flags on attachment: 188071

Committed r142745: <http://trac.webkit.org/changeset/142745>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.