1432256 - Color Picker does not detect selected input outline

Status: NEW

(DevTools :: Inspector, defect, P3)

Description

[Erica Wright [:ewright]]

Attachment: Screen Shot 2018-01-22 at 1.56.08 PM.png

mac OS 10.13.2

STR:

- select/focus on an input to trigger the blue outline. 
- click on the color eye-dropper tool. (be sure to not lose the focus) 
- Mouse up to the border of the selected element.
- observe

AR:
- inside of the color picker, the blue outline does not show up. It appears as if the element is not focussed

ER: 
- the insides of the color picker should accurately represent what's seen on the screen.

Comment 1

[Patrick Brosset <:pbro>]

Thanks for filing Erica.
I think the reason is that right when the click happens, focus is lost temporarily (because it goes to the eyedropper icon), and that's exactly when the tool takes a full page screenshot. Then focus goes back to the input, but it's too late. The image that the tool uses to sample colors has already been taken.
So, I think a potential fix would be to try grabing the screenshot at another time. Either before focus is lost (maybe doing it on mousedown would work), or after a short delay, to make sure the page is back its previous state.

Comment 2

[Erica Wright [:ewright]]

Attachment: Bug 1432256 - Fix color picker not detecting focused styling; .

Review commit: https://reviewboard.mozilla.org/r/219892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219892/

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8950622 [details]
Bug 1432256 - Fix color picker not detecting focused styling; .

https://reviewboard.mozilla.org/r/219892/#review225730

::: devtools/server/actors/highlighters/eye-dropper.js:200
(Diff revision 1)
>    },
>  
>    prepareImageCapture() {
>      // Get the image data from the content window.
> -    let imageData = getWindowAsImageData(this.win);
> +    getWindowAsImageData(this.win)
> +    .then(imageData => this.win.createImageBitmap(imageData))

Add a tab indent here.

::: devtools/server/actors/highlighters/eye-dropper.js:205
(Diff revision 1)
> +    .then(imageData => this.win.createImageBitmap(imageData))
>  
>      // We need to transform imageData to something drawWindow will consume. An ImageBitmap
>      // works well. We could have used an Image, but doing so results in errors if the page
>      // defines CSP headers.
> -    this.win.createImageBitmap(imageData).then(image => {
> +    .then(image => {

Same as above.

::: devtools/server/actors/highlighters/eye-dropper.js:481
(Diff revision 1)
>   * Draw the visible portion of the window on a canvas and get the resulting ImageData.
>   * @param {Window} win
> - * @return {ImageData} The image data for the window.
> + * @return {Promise} Resolves with the ImageData (after a run of the event loop to
> + * allow the window to draw the focus effects on a focused element).
>   */
> +

Don't add a new line here.

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

It would be also nice to convert prepareImageCapture to use async/await.

Comment 5

[Erica Wright [:ewright]]

Comment on attachment 8950622 [details]
Bug 1432256 - Fix color picker not detecting focused styling; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219892/diff/1-2/

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8950622 [details]
Bug 1432256 - Fix color picker not detecting focused styling; .

https://reviewboard.mozilla.org/r/219892/#review225972

I took a quick look at this, and the code changes look great. However the bug still appears for me on Windows 10. If I change the timeout to something random like 100ms, then it works. So just spinning the event loop once doesn't seem to be doing the trick on windows at least. Not sure what to do here. And also we need to avoid showing the eyedropper UI itself before taking the screenshot, otherwise it will appear in the image.
Perhaps there's an event we could listen to that would tell us when the page actually got focused, and then take the image, and then wait for that to be done before showing the eyedropper.