Closed Bug 1577372 Opened 5 years ago Closed 5 years ago

Working copy functionality was removed from page info window

Categories

(Firefox :: Page Info Window, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 70
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.

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:

  1. Open Firefox and navigate to a page e.g. https://www.mozilla.org/
  2. Select View Page Info from context menu
  3. Select any line on meta or media views
  4. Either use copy from context menu or keyboard shortcut
  5. Paste into another application e.g. text editor

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);
    }

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: nobody → ajvincent

[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.

Status: NEW → ASSIGNED

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.

(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?

As I cannot ask for r+ directly, setting f? so frg can review and confirm it is okay.

Attachment #9089380 - Flags: feedback?(frgrahl)

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.

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

Keywords: checkin-needed

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=264312922&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&classifiedState=unclassified&searchStr=linting%2Copt%2Cjavascript%2Cchecks%2Csource-test-mozlint-eslint%2Cjs%28es%29&revision=1dca0c6a9ad2cc92ccaf713983e975b28dec7eee

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

Flags: needinfo?(ajvincent)

eslint fixes have been applied.

Flags: needinfo?(ajvincent)
Keywords: checkin-needed

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

Keywords: checkin-needed
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
Attachment #9089380 - Flags: feedback?(frgrahl) → feedback+
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Keywords: leave-open
See Also: → 1585377
No longer depends on: 1508169
Target Milestone: --- → Firefox 70
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: