Closed Bug 1741797 Opened 6 months ago Closed 6 months ago

#menu_eyedropper spawns multiple eyedroppers if an eyedropper has already started, instead of destroying the existing one, permanently breaking copy/paste interactions on the page

Categories

(DevTools :: Inspector, defect, P2)

Desktop
Unspecified
defect

Tracking

(firefox-esr91 unaffected, firefox94 unaffected, firefox95 fixed, firefox96 fixed)

RESOLVED FIXED
96 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox94 --- unaffected
firefox95 --- fixed
firefox96 --- fixed

People

(Reporter: aminomancer, Assigned: jdescottes)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file)

When you open the inspector with ctrl + shift + i, there is an eyedropper button #inspector-eyedropper-toggle on the top right side of the markup inspector pane. This button holds a checked state, so its command checks the button and starts the eyedropper; then when pressed again, its command unchecks the button and destroys the eyedropper. See inspector.js:

  onEyeDropperButtonClicked: function() {
    this.eyeDropperButton.classList.contains("checked")
      ? this.hideEyeDropper()
      : this.showEyeDropper();
  },

Something comparable does not exist for the menuitem #menu_eyedropper at Tools > Browser Tools > Eyedropper. It only has one callback and that callback doesn't check if the eyedropper is already running. So upon pressing it more than once, multiple eyedroppers spawn on top of each other.

On mouseover, the redundant eyedroppers get visually destroyed, but that doesn't stop them from appearing when the mouse is not moving. So, there's some element of visual jank there, but something else happens which persists until the tab is closed. This is a bit more complicated so bear with me.

It's possible to use the eyedropper to some extent without the mouse or touch events. If you open the menubar and then use arrow keys to navigate and the enter key to trigger the eyedropper menuitem's callback, that's probably the easiest way to reproduce it. So do this on a page where you have a textarea you can paste stuff into, and some text on the page (or already in the textarea) that you can copy. The Tab Notes extension or my fork thereof is a good example.

Once you've found a suitable page, make sure there's some text you can copy and paste into the textarea. And repeat the steps I described, opening multiple redundant instances of the eyedropper. When you move your mouse in the page, they will visually disappear, but apparently they still exist. Now exit out of the eyedropper by clicking or hitting Escape. There should be no eyedroppers remaining on the screen, at least visually.

Now, click and drag to select some text. Go for a big paragraph of regular prose. It doesn't matter really, just make sure you don't copy a color code like #FFFFFF or this is gonna get confusing. Hit ctrl/cmd+C to copy it. Then, click somewhere else inside the textarea to move your text caret, and hit ctrl/cmd+V to paste it. So you're basically copying some text and pasting it elsewhere.

But that's not what happens — instead, you paste a color code that you never copied. Between the time you hit ctrl+C and the time you hit ctrl+V, you clicked something with the mouse. Apparently you activated the hidden redundant eyedroppers, which no longer exist visually but are still chugging away in the background. Instead of pasting the string you copied with ctrl+C, you pasted the color code that eyedropper generated when you clicked to move your caret.

And this seems to persist indefinitely for as long as that page exists. I tried clicking dozens of times but it seems the hidden eyedropper never gets destroyed. So, the menuitem's inability to check if the eyedropper is already running and close the existing one if necessary can apparently result in a page being permanently crippled.

Of course this could be fixed at a lower level. The redundant eyedroppers should be completely eliminated on mousemove, not just visually. Apparently that isn't working, and should be fixed anyway.

But that's not the main issue, I think. Putting the duplicate eyedroppers issue aside, and even if this hidden eyedropper issue was fixed, you also can't use the menuitem to exit/close the eyedropper. The only way out is to hit escape (a different input mode than you used to enter the eyedropper, so intrinsically less convenient) or to click somewhere in the content area to actually pick a color (which will change your clipboard, so it's not desirable as a means of bailing out of the eyedropper, as a means of changing your mind).

Since that menuitem is the simplest and most practical way to open the eyedropper without opening the devtools, and it lacks the capacity to exit an existing eyedropper, making a hotkey or toolbar button that can launch an eyedropper in fewer steps is more problematic.

For example, I have an autoconfig script that does both, making a widget the user can put in their toolbar and a key element for ctrl+shift+Y. So those are both pretty easy to get to compared to the menuitem, and I imagine very useful for web developers, since a webextension can't inject its scripts into every kind of page the devtools can interact with.

But both the hotkey and the toolbar button work by sending click events to the menuitem in question, because the relevant devtools stuff is not exposed to the execution context the autoconfig script is running in. In order to get to it I'd have to import a lot of redundant code and even libraries. At that point I might as well be modifying the devtools source code, which isn't really a user-friendly customization. So just piggybacking on the menuitem is the easiest method I could find.

If Firefox actually ships a toolbar button or hotkey for the eyedropper this won't be as serious since they can just be implemented in DevToolsStartup.jsm and have access to the methods to tell whether an eyedropper is running and if so, destroy it. And maybe that's reasonable to do, since at least those would be useful features to have in Nightly and DE builds.

But either way, the menuitem should be fixed since even without the toolbar button or hotkey, the duplicate eyedroppers can be seen, and it's apparent that you can't close the eyedropper without switching to a different input device or copying something to your clipboard. And then with added features like the toolbar button, that just becomes even more apparent.

Actually, I think the problem I mentioned (where copy/paste becomes impossible because clicking anything causes a color code to be copied, even though the eyedropper has visually disappeared) has nothing to do with duplicate eyedroppers being opened. It seems to appear even when you only open the eyedropper once, and even when you open the eyedropper with the inspector button, inside the devtools. I guess it's a new thing or maybe I just never noticed it until now. So that seems like a more serious problem that will happen invariably when you use the eyedropper feature at all.

So to reproduce that you can simply open the eyedropper once, close it by hitting Escape or by clicking, and then try copying text, clicking into a textarea or input, and pasting it. Instead of pasting the copied text, you'll paste a color code, because when you clicked the textarea/input, the eyedropper was apparently still running, so it copied the textarea/input's color code.

Flags: needinfo?(odvarko)

Thank you for the report and detailed description!

I can easily reproduce it on my machine (Win10 + Firefox Nightly 96)

I created a simple test case here:
http://janodvarko.cz/tests/bugzilla/1741797/

Honza

Flags: needinfo?(odvarko)

I think it's because of this line: https://searchfox.org/mozilla-central/rev/0f3e259c24e52932387318ac503bfad3c82baa44/devtools/server/actors/highlighters/eye-dropper.js#169

pageListenerTarget.addEventListener("click", this, true, config);

The signature is incorrect the config argument gets ignored here.

Assignee: nobody → jdescottes
Severity: -- → S3
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Regressed by: 1732906
Has Regression Range: --- → yes

Set release status flags based on info from the regressing bug 1732906

ni to update the patch with a mochitest and send to review

Flags: needinfo?(jdescottes)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/75d83b5f0cba
[devtools] Fix colorpicker click listener signature r=nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch

Comment on attachment 9251548 [details]
Bug 1741797 - [devtools] Fix colorpicker click listener signature

Beta/Release Uplift Approval Request

  • User impact if declined: Users who start the eyedropper in DevTools will be unable to use their clipboard properly: it will be overridden with a color string everytime they click on a page. (eg "#ff0000")
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Small javascript fix, we just update a call to addEventListener which was using a slightly wrong signature. The regression is now covered by an automated test.
  • String changes made/needed:
Flags: needinfo?(jdescottes)
Attachment #9251548 - Flags: approval-mozilla-beta?

Comment on attachment 9251548 [details]
Bug 1741797 - [devtools] Fix colorpicker click listener signature

Has a test and looks small and safe enough for our last beta, approved for 95 beta 12, thanks.

Attachment #9251548 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

The signature fix is great but I don't think this should be closed yet. The other issue I indicated before is still present, that you can't exit the eyedropper by activating the menuitem, or by any means besides clicking in the browser (affects the clipboard so doesn't really "cancel" the eyedropper action), hitting escape (different input device), or opening the content toolbox to click the eyedropper checkbox button. I don't think the menuitem should be turned into a checkbox type menuitem, just that the command it sends to the eyedropper should result in exiting the eyedropper if already active. So that could be handled much further downstream of the menuitem itself

I filed a follow up bug 1742793

Thank you for the note.

You need to log in before you can comment on or make changes to this bug.