[Flexbox Inspector][Color Picker] User selected color is not persistent with the color picker

VERIFIED FIXED in Firefox 64

Status

defect
P3
normal
VERIFIED FIXED
7 months ago
3 months ago

People

(Reporter: paul.boiciuc, Assigned: gl)

Tracking

(Blocks 1 bug)

64 Branch
Firefox 64
Unspecified
All
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed, firefox65 verified)

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

7 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0
Build ID: 20181008100121

STR:

1. Launch Firefox and navigate to a random page (e.g. nytimes.com)
2. Enable the element inspector.
3. Choose a Flex Container or Item and enable the highlighter.
4. Navigate to the Flexbox Inspector section and select the color picker.
5. Try changing the overlay color.
6. Observe the overlay color on the webpage while the user changes colors in the color picker.

Actual result:

The color selected by the user is not reflected on the page. 

*Workaround: The change can be observed if the flex overlay is disabled and then re-enabled.  

I have attached a short video for better understanding.

Expected result:

The user can choose a new color for the highlighter overlay and the color is properly reflected in the webpage.
(Reporter)

Updated

7 months ago
Blocks: 1470379
(Assignee)

Updated

6 months ago
Priority: -- → P3
(Assignee)

Updated

6 months ago
Assignee: nobody → gl
Summary: [Flexbox Inspector][Color Picker] The color of the overlay does not change on user input → [Flexbox Inspector][Color Picker] The color of the overlay does not change on user input and color picker
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1494804
(Assignee)

Updated

6 months ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 months ago
Posted patch 1497312.patch [1.0] (obsolete) — Splinter Review
Attachment #9016021 - Flags: review?(pbrosset)
(Assignee)

Updated

6 months ago
Blocks: 1478396, dt-flexbox
No longer blocks: 1470379
Comment on attachment 9016021 [details] [diff] [review]
1497312.patch [1.0]

Review of attachment 9016021 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/flexbox/test/browser_flexbox_highlighter_color_picker_on_ESC.js
@@ +2,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test that the flexbox highlighter color change in the color picker is reverted when

Thank you for adding this test (and the other one). Only comment I have is that they claim to be testing the highlighter color, but really they test the color of the swatch in the panel.
Can we also somehow test the color sent to the highlighter? I know it uses canvas to draw, so that might be hard. But maybe there's a way we can check the call to the highlighter (maybe stubbing the highlighter's show/hide methods so it doesn't actually appear, but we get to see what options it is called with).
Attachment #9016021 - Flags: review?(pbrosset)
(Assignee)

Comment 4

6 months ago
I don't know if we could do any better here.
Attachment #9016021 - Attachment is obsolete: true
Attachment #9016753 - Flags: review?(pbrosset)
Attachment #9016753 - Flags: review?(pbrosset) → review+

Comment 5

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb2909f660d
The flexbox highlighter color should change on input from the color swatch. r=pbro
Backed out changeset ffb2909f660d (Bug 1497312) for browser_flexbox_highlighter_color_picker_on_RETURN.js failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=205603026&revision=ffb2909f660d4ea2f9961e5d1d4964a2c98e51a3

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/20cc444ecb1222fc280c08e658fe1b22de00d1a7

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=205603026&repo=mozilla-inbound&lineNumber=1635

[task 2018-10-15T19:46:14.436Z] 19:46:14     INFO - TEST-PASS | devtools/client/inspector/flexbox/test/browser_flexbox_highlighter_color_picker_on_RETURN.js | The flexbox color state is correct. - 
[task 2018-10-15T19:46:14.437Z] 19:46:14     INFO - Toggling ON the flexbox highlighter.
[task 2018-10-15T19:46:14.438Z] 19:46:14     INFO - Waiting for state predicate "state => state.flexbox.highlighted"
[task 2018-10-15T19:46:14.438Z] 19:46:14     INFO - Found state predicate "state => state.flexbox.highlighted"
[task 2018-10-15T19:46:14.439Z] 19:46:14     INFO - Opening the color picker by clicking on the color swatch.
[task 2018-10-15T19:46:14.440Z] 19:46:14     INFO - Buffered messages logged at 19:46:13
[task 2018-10-15T19:46:14.440Z] 19:46:14     INFO - Getting the spectrum colorpicker object
[task 2018-10-15T19:46:14.441Z] 19:46:14     INFO - Setting the new color
[task 2018-10-15T19:46:14.442Z] 19:46:14     INFO - Applying the change
[task 2018-10-15T19:46:14.442Z] 19:46:14     INFO - Checking the flexbox highlighter display settings.
[task 2018-10-15T19:46:14.443Z] 19:46:14     INFO - TEST-PASS | devtools/client/inspector/flexbox/test/browser_flexbox_highlighter_color_picker_on_RETURN.js | Flexbox highlighter color is correct. - 
[task 2018-10-15T19:46:14.443Z] 19:46:14     INFO - Buffered messages finished
[task 2018-10-15T19:46:14.446Z] 19:46:14     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/flexbox/test/browser_flexbox_highlighter_color_picker_on_RETURN.js | The color swatch's background was updated. - Got rgb(148, 0, 255), expected rgba(0, 255, 0, 0.5)
[task 2018-10-15T19:46:14.448Z] 19:46:14     INFO - Stack trace:
[task 2018-10-15T19:46:14.449Z] 19:46:14     INFO - chrome://mochikit/content/browser-test.js:test_is:1295
[task 2018-10-15T19:46:14.450Z] 19:46:14     INFO - chrome://mochitests/content/browser/devtools/client/inspector/flexbox/test/browser_flexbox_highlighter_color_picker_on_RETURN.js:null:51
[task 2018-10-15T19:46:14.452Z] 19:46:14     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1093
[task 2018-10-15T19:46:14.453Z] 19:46:14     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084
[task 2018-10-15T19:46:14.454Z] 19:46:14     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:986
[task 2018-10-15T19:46:14.454Z] 19:46:14     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
[task 2018-10-15T19:46:14.455Z] 19:46:14     INFO - Pressing RETURN to commit the color change.
[task 2018-10-15T19:46:14.456Z] 19:46:14     INFO - Waiting for state predicate "state =>
[task 2018-10-15T19:46:14.457Z] 19:46:14     INFO -     state.flexbox.color === "#00FF0080""
[task 2018-10-15T19:46:14.458Z] 19:46:14     INFO - GECKO(1073) | console.log: "[DISPATCH] action type:" "UPDATE_FLEXBOX_COLOR"
[task 2018-10-15T19:46:14.459Z] 19:46:14     INFO - GECKO(1073) | console.log: "[DISPATCH] action type:" "UPDATE_FLEXBOX_COLOR"
[task 2018-10-15T19:46:14.467Z] 19:46:14     INFO - GECKO(1073) | --DOMWINDOW == 40 (0x7f6ea4da8400) [pid = 1073] [serial = 30] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:14.468Z] 19:46:14     INFO - GECKO(1073) | --DOMWINDOW == 39 (0x7f6ec551c800) [pid = 1073] [serial = 28] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:14.469Z] 19:46:14     INFO - GECKO(1073) | --DOMWINDOW == 38 (0x7f6eafc1e400) [pid = 1073] [serial = 10] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:14.470Z] 19:46:14     INFO - GECKO(1073) | --DOMWINDOW == 37 (0x7f6eafc1d400) [pid = 1073] [serial = 8] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:14.473Z] 19:46:14     INFO - GECKO(1073) | --DOMWINDOW == 36 (0x7f6ebf15d000) [pid = 1073] [serial = 2] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:14.474Z] 19:46:14     INFO - GECKO(1073) | --DOMWINDOW == 35 (0x7f6ed811dc00) [pid = 1073] [serial = 14] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:14.475Z] 19:46:14     INFO - GECKO(1073) | --DOMWINDOW == 34 (0x7f6ec5526400) [pid = 1073] [serial = 18] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:14.476Z] 19:46:14     INFO - GECKO(1073) | --DOMWINDOW == 33 (0x7f6ec5522800) [pid = 1073] [serial = 17] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:15.105Z] 19:46:15     INFO - GECKO(1073) | --DOCSHELL 0x7f6ea817d000 == 11 [pid = 1073] [id = {5abed056-4f89-4ce7-a29a-2113346b7cf2}]
[task 2018-10-15T19:46:17.063Z] 19:46:17     INFO - GECKO(1073) | --DOMWINDOW == 2 (0x7fed767b4000) [pid = 1242] [serial = 2] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:19.626Z] 19:46:19     INFO - GECKO(1073) | --DOMWINDOW == 1 (0x7fdc29d56800) [pid = 1213] [serial = 1] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:19.662Z] 19:46:19     INFO - GECKO(1073) | --DOMWINDOW == 1 (0x7fed8d654600) [pid = 1242] [serial = 1] [outer = (nil)] [url = http://example.com/browser/devtools/client/inspector/flexbox/test/doc_flexbox_simple.html]
[task 2018-10-15T19:46:20.405Z] 19:46:20     INFO - GECKO(1073) | --DOMWINDOW == 2 (0x7fc4aee4e800) [pid = 1289] [serial = 2] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:22.197Z] 19:46:22     INFO - GECKO(1073) | --DOMWINDOW == 32 (0x7f6eada34000) [pid = 1073] [serial = 22] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:22.198Z] 19:46:22     INFO - GECKO(1073) | --DOMWINDOW == 31 (0x7f6eb1edcc00) [pid = 1073] [serial = 41] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:22.199Z] 19:46:22     INFO - GECKO(1073) | --DOMWINDOW == 30 (0x7f6eafc5ee00) [pid = 1073] [serial = 25] [outer = (nil)] [url = chrome://devtools/content/inspector/index.xhtml]
[task 2018-10-15T19:46:22.201Z] 19:46:22     INFO - GECKO(1073) | --DOMWINDOW == 29 (0x7f6ea4d59800) [pid = 1073] [serial = 31] [outer = (nil)] [url = chrome://devtools/content/inspector/markup/markup.xhtml]
[task 2018-10-15T19:46:23.614Z] 19:46:23     INFO - GECKO(1073) | --DOMWINDOW == 0 (0x7fdc13802400) [pid = 1213] [serial = 2] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:23.733Z] 19:46:23     INFO - GECKO(1073) | --DOMWINDOW == 0 (0x7fed75dcc400) [pid = 1242] [serial = 3] [outer = (nil)] [url = http://example.com/browser/devtools/client/inspector/flexbox/test/doc_flexbox_simple.html]
[task 2018-10-15T19:46:33.063Z] 19:46:33     INFO - GECKO(1073) | --DOMWINDOW == 28 (0x7f6ea45a5000) [pid = 1073] [serial = 33] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:33.064Z] 19:46:33     INFO - GECKO(1073) | --DOMWINDOW == 27 (0x7f6eac294000) [pid = 1073] [serial = 26] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:33.065Z] 19:46:33     INFO - GECKO(1073) | --DOMWINDOW == 26 (0x7f6eada2ec00) [pid = 1073] [serial = 24] [outer = (nil)] [url = about:devtools-toolbox]
[task 2018-10-15T19:46:33.067Z] 19:46:33     INFO - GECKO(1073) | --DOMWINDOW == 25 (0x7f6eac29f800) [pid = 1073] [serial = 42] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:46:33.068Z] 19:46:33     INFO - GECKO(1073) | --DOMWINDOW == 24 (0x7f6ea4db1000) [pid = 1073] [serial = 32] [outer = (nil)] [url = chrome://devtools/content/inspector/markup/markup.xhtml]
[task 2018-10-15T19:46:33.068Z] 19:46:33     INFO - GECKO(1073) | --DOMWINDOW == 23 (0x7f6eada28400) [pid = 1073] [serial = 23] [outer = (nil)] [url = about:blank]
[task 2018-10-15T19:47:34.591Z] 19:47:34     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-10-15T19:47:34.593Z] 19:47:34     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/flexbox/test/browser_flexbox_highlighter_color_picker_on_RETURN.js | Test timed out - 
[task 2018-10-15T19:47:34.933Z] 19:47:34     INFO - GECKO(1073) | ++DOMWINDOW == 24 (0x7f6eaa1b1c00) [pid = 1073] [serial = 45] [outer = 0x7f6eada39800]
[task 2018-10-15T19:47:35.765Z] 19:47:35     INFO - Removing tab.
[task 2018-10-15T19:47:35.769Z] 19:47:35     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2018-10-15T19:47:35.814Z] 19:47:35     INFO - Got event: 'TabClose' on [object XULElement].
[task 2018-10-15T19:47:35.850Z] 19:47:35     INFO - Tab removed and finished closing
Flags: needinfo?(gl)

Comment 7

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/458bf1864db0
The flexbox highlighter color should change on input from the color swatch. r=pbro

Comment 8

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/458bf1864db0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
(Assignee)

Updated

6 months ago
Flags: needinfo?(gl)
(Reporter)

Comment 9

6 months ago
Verified using the latest Nightly build: 20181017101626 on Windows 10, Ubuntu 18.04 and Mac OSX, and part of the issue is fixed. The overlay color does change on user input and the color change is reflected on the color picker "icon". 

However, there are some small issues that are still persistent, namely the actual color of the color picker "icon" and the persistence of the selected color.

1. The color of the picker changes just for a quick second when the user selects a new color using the dropper, as it defaults back to the original one.

2. If the user navigates to the sizing info display of one flex item that is part of the selected container and then back, the overlay does maintain its color, but the color on the color picker "icon" defaults to the original one. 

The same issue is observed if the selected flex item is actually an inner container. The color of the color picker displayed next to the inner flex container is the default one, instead of the color that the user previously selected and is properly reflected by the overlay. Furthermore, if the toggle is enabled in order to view just the overlay for the inner container, the color of the overlay is changed to the default one. This issue is somewhat the same as if the user chooses to select a new flex element. In this case, the color picker defaults back to the original color. As far as I remember, Victoria mentioned that the color selection should be persistent.
(Reporter)

Updated

6 months ago
Flags: needinfo?(gl)
(Reporter)

Comment 10

6 months ago
I know it's a low priority issue, and probably we can treat it/edit as such (P4, P5), but I am going to reopen this ticket, and maybe, whenever there is availability, this can be addressed:

1. The color picker icon is reset to the default color, although the overlay color reflects the color previously modified by the user, in a number of scenarios like: page refresh; user enabling the sizing info display of a flex child and then going back to the properties display; another element from the 1st panel is selected and then the user navigates back to the flex-element; inspector is resized
2. The color selected using the dropper is working now for the overlay but is still not reflected on the same color picker icon
3. If the color is selected using the big square, and the selector is moved anywhere on the left or bottom edge, then closed and reopened, the vertical color selector defaults to the top position, making the color of the big square selector, red.

I'm going to add a short video of this small issues, for a better understanding.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 11

6 months ago
(Reporter)

Comment 12

6 months ago
I've edited the title of the issue in order to better reflect the current behavior.
Summary: [Flexbox Inspector][Color Picker] The color of the overlay does not change on user input and color picker → [Flexbox Inspector][Color Picker] The color picker is not persistent
(Reporter)

Updated

6 months ago
Summary: [Flexbox Inspector][Color Picker] The color picker is not persistent → [Flexbox Inspector][Color Picker] User selected color is not persistent with the color picker
(Reporter)

Comment 13

6 months ago
The color picker does not work in iFrame scenarios. The color change does not apply to the overlay and the color of the color picker defaults back to the original as soon as the user is done selecting a different color. I have attached a short video of the issue.
(Assignee)

Comment 14

6 months ago
Posted patch 1497312.patchSplinter Review
I was incorrectly renaming hostname to hostName at some point for the constant name, and that also changed the property we were fetching from hostname to hostName. The difference is that hostName returns only "https://" whereas hostname returns "https://bugzilla.mozilla.org/attachment.cgi?bugid=1497312&action=enter".
Flags: needinfo?(gl)
Attachment #9023408 - Flags: review?(mtigley)
Attachment #9023408 - Flags: review?(mtigley) → review+

Comment 15

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/864f1996cb75
Use hostname instead of hostName to get the correct the hostname. r=mtigley

Comment 16

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/864f1996cb75
Status: REOPENED → RESOLVED
Last Resolved: 6 months ago6 months ago
Resolution: --- → FIXED
(Reporter)

Comment 17

5 months ago
Verified using latest nightly build: 20181112100105 on all OS-es and the issue does not seem to be fixed. 

The color of the picker defaults to the original one once the user navigates to the sizing display of a flex item and then back to the initial flex container layout and the color of the big square pallet changes to red, as the vertical picker defaults to the top position if the user selects a color from the right edge or bottom edge of the big square. Furthermore, if the color is changed on nested containers, this will impact the mini-map, and using the eye-dropper to change the color does not work. 

I attached a short video of all this. Based on the above, I'm going to re-open this issue.
(Reporter)

Updated

5 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

5 months ago
Posted patch 1497312.patchSplinter Review
Seems like I missed one final step in my last patch which was to also store the new color in this._overlayColor which is actually the cached overlay color value that we show.
Attachment #9025439 - Flags: review?(mtigley)
Comment on attachment 9025439 [details] [diff] [review]
1497312.patch

Review of attachment 9025439 [details] [diff] [review]:
-----------------------------------------------------------------

Verified that the mini-map and color swatch no longer default back to the original color when using the eye dropper tool after changing colors on a nested container. 

As for the issue with the color picker defaulting back to red when the very bottom corners are selected, I think it has to do with the color picker itself than the flexbox inspector. I see this happening with the Grid Inspector too. So I don't think this issue should be addressed in this patch, but in another one instead.
Attachment #9025439 - Flags: review?(mtigley) → review+

Comment 20

5 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9864c98ab553
Store the overlay color when the overlay color is changed from the color picker. r=mtigley

Comment 21

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9864c98ab553
Status: REOPENED → RESOLVED
Last Resolved: 6 months ago5 months ago
Resolution: --- → FIXED
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1507719
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1507752
(Reporter)

Comment 24

5 months ago
(In reply to Micah Tigley [:mtigley] from comment #19)
 
> As for the issue with the color picker defaulting back to red when the very
> bottom corners are selected, I think it has to do with the color picker
> itself than the flexbox inspector. I see this happening with the Grid
> Inspector too. So I don't think this issue should be addressed in this
> patch, but in another one instead.

I added a new report based on the comment above - https://bugzilla.mozilla.org/show_bug.cgi?id=1510038

On the same topic, I've also added a new report that although it is not generated by this feature, it does impact it as it reproduces on every color picker - https://bugzilla.mozilla.org/show_bug.cgi?id=1510036

The persistence issue is still not fully fixed, therefore I added a new report here: https://bugzilla.mozilla.org/show_bug.cgi?id=1510023.
See Also: → 1510023

I reproduced this issue using 64.0a1(2018-10-08), on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 65.0b10 on Windows 10 x64, macOS 10.13 and Ubuntu 16.04 LTS.
Cheers!

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.