User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0 Build ID: 20160726030213 Steps to reproduce: I opened the color picker and tried to move it using my arrow keys. Actual results: The color picker didn't move. Expected results: The color picker should move in the direction of the arrow key.

Most likely related to new color picker: https://bugzilla.mozilla.org/show_bug.cgi?id=1262439

Regression ragne: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=e0bc88708ffed39aaab1fbc0ac461d93561195de&tochange=a7a882d122e36defe6c2a102a28ae7dfc16311c5 surely it's from bug 1262439.

Joe, what should we do about this regression (Patrick's on PTO so I'm needinfo'ing you)?

Alex, in Patrick's absence, please could you take a look at this

That seems to fix it on Linux. Also I tweaked the test in order to fail without the patch and pass with it.

Fixed a typo.

Comment on attachment 8778932 [details] [diff] [review] patch v2 Review of attachment 8778932 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this! This fixes the issue, but if the content page has a focusable element, it will move the focus to this element. This will visually impact the page and might trigger listeners. We should investigate other ways to handle keyboard navigation for highlighters. Is there any way we could create a focusable element in the eyedropper container and focus this element? I couldn't find a way to retrieve the anonymous content nodes, so I'm not sure this is possible. Another approach would be to fire an event from the inspector actor when the eye-dropper appears, and the inspector panel could then handle the keyboard navigation. But this means the actor should then expose methods to move the eyedropper, and I don't think we want to have client-server communication just for this. If we can't find any way to make this work, let's land your fix and log a follow up. I guess it's still better to have keyboard navigation with this side-effect than no keyboard navigation at all. Another issue with the current approach is that the focus is not restored to its previous target when the eye-dropper is dismissed. ::: devtools/client/inspector/test/head.js @@ -464,5 @@ > options = Object.assign({selector: ":root"}, options); > yield testActor.synthesizeMouse(options); > }, > > - synthesizeKey: function* (options) { synthesizeKey is still used in browser_inspector_highlighter-eyedropper-clipboard.js

Hum. Looking at the current behavior on release, it seems to be already kind of broken regarding the focus. Here is a test document: data:text/html,<style>:focus {background:blue}</style><input type="text" /> First, focus the input. Then if you only click (don't move the picker) the focus is preserved, but whenever you start moving the picker, the focus is lost. But what is completely broken is that if you over the input, you will still see the blue background within the eyedropper, whereas the input is white!! I'm not sure it is worth fixing more than just the regression. It is at least something coherent with this patch. Unfortunately, there is no way to access the DOM of the anonymous content. The API is limited to this: http://searchfox.org/mozilla-central/source/dom/webidl/AnonymousContent.webidl The only thing I can think about is inserting a autofocusable element, but that sounds very hacky. Otherwise, listening on the top level chrome window would address that, but that's very complex on e10s given that the highlighter actor lives in the child. Manually listening for events from the inspector sounds also hacky, unless we rip that off from the actor and manually listen from the top level window when hitting the menu or the inspector document when opening from the eyedroper icon.

(In reply to Alexandre Poirot [:ochameau] from comment #11) > Hum. Looking at the current behavior on release, it seems to be already kind > of broken regarding the focus. > > Here is a test document: > data:text/html,<style>:focus {background:blue}</style><input type="text" /> > First, focus the input. > Then if you only click (don't move the picker) the focus is preserved, but > whenever you start moving the picker, the focus is lost. Interesting, this behavior is Linux only though. On Windows and OSX, the input is blurred when clicking on the eyedropper icon and the zoomed preview is in-sync with the page. > But what is > completely broken is that if you over the input, you will still see the blue > background within the eyedropper, whereas the input is white!! > Makes sense since we screenshot the browser when starting the eyedropper tool. Any change to the page occurring after that will not be visible in the zoomed-in previews. > I'm not sure it is worth fixing more than just the regression. It is at > least something coherent with this patch. > I still think the fact that the new issue impacts all platforms vs Linux-only previously makes it a bigger problem. > Unfortunately, there is no way to access the DOM of the anonymous content. > The API is limited to this: > http://searchfox.org/mozilla-central/source/dom/webidl/AnonymousContent. > webidl > The only thing I can think about is inserting a autofocusable element, but > that sounds very hacky. Otherwise, listening on the top level chrome window > would address that, but that's very complex on e10s given that the > highlighter actor lives in the child. Manually listening for events from the > inspector sounds also hacky, unless we rip that off from the actor and > manually listen from the top level window when hitting the menu or the > inspector document when opening from the eyedroper icon. As I said, if we have no other option for now, let's go ahead with this approach (and maybe file follow ups, to restore the focus for instance). Something that should be addressed before landing though: the eyedroppper supports both <arrow key> and <SHIFT+arrow key>. However, in eye-dropper.js::handleKeyDown, there is no preventDefault when we are getting a <SHIFT+arrow key> event. Try the following STRs: - open about:home or any page with an input text - type some text in the input - open eyedropper - move it using shift+left/right => text in the input gets selected/deselected. IMO, handleKeyDown should only process arrow keys if there is no modifier or only the SHIFT modifier, and always preventDefault(). Can you submit a new patch for review addressing this issue? Thanks!

Tweak the preventDefault call. Shift+Arrow keys should be fixed.

Comment on attachment 8779395 [details] [diff] [review] patch v3 Review of attachment 8779395 [details] [diff] [review]: ----------------------------------------------------------------- A few things to fix/check before landing but looks good overall, thanks! I don't think this needs another round of review, so clearing my flag here. ::: devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-clipboard.js @@ +28,1 @@ > generateKey.next(); This fails now because synthesizeKey() used to return an iterator. I think you can simply remove the variable and the call to next() now. I just wonder why this was implemented in this way? Particularly considering it is a recent test. Please submit to try on a wide array of platforms before landing. ::: devtools/server/actors/highlighters/eye-dropper.js @@ +363,5 @@ > + // Prevent all keyboard interaction with the page, except if a modifier is used to let > + // keyboard shortcuts through. > + let hasModifier = e.metaKey || e.ctrlKey || e.altKey || e.shiftKey; > + if (!hasModifier) { > + e.preventDefault(); Just a style suggestion, so feel free to ignore as your current implementation works fine. I feel the code would be easier to understand if we would do 1 - // Bail out early if any unsupported modifier is used. if (e.metaKey || e.ctrlKey || e.altKey) { return; } 2 - Then remove the else {return} line 397 3 - Finally move the preventDefault() from line 404 to the last if statement (meaning "if we have any offset to apply, it means we are processing this event and we should preventDefault()"). @@ +395,3 @@ > offsetY = 1; > } > + else { lint: else and else if should be on the same line as the closing curly brace (4 errors)

With the style suggestion. Note that it forces to call preventDefault also for Return and Escape if blocks.

https://hg.mozilla.org/integration/fx-team/rev/481b43b3340f0f960063264eee946314d4357a79 Bug 1289433 - Fix Eye dropper focus to support key shortcuts on it when opening from the inspector. r=jdescottes

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/481b43b3340f Fix Eye dropper focus to support key shortcuts on it when opening from the inspector. r=jdescottes

I have reproduced this bug with Nightly 50.0a1 (2016-07-26) on Windows 8.1 , 64 bit! This Bug's fix is verified on Latest Nightly 51.0a1. Build ID : 20160816030459 User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 [bugday-20160817]

Reproduced this bug in firefox nightly 50.0a1 (2016-07-26) with ubuntu 16.04 (64 bit) Verified this bug as fixed with latest firefox nightly 51.0a1 (Build ID: 20160819080504) Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0 As it is also verified on windows (Comment 21), Marking it as verified!

How do you feel about an uplift here, Alexandre?

Sure. This patch is quite simple and focused.

Comment on attachment 8779742 [details] [diff] [review] patch v4 Approval Request Comment [Feature/regressing bug #]: bug 1262439 [User impact if declined]: [Describe test coverage new/current, TreeHerder]: Tested, verified, haven't heard about related regression. [Risks and why]: [String/UUID change made/needed]: no

Comment on attachment 8779742 [details] [diff] [review] patch v4 Fix was verified in Nightly, patch seems to add automated test (yay!), let's uplift to Aurora50.