Closed Bug 1201750 Opened 9 years ago Closed 8 years ago

[computed view] Selecting text with keyboard (e.g. Ctrl+A) is unreliable - doesn't copy correct text to clipboard

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox43 affected, firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox43 --- affected
firefox54 --- fixed

People

(Reporter: arni2033, Assigned: djmdeveloper060796)

References

()

Details

Attachments

(2 files, 7 obsolete files)

STR: (Win7_64, Nightly 43, 32bit, ID 20150901030226, new profile, safe mode) 1. Open the following "data:" URL or click "URL" field in the form above data:text/html,<style>body{margin:0px;}</style> 2. Open Inspector, click <body> element, open "Computed" panel 3. Click on that panel's content (to focus it) 4. Press Ctrl+A to select all text, then Ctrl+C to copy it 5. Press Shift+F4 to open Scratchpad, then paste copied text there to see the result Result: Text was copied without ":" between names and values, and without ";" at the end of each rule Expectations: Text should be copied just like when I select all text with mouse (with ":" and ";")
Component: Developer Tools: Inspector → Developer Tools: Computed Styles Inspector
Summary: [ruleview] Selecting all text with Ctrl+A doesn't add ":" between name and value of rule AND doesn't add ";' in the end of line → [computed view] Selecting all text with Ctrl+A doesn't add ":" between name and value of rule AND doesn't add ";' in the end of line
Fairly certain this is a duplicate of bug 1222737. Please re-open if not.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
This applies to _all_ rules in computed view unlike bug 1222737. v_v
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Sorry for closing it again arni, but even if the 2 bugs are about different properties shown in the computed-view, the root cause is the same here. And since that other bug already has an assignee and a patch ongoing reviews, I prefer to close this one.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → DUPLICATE
This won't be fixed by bug 1222737 as "copy" event doesn't even fire in computed view anymore. It was already present when you closed this bug. I don't want to file another bug for the same issue, so please use this bug. If I detected this bug _now_, I'd have definitely mentioned comment 0 anyway. Bug 1238133 added this additional breakage. Regression range: > https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=de45c37454922316fb452f90b11f3b24ef6956dc&tochange=54bf7536f7006f279889b0abf9ab7deeb532e63c STR_2: 1. Open data:text/html,<style>body{margin:0px;}</style> 2. Open computed view, close and reopen devtools, select <body> in inspector, click in computed view 3. Doubleclick last word in computed view, press Ctrl+Shift+Home, press Ctrl+C, paste text somewhere AR: "\n \n margin-bottom0pxmargin-left0pxmargin-right0pxmargin-top0px" ER: "margin-bottom: 0px;\nmargin-left: 0px;\nmargin-right: 0px;\nmargin-top: 0px;\n" STR_3: 1. Open data:text/html,<style>body{margin:0px;}</style> 2. Open inspector, select <body>, open ruleview, open computed view 3. Select all text with Ctrl+A, copy it to clipboard, paste it somewhere AR: "element {}body {margin: 0px;}\n margin-bottom0pxmargin-left0pxmargin-right0pxmargin-top0px" ER: "margin-bottom: 0px;\nmargin-left: 0px;\nmargin-right: 0px;\nmargin-top: 0px;\n"
Blocks: 1238133
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
See Also: → 1278552
Summary: [computed view] Selecting all text with Ctrl+A doesn't add ":" between name and value of rule AND doesn't add ";' in the end of line → [computed view] Selecting text with keyboard (e.g. Ctrl+A) is unreliable - doesn't copy correct text to clipboard
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Hi, I would like to take up this bug. Please provide me with some pointers. Thanks
Flags: needinfo?(arni2033)
(In reply to Deepjyoti Mondal from comment #6) > Hi, I would like to take up this bug. Please provide me with some pointers. > Thanks I'm simply a bug reporter. You'll need to wait for Patrick Brosset's reply (I'm setting NI? request) Until that you can see bug 1222737 comment 5 @ Patrick Brosset (overview) Several "improvements" made the situation worse than it was. First, text selection after Ctrl+A only included \n in the beginning [1] After bug 1238133 - text selection includes text from ruleview [2] After bug 1259819 - text selection includes text tabs in inspector sidebar [3] After bug 1247729 - text selection now includes box model values if it's opened (The last 2 regressions should better be checked) I think [1] is already fixed by [2]. The remaining problem is to decide on Ctrl+A behavior to make it more logical. Here are 2 options which would be OK for me. X) Ctrl+A selects all rules if "Computed" tab is focused Y) Ctrl+A selects all rules if "Computed" tab is focused AND [rules (not box model) are focused OR box model is closed] Ctrl+A selects all values in box model if "Computed" tab is focused AND box model is focused AND box model is opened
Flags: needinfo?(arni2033) → needinfo?(pbrosset)
Forget to, mention [2] is a general problem, "rules" tab is also affected
As described in comment 7, this bug is now worse than it was when it was opened. Selecting all text in the computed-view (with ctrl+A) not only selects the text in this tab but also in the rule-view. Copying the text however only copies the text that's currently selected in the current tab, not in the other tabs (at least that's what I experienced). So I think the first thing to fix is to avoid text selection in background tabs. -moz-user-select:none should work fine for this. Honza: can you point Deepjyoti in the right direction for this? I don't remember where the code for the tab panels is these days. Then the second problem described here is, since bug 1247729, the box-model is in the computed-view, and that means that ctrl+A also selects the text inside the box-model. I tend to think that copy/pasting the values inside the box-model isn't very useful. If people agree on this, then -moz-user-select:none on the whole box-model UI would work nicely too. This should be done in devtools\client\themes\boxmodel.css Finally, the original problem mentioned in comment 0 needs to be fixed too. When the text gets selected in the computed-view and then pasted, the formatting is gone. I think the fix for this would need to go in the copySelection function in devtools\client\inspector\computed\computed.js
Flags: needinfo?(pbrosset) → needinfo?(odvarko)
(In reply to Patrick Brosset <:pbro> from comment #9) > So I think the first thing to fix is to avoid text selection in background > tabs. -moz-user-select:none should work fine for this. > Honza: can you point Deepjyoti in the right direction for this? I don't > remember where the code for the tab panels is these days. @Deepjyoti: 1) All the side panels are appended into the sidebar here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector.js#550 (look for `this.sidebar.addExistingTab`) Rules side panel is represented by RuleViewTool() Computed side panel by: ComputedViewTool etc. 2) First argument passed to `addExistingTab` is used as a key of the side panel, e.g. "ruleview" or "computedview". This key is used to calculate ID of the right DOM element that represents content of the panel. Element ID uses "sidebar-panel-" as a prefix. So for example, the Computed side panel's content is represented by a <div "sidebar-panel-computedview">. You can find it in inspector.xhtml file (in the same directory as inspector.js file above) 3) As soon as you have the <div> element you might apply CSS style to avoid selection (see Patrick's comment about -moz-user-select:none) 4) The object responsible for the side panel logic is in sub-dir, e.g. For Computed panel see: devtools/client/inspector/computed/computed.js (this is the place where ComputedViewTool and ComputedViewTool is) 5) There is also the 'Tab widget' itself, I don't know if you need it, but it lives in: devtools/client/shared/components/tabs directory Feel free to ask for more help! Honza
Flags: needinfo?(odvarko)
Thanks a lot for those informations Honza. I will look into those files as soon as possible.
Attached patch bug1201750.patch (obsolete) — Splinter Review
This is just a test patch (diff). I have updated boxmodel.css and tabs.css. Now the background tabs are no more selected and the text within the boxmodel is also not selected. The original problem is yet to be solved. I noticed the following : When I am clicking on the computed panel's content and pressing ctrl+A followed by ctrl+c and pasting it somewhere else, the formatting (done in copySelection function) is gone. However when I am using ctrl+A and then right-clicking the content and selecting copy from the context menu and pasting it somewhere else the formatting is retained. So when I am copying using ctrl+c is the copySelection function getting invoked at all?. Is there some other function which is handing ctrl+c event?
Flags: needinfo?(pbrosset)
Attachment #8810025 - Flags: review?(pbrosset)
Comment on attachment 8810025 [details] [diff] [review] bug1201750.patch Review of attachment 8810025 [details] [diff] [review]: ----------------------------------------------------------------- Good start, but more is needed. We need the *content* of the tabs that are hidden to not be selectable. So, when you select the computed-view for example, and press ctrl+A, then the content of the rule-view should not become selected. It is today. To achieve this, I think we need to add a new class to tab-panel-box elements inside devtools\client\shared\components\tabs\tabs.js. When one tab-panel-box is not selected, it should have a class like "hidden" or something. Using this class, we can add a new rule in devtools\client\shared\components\tabs\tabbar.css like: .tabs .tab-panel-box.hidden { -moz-user-select: none; } This way, when a tab is hidden from view, because another one is selected, then its content becomes unselectable. This would fix the first problem described in comment 9. Then, the second problem in comment 9 seems to be addressed in your patch. Great. Finally, the original problem still needs fixing. As for your question about the different between pressing ctrl+C and using the right-click copy menu item: you are right, they seem to be handled separately. The copy menu item is implemented here: devtools\client\inspector\shared\style-inspector-menu.js (see menuitemCopy in _openMenu). On click of the menu, this calls this.view.copySelection which basically ends up calling copySelection inside devtools\client\inspector\computed\computed.js. But when you press ctrl+C nothing seems to be happening. It's strange because we do have a "copy" event listener in devtools\client\inspector\computed\computed.js (see this.element.addEventListener("copy", this._onCopy) in this file, in the CssComputedView constructor). I looked at this more closely and found out that the _onCopy event handler is in fact called on ctrl+C but only if you select text within the list of properties in the computed-view. If you select all the text with ctrl+A, then this callback isn't called. That's probably because the event handler is only added on this.element, which is the container for the list of properties, instead we should add it to the whole computed-view panel container. I'll let you investigate a little bit more on this, and report back findings. I can try to spend more time investigating if that helps. Oh, also, sorry for the delay, I'm getting a bit behind on reviews at the moment and I don't expect to be able to get any quicker in the future. If that is a problem and you would like a fast turn around time, let me know and we'll figure out another reviewer who can get get to these reviews quicker than me. ::: devtools/client/themes/boxmodel.css @@ +229,4 @@ > > .boxmodel-editable { > border: 1px dashed transparent; > + -moz-user-select: none; I think you can remove this property completely, since there's already a -moz-user-select:none on the main container element.
Attachment #8810025 - Flags: review?(pbrosset) → feedback+
Flags: needinfo?(pbrosset)
Thanks a lot for the informations :) I will be uploading my next patch soon.
Hi Patrick,sorry for taking so long. I am stuck with the following : In order to prevent the text in background panels from getting selected I tried the following: In tabs.js : > return ( > DOM.div({ > key: index, > id: "panel-" + index, > style: style, > // added hidden class if not selected > className: selected ? "tab-panel-box" : "hidden", > role: "tabpanel", > "aria-labelledby": "tab-" + index, > }, > (selected || this.state.created[index]) ? tab : null > ) then in tabbar.css : > .tabs .hidden { > -moz-user-select: none; > } but it did not workout alternatively I tried this : tabs.js : > let style = { > visibility: selected ? "visible" : "hidden", > height: selected ? "100%" : "0", > width: selected ? "100%" : "0", > // add MozUserSelect property > MozUserSelect: selected ? "auto" : "none", > }; but still it did not work, the background text still gets selected > I looked at this more closely and found out that the _onCopy event handler is in fact called on > ctrl+C but only if you select text within the list of properties in the computed-view. If you select > all the text with ctrl+A, then this callback isn't called. That's probably because the event handler > is only added on this.element, which is the container for the list of properties, instead we should > add it to the whole computed-view panel container. I tried to apply the copy listener to the entire computedview panel by the following : > this.computedPanel = doc.getElementById("sidebar-panel-computedview"); > this.computedPanel.addEventListener("copy", this._onCopy); However Ctrl+C is still not working as desired
Flags: needinfo?(pbrosset)
See Also: → 1327771
(In reply to Patrick Brosset <:pbro> from comment #13) > Oh, also, sorry for the delay, I'm getting a bit behind on reviews at the > moment and I don't expect to be able to get any quicker in the future. If > that is a problem and you would like a fast turn around time, let me know > and we'll figure out another reviewer who can get get to these reviews > quicker than me. Sorry about the delay again. I feel really bad. Your request found me when I was away traveling for work, and then I took some time off for Christmas and now I'm lagging behind on review requests. I'm going to forward this to someone else on the team with fewer requests than me because otherwise I'm not going to be able to get to them. Sorry again. @julian: do you think you could help Deepjyoti and review the patches on this bug in the future?
Flags: needinfo?(pbrosset) → needinfo?(jdescottes)
Sure I'll try to take over reviews here :) Let's summarize the issues discussed here: #1 - ctrl+A in computed view also selects text in rule view This is actually har to reproduce right now, because clicking on a tab title clears the selection, so switching tab using the mouse doesn't trigger the issue. However if we go ahead with your fix and add -moz-user-select: none on the tab items this issue becomes much more problematic (because selection is no longer cleared when clicking on a tab) I think you are on the right track so just a few hints: - it would be better to add the hidden class rather than replacing tab-panel-box - there are other rules setting -moz-user-select (see in rules.css for instance), so we'll have to make sure our rule has a higher order than the exising ones. Also -moz-user-select is not inherited, so we need to make sure that all elements are targetted by our selector. The following selector seems to work nicely here: > .tabs .hidden, > .tabs .hidden * { > -moz-user-select: none !important; > } #2 - ctrl+A in computed view also selects tab titles Fixed by your current patch. #3 - ctrl+A in computed view also selects box model info Fixed by your current patch. #4 - formatting of the copied text is wrong I propose we first land all the CSS changes for #1, #2, #3 and then move on to this part! @Deepjyoti : assigning the bug to you. Have a look at my comments and let me know if you need anything else to work on this first patch.
Assignee: nobody → djmdeveloper060796
Status: REOPENED → ASSIGNED
Flags: needinfo?(jdescottes) → needinfo?(djmdeveloper060796)
@julian Thanks a lot for informations. I will be uploading a patch soon.
Flags: needinfo?(djmdeveloper060796)
Attached patch bug1201750.patch (obsolete) — Splinter Review
This is a test patch (diff file) this solves #1 - ctrl+A in computed view also selects text in rule view I think now we can move on to #4
Attachment #8810025 - Attachment is obsolete: true
Attachment #8826939 - Flags: review?(jdescottes)
Comment on attachment 8826939 [details] [diff] [review] bug1201750.patch Review of attachment 8826939 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! r+ with my review comments added, and also: - the "browser styles" checkbox label is still getting selected when doing CTRL/CMD+A. add a -moz-user-select: none; to #computedview-toolbar in computed.css - can you a patch instead of a diff before we land this ::: devtools/client/shared/components/tabs/tabs.css @@ +30,5 @@ > } > > +.tabs .hidden, > +.tabs .hidden * { > + -moz-user-select: none !important; maybe a comment here to indicate we do this to avoid "select all" commands from selecting content in hidden tabs? ::: devtools/client/themes/boxmodel.css @@ +13,4 @@ > max-width: 400px; > margin: 0px auto; > padding: 0; > + -moz-user-select: none; We should move this to #boxmodel-wrapper selector above, so that the "Box model" title is no longer selected by CTRL/CMD+A.
Attachment #8826939 - Flags: review?(jdescottes) → review+
Regarding the part 4, comment 13 from Patrick has all you need to get started I think. In computed.js we need to attach the onCopy event listener to an upper element (or on the document?). The downside here is that we might be getting selected text we are not interested in. We could try to loop on all the selection ranges in the current selection, but it looks like the selection range containing the text that interests us also contains nodes that are outside of the #propertyContainer. Would be nice to make sure we filter out unrelated selected text, but in case we fail to do so let's just try to have a nice mochitest covering this scenario. Short checklist of what is left to be done here: - find out the best node to attach the copy event listener - try to filter out unwanted text - add a test
Attached patch bug1201750.patch (obsolete) — Splinter Review
Made the necessary css changes.The browser style checkbox label and the Box model title is no longer selected.
Attachment #8826939 - Attachment is obsolete: true
Attachment #8828654 - Flags: review?(jdescottes)
I am working on part4, will give an update soon.
Comment on attachment 8828654 [details] [diff] [review] bug1201750.patch Review of attachment 8828654 [details] [diff] [review]: ----------------------------------------------------------------- Great, thanks! For future patches, if someone already gave you a R+ on your patch, that means you don't need to ask a review for the new revision of your patch :) Just set the R+ flag yourself once you implemented the changes mentioned by the reviewer.
Attachment #8828654 - Flags: review?(jdescottes) → review+
I tried attaching the onCopy listener to #computedview-container-focusable ,computedview-container and sidebar-panel-computedvie but it was of no use. When copying using Ctrl+A followed by Ctrl+C and pasting, the formatting is gone.
Flags: needinfo?(jdescottes)
(In reply to Deepjyoti Mondal from comment #25) > I tried attaching the onCopy listener to #computedview-container-focusable > ,computedview-container and sidebar-panel-computedvie but it was of no use. > When copying using Ctrl+A followed by Ctrl+C and pasting, the formatting is > gone. Try attaching it to the document: instead of > this.element.addEventListener("copy", this._onCopy); use > doc.addEventListener("copy", this._onCopy);
Flags: needinfo?(jdescottes)
However, with this approach we will need to check in which "unwanted" situations the event listener is triggered, and how to workaround them. The document here is the document for the whole inspector, so we can expect the event to be triggered in many unwanted situations A first thing would be to add a way to check the currently selected panel in the sidebar. But even then, you might select text in the rest of the UI (inspector search for instance) while the computed view is opened, but you don't want the computed view onCopy to modify the content of what was selected. Can you investigate what we can do if you look inside the selection and inspect the various ranges that are selected ? Maybe we can detect if the selection has text from the computed view. And if not, we don't do anything. https://developer.mozilla.org/en-US/docs/Web/API/Selection https://developer.mozilla.org/en-US/docs/Web/API/Range Not entirely this is feasible, if this is a dead end we'll check with the rest of the team if we should push forward or set as wontfix :)
Attached patch bug1201750_select.patch (obsolete) — Splinter Review
@Julian Thanks,attaching onCopy listener to doc helped. Now Ctrl+A and Ctrl+C is giving formatted text in the computed panel, however the formatting is getting applied to the rules panel also.I have tried out a work around for this problem by updating the copySelection function.If this is alright I will add comments and upload a formatted patch
Attachment #8828654 - Attachment is obsolete: true
Attachment #8829854 - Flags: review?(jdescottes)
Comment on attachment 8828654 [details] [diff] [review] bug1201750.patch This patch has not landed yet, so we shouldn't mark it as obsolete.
Attachment #8828654 - Attachment is obsolete: false
Comment on attachment 8829854 [details] [diff] [review] bug1201750_select.patch Review of attachment 8829854 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the diff (please prefer patches, they're easier to handle :)) Some issues but we should be on the right track. The current approach prevents using ctrl C in some areas: - edit as HTML in markup view - inspector search field - animation inspector As I mentioned in an earlier comment, our onCopy listener is being triggered much more often now, basically anytime you copy in the inspector. > _onCopy: function (event) { > this.copySelection(); > event.preventDefault(); > }, Since the listener calls preventDefault(), the "copySelection" method needs to be able to fill the clipboard correctly in any situation. Or we need to avoid the preventDefault() if the selection that we are copying is not compatible with copySelection(). For all the use cases I mentioned earlier win.getSelection().toString().trim(); is empty. What about checking this before calling this.copySelection() and event.preventDefault() ? We consider that if the selection is empty, then the copy event should most likely not be handled by our callback, but rather by the default behavior of the browser. ::: devtools/client/inspector/computed/computed.js @@ +692,4 @@ > try { > let win = this.styleWindow; > let text = win.getSelection().toString().trim(); > + let isPropertyPresent = 0; isPropertyPresent acts as a boolean, let's use false/true and not 0/1 :) Should also have a comment to explain the role of isPropertyPresent. As I understand, we set this flag when we spot a property name, and we assume the next selected line will be a property value. @@ +706,5 @@ > // Property name > result += prop; > } else { > // Property value > + if (isPropertyPresent == 1) { This fails eslint. Should be if (1) {...} else if (2) {...} else {...} instead of having a nested if-else. Also fails eslint for whitespace issues. You can run eslint by using ./mach eslint devtools @@ +710,5 @@ > + if (isPropertyPresent == 1) { > + result += ": " + prop + ";\n"; > + isPropertyPresent = 0; > + } else { > + result += prop + "\n"; This needs a comment
Attachment #8829854 - Flags: review?(jdescottes)
Attached patch bug1201750_select.patch (obsolete) — Splinter Review
Updated my previous patch and made the necessary changes.
Attachment #8829854 - Attachment is obsolete: true
Attachment #8830904 - Flags: review?(jdescottes)
Attached patch bug1201750-css.rebased.patch (obsolete) — Splinter Review
Rebased your first patch as it no longer applies cleanly on central.
Attachment #8828654 - Attachment is obsolete: true
Comment on attachment 8830904 [details] [diff] [review] bug1201750_select.patch Review of attachment 8830904 [details] [diff] [review]: ----------------------------------------------------------------- This looks OK. I sent your patch to try, let's see if this triggers any test regression. One small style nit to address. https://treeherder.mozilla.org/#/jobs?repo=try&revision=742f2a367649741698caf94cc06bd5c0adac2e0f ::: devtools/client/inspector/computed/computed.js @@ +177,4 @@ > this.shortcuts.on("Escape", this._onShortcut); > this.styleDocument.addEventListener("mousedown", this.focusWindow); > this.element.addEventListener("click", this._onClick); > + doc.addEventListener("copy", this._onCopy); let's move this two lines up and use this.styleDocument instead of doc, because this will pair nicely with the mousedown listener added on styleDocument at line 178
Attachment #8830904 - Flags: review?(jdescottes) → review+
Tests look good on try. I realized the changeset summary were a bit off: can you make sure to add a space in "bug1201750" between "bug" and "1201750". Thanks!
Flags: needinfo?(djmdeveloper060796)
Sure, I will upload a new patch ASAP :)
Flags: needinfo?(djmdeveloper060796)
Attached patch bug1201750_select.patch (obsolete) — Splinter Review
Updated my previous patch.
Attachment #8830904 - Attachment is obsolete: true
Attachment #8831781 - Flags: review+
Rebased both patches. Also updated commit summary (please stick to "Bug 1234567 - message;r=reviewer"). try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c14f402fc93a0401805ba41972124a58c2a5ebf1
Attachment #8831781 - Attachment is obsolete: true
Attachment #8832468 - Flags: review+
Attachment #8832466 - Attachment is patch: true
Attachment #8832468 - Attachment is patch: true
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7dcecb955445 Prevent "select all" command from selecting content in hidden tabs. r=jdescottes https://hg.mozilla.org/integration/mozilla-inbound/rev/5af446b47676 Selecting text with Ctrl+A now copies correct text to clipboard. r=jdescottes
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
I have reproduced this bug with Nightly 43.0a1 (2015-09-03) on Windows 7 , 64 Bit! This bug's fix is verified with latest Nightly! Build ID : 20170215030342 User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0 [bugday-20170215]
Product: Firefox → DevTools
Component: Inspector: Computed → Inspector
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: