Select all option selects even the [Copy All Changes] button
Categories
(DevTools :: Inspector: Changes, defect, P1)
Tracking
(firefox67 verified, firefox68 verified)
People
(Reporter: cfogel, Assigned: rcaliman)
References
Details
(Keywords: good-first-bug, Whiteboard: [lang=js])
Attachments
(3 files)
(deleted),
image/png
|
Details | |
(deleted),
video/mp4
|
Details | |
(deleted),
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details |
Affected versions
- 67.0b4, 68.0a1 (2019-03-24)
Affected platforms
- Windows 10, macOS 10.11, ubuntu 18.04
Steps to reproduce
- Launch Firefox and open the Changes tab from the devTools inspector;
- Make any change in the rules inspector;
- Right click on the change (inside the changes tab);
- 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;
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 1•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Comment 2•6 years ago
|
||
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!
Comment 3•6 years ago
|
||
Hello, Patrick, I would love to work on this bug could you please assign it to me
Comment 4•6 years ago
|
||
Hey Adrian, thanks for your interest. I'll assign it to you now. Please let me know if you need anything to get started.
Comment 5•6 years ago
|
||
Thanks, Patrick
Assignee | ||
Comment 6•6 years ago
|
||
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()
.
Comment 7•6 years ago
|
||
Thanks for the information
Updated•6 years ago
|
Comment 8•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
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.
Assignee | ||
Comment 10•6 years ago
|
||
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.
Assignee | ||
Comment 11•6 years ago
|
||
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.
Comment 12•6 years ago
|
||
Comment 13•6 years ago
|
||
bugherder |
Comment 14•6 years ago
|
||
Thanks for landing the fix Razvan. I downloaded the latest nightly and tested. I can confirm that the problem is now fixed.
Comment 15•6 years ago
|
||
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:
Comment 16•6 years ago
|
||
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!
Comment 17•6 years ago
|
||
bugherder uplift |
Reporter | ||
Comment 18•6 years ago
|
||
Verified with 67.0b15 and 68.0a1 (2019-05-01) on Windows 10, macOS 10.13, Ubuntu 18.04.
Description
•