Closed Bug 1538648 Opened 5 years ago Closed 5 years ago

Select all option selects even the [Copy All Changes] button

Categories

(DevTools :: Inspector: Changes, defect, P1)

67 Branch
defect

Tracking

(firefox67 verified, firefox68 verified)

VERIFIED FIXED
Firefox 68
Tracking Status
firefox67 --- verified
firefox68 --- verified

People

(Reporter: cfogel, Assigned: rcaliman)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(3 files)

Affected versions

  • 67.0b4, 68.0a1 (2019-03-24)

Affected platforms

  • Windows 10, macOS 10.11, ubuntu 18.04

Steps to reproduce

  1. Launch Firefox and open the Changes tab from the devTools inspector;
  2. Make any change in the rules inspector;
  3. Right click on the change (inside the changes tab);
  4. Click on the Select All option;

Expected result

  • inside the Changes tab the class changes are selected;

Actual result

  • The whole content for the Changes tab is selected;
  • the [Copy All] button is also selected;

Regression range

  • not a regression, introduced with the Track Changes feature;

Additional notes

  • attached screenshot with the issue
  • as per n.3 from this document;
Has Regression Range: --- → no
Has STR: --- → yes
Priority: -- → P3

The good thing is that the copy all button does not get copied even if it looks selected. So we should still fix this because it looks confusing, but the feature still works properly.

The fix is probably needed somewhere around here: https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/devtools/client/inspector/changes/ChangesContextMenu.js#99-105
We create a new text selection here and then make it select the entire panel. Instead, we should make it select everything below the "select all" button.

This seems like a simple enough fix for someone who wants to try their first DevTools contribution.
If that's you, please make sure you go through the contribution docs first at https://docs.firefox-dev.tools and make sure you know how to get the code, build it locally, and test changes. Once you do, please take a look at that piece of code I mentioned and attempt a fix. Do not hesitate to ask any questions here as Razvan, I or other people can help you!

Keywords: good-first-bug
Whiteboard: [lang=js]

Hello, Patrick, I would love to work on this bug could you please assign it to me

Hey Adrian, thanks for your interest. I'll assign it to you now. Please let me know if you need anything to get started.

Assignee: nobody → princewurl510
Status: NEW → ASSIGNED

Thanks, Patrick

The buttons in the Changes panel already inherit an User Agent style (-moz-user-select: none) which prevents their contents from being selected. This can be seen when selecting, copying, then pasting the content somewhere -- the button text is missing, as expected.

The visual selection, however, still occurs. Selection.selectAllChildren() seems to ignore the -moz-user-select: none CSS declaration. Perhaps that requires investigation and logging a separate bug.

(Note that visual selection doesn't seem to occur when using the default keyboard shortcut for select all: Cmd+A / Ctrl+A)

Ideally, we'd want to avoid maintaining an explicit list of nodes to be selected vs nodes to be skipped. This list is not likely to be maintained over time. If a pure CSS solution can't be found, perhaps a filter for button nodes is a good enough compromise.

One way to achieve this, as suggested by Patrick in a separate thread, is the following:

Create a selection like today, loop over all code blocks in the panel, for each, create a Range (and set the right content with Range.selectNode() by skipping the buttons) and add those ranges to the selection object. And add a mochitest (if we don’t have one) to check the copied value, so we avoid future bugs.

This may be a bit more involved than the "good-first-bug" label implies, but we stand ready to help if you want to continue.

Ideally, a simple solution which respects the CSS declaration of -moz-user-select: none is best, because future changes for new nodes would only be needed in their CSS. This warrants a deeper investigation of CSS properties and/or attributes that will be respected by Selection.selectAllChildren().

Thanks for the information

Has Regression Range: no → ---

Hi Adrian. Could you please let us know if you still intend to work on this bug or not?
If you still need some time, that's fine, but just let us know.
If you don't plan on working on this anymore, that's fine too, we'll make this bug available to someone else.

Flags: needinfo?(princewurl510)
Depends on: 1546366

I investigated deeper and it looks like a platform implementation issue with the way Gecko highlights elements as selected when it shouldn't. I logged bug 1546366.

Back to this issue: the button content is highlighted but it is not copied to the clipboard, as expected.
Until the platform issue in bug 1546366 is addressed, we can work around this by negating the selection styles on buttons with a simple CSS trick:

button::selection {
  all: unset !important;
}

This rule overrides all selection styles on buttons (which shouldn't apply to begin with; buttons are not selectable anyway), and forces them to unset their selection-specific styles (color, background-color, etc) to the inherited values. This is the easiest approach I could find.

Once the platform bug is fixed, this CSS rule can be removed or it can stay. It is harmless as far as I can tell.

@Adrian, if you still want to work on this bug, the CSS rule needs to be added in changes.css, restricted to the buttons under the #sidebar-panel-changes container. If you prefer to work on another bug, I can do this.

Button elements are not user-selectable by default as defined by User Agent stylesheet defaults: https://www.w3.org/TR/css-ui-4/#issue-74a40dd9

However, by using the Selection.selectAllChildren() API, button elements do get a selection highlight (their text contents don't get copied to the clipboard; that is expected). See Bug 1546366.

Until bug 1546366 is addressed, this patch ensures that button elements in DevTools never get a selection highlight by unsetting any applied styles with the ::selection pseudo-element.

We want to fix and uplift this before the code freeze and Firefox 67 going to Release. In the interest of time, I will take this bug to land the fix quickly.

@Adrian, if you want to work on another bug, feel free to pick one from the good-first-bug list.

Assignee: princewurl510 → rcaliman
Flags: needinfo?(princewurl510)
Priority: P3 → P1
No longer depends on: 1546366
See Also: → 1546366
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f482629843e0
Ensure buttons don't have a selection highlight. r=pbro
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68

Thanks for landing the fix Razvan. I downloaded the latest nightly and tested. I can confirm that the problem is now fixed.

Status: RESOLVED → VERIFIED

Comment on attachment 9060359 [details]
Bug 1538648 - Ensure buttons don't have a selection highlight. r=pbro

Beta/Release Uplift Approval Request

  • User impact if declined: If we don't uplift this, users trying to select all of the content of the Changes tab in DevTools (via the context menu option) may get confused that buttons appear selected too while that's really not what they wanted (and, on top of this, not what will happen).
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • 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): This change isn't risky for several reasons:
  • it's a devtools only change, it won't impact 99% of our user population
  • it's a CSS only change which only impacts buttons displayed inside devtools so their text content can't be visually selected. That's it.
  • String changes made/needed:
Attachment #9060359 - Flags: approval-mozilla-beta?

Comment on attachment 9060359 [details]
Bug 1538648 - Ensure buttons don't have a selection highlight. r=pbro

Low risk and minimal patch, uplift approved for 67 beta 15, thanks!

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

Verified with 67.0b15 and 68.0a1 (2019-05-01) on Windows 10, macOS 10.13, Ubuntu 18.04.

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

Attachment

General

Created:
Updated:
Size: