Working copy functionality was removed from page info window
Categories
(Firefox :: Page Info Window, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox-esr68 | --- | unaffected |
firefox67 | --- | unaffected |
firefox68 | --- | unaffected |
firefox69 | --- | unaffected |
firefox70 | + | fixed |
People
(Reporter: iannbugzilla, Assigned: WeirdAl)
References
(Regression)
Details
(Keywords: regression)
Attachments
(2 files)
Bug 1508169 removed the working copy functionality from Firefox's page info window.
Having just tested on linux x64 (Fedora 30):
- Firefox 68.0.2
- Firefox 69b16
- Firefox 70.0a1 (2019-08-28)
I can confirm that both the context copy menu item and the keyboard shortcut for copy were working in those versions, so there was no need for the code to have been removed.
Please can the changes from Bug 1508169 be backed out.
Assignee | ||
Comment 1•5 years ago
|
||
cc'ing jorgk for the equivalent SeaMonkey changes, which were nearly identical.
I have no objection to reverting some of the changes for just the pageInfo dialog, but I would feel much better about this if we could add an automated regression test as well for the specific pageInfo feature that we broke.
IanN, can you give me very specific steps to reproduce the correct behavior in Firefox 68/69? I apologize for the mess, but I had assurances that what I removed was dead code.
Done further testing, it was broken on Firefox 67 (well at least 67.0.4) but was working again by Firefox 68
Steps are:
- Open Firefox and navigate to a page e.g. https://www.mozilla.org/
- Select View Page Info from context menu
- Select any line on meta or media views
- Either use copy from context menu or keyboard shortcut
- Paste into another application e.g. text editor
Comment 3•5 years ago
|
||
Ah, this isn't working in any version for me on OSX but it seems fine on Windows. Weird that this is a cross-platform issue, I hadn't expected that.
Still, I don't think we need to back out the change from bug 1508169, you can still easily copy from the "location" field, no?
(In reply to Ian Neal from comment #2)
Done further testing, it was broken on Firefox 67 (well at least 67.0.4) but was working again by Firefox 68
I've done some more testing, I think the issue I had with Firefox 67 was the usual with multiple copy buffers and copy does work fine on all versions of Firefox I have managed to test on linux.
(In reply to Johann Hofmann [:johannh] from comment #3)
Ah, this isn't working in any version for me on OSX but it seems fine on Windows. Weird that this is a cross-platform issue, I hadn't expected that.
Still, I don't think we need to back out the change from bug 1508169, you can still easily copy from the "location" field, no?
Different views copy different information, so media copies location but on meta view it is the content. See https://searchfox.org/mozilla-central/source/browser/base/content/pageinfo/pageInfo.js#175-178
One option would be to change the pageInfoTreeView.prototype to have:
getCellValue: function(row, column) {
let col = (column != null) ? column : this.copycol;
return (row < 0 || col < 0) ? "" : (this.data[row][col] || "");
},
and then in doCopy function use getCellValue and have something like:
for (var row = min.value; row <= max.value; row++) {
tmp = view.getCellValue(row, null);
if (tmp)
text.push(tmp);
}
Assignee | ||
Comment 6•5 years ago
|
||
OK, I have a basic backout patch incorporating IanN's suggested changes, and I've tested it with his steps on Fedora 30. Phabricator attachment coming soon.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 7•5 years ago
|
||
Assignee | ||
Comment 8•5 years ago
|
||
[Tracking Requested - why for this release]:
Fixing a regression I introduced last week, backing out only the part that regressed with the small tweak IanN suggested in comment 5. Hoping this will be small enough to get in before the merge to mozilla-beta.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
Writing an automated test for this is proving harder than I expected. I was tinkering around in browser/base/content/test/pageinfo/browser_pageinfo_images.js, but on MacOS the cmd_copy is disabled and apparently for good reason. doCopy doesn't work on the Mac.
Which is funny, because when I tested it on Fedora, it worked fine.
Reporter | ||
Comment 10•5 years ago
|
||
(In reply to Alex Vincent [:WeirdAl] from comment #9)
Writing an automated test for this is proving harder than I expected. I was tinkering around in browser/base/content/test/pageinfo/browser_pageinfo_images.js, but on MacOS the cmd_copy is disabled and apparently for good reason. doCopy doesn't work on the Mac.
Which is funny, because when I tested it on Fedora, it worked fine.
Would it be worth fixing why doCopy doesn't work on the Mac in another bug and have the test disabled for the Mac until then?
Reporter | ||
Comment 11•5 years ago
|
||
As I cannot ask for r+ directly, setting f? so frg can review and confirm it is okay.
Assignee | ||
Comment 12•5 years ago
|
||
I'm using Phabricator to handle the patch, including review requests. :johann has just r+'d the patch, and now we just need fluent-reviewers to hopefully rubber-stamp the restoration of the localized strings.
(In reply to Ian Neal from comment #10)
Would it be worth fixing why doCopy doesn't work on the Mac in another bug and have the test disabled for the Mac until then?
I think so. I also think there's a bit of inconsistency in what various people are seeing. I still think I can write the test, but in the spirit of one task per bug I would recommend filing another ticket for the fix once this regression is taken care of.
Assignee | ||
Updated•5 years ago
|
Comment 13•5 years ago
|
||
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1dca0c6a9ad2
restore copy command via context menu to pageInfo dialog. r=johannh,fluent-reviewers,flod
Comment 14•5 years ago
|
||
Backed out changeset 1dca0c6a9ad2 (bug 1577372) for eslint failure at pageInfo.js on a CLOSED TREE.
Backout link: https://hg.mozilla.org/integration/autoland/rev/aa070cf72750b2bda6fac3591f100d98c75a5f22
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=264312922&repo=autoland&lineNumber=70
Log snippet:
[task 2019-08-30T19:19:27.151Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2019-08-30T19:19:27.151Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2019-08-30T19:19:27.151Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2019-08-30T19:19:27.151Z]
[task 2019-08-30T19:19:27.151Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2019-08-30T19:34:51.263Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/pageinfo/pageInfo.js:153:15 | Replace (column?!=?null)
with column?!=?null
(prettier/prettier)
[task 2019-08-30T19:34:51.263Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/pageinfo/pageInfo.js:154:12 | Replace (row?<?0?||?col?<?0)???""?:?(this.data[row][col]?||?"")
with row?<?0?||?col?<?0???""?:?this.data[row][col]?||?""
(prettier/prettier)
[task 2019-08-30T19:34:51.263Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/pageinfo/pageInfo.js:1216:9 | Expected { after 'if' condition. (curly)
[task 2019-08-30T19:34:51.263Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/pageinfo/pageInfo.js:1216:17 | Delete ??????????
(prettier/prettier)
[taskcluster 2019-08-30 19:34:51.604Z] === Task Finished ===
[taskcluster 2019-08-30 19:34:52.547Z] Unsuccessful task run with exit code: 1 completed in 949.543 seconds
Assignee | ||
Comment 15•5 years ago
|
||
eslint fixes have been applied.
Comment 16•5 years ago
|
||
Pushed by malexandru@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ec40f7e5d5
restore copy command via context menu to pageInfo dialog. r=johannh
Comment 17•5 years ago
|
||
bugherder |
Comment 18•5 years ago
|
||
bugherder uplift |
Comment 19•5 years ago
|
||
Comment on attachment 9089380 [details] [diff] [review] Suite patch to put back copy functionality Looks good. I will not file a separate backout bug for suite and just put it in comm-central
Comment 20•5 years ago
|
||
Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/0e49bcfec167 Working copy functionality was removed from page info window - suite patch. r=frg
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•2 years ago
|
Description
•