Copying attribute value to clipboard should use the actual attribute value, not the display value (i.e. not show ellipsis)

VERIFIED FIXED in Firefox 60

Status

defect
VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: sole, Assigned: davidwalsh)

Tracking

({good-first-bug})

unspecified
Firefox 60

Firefox Tracking Flags

(firefox60 verified)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

STR

1. Go to http://sole.github.io/test_cases/devtools/long-attribute-values/
2. Right click on the link, select "inspect element"
3. The inspector opens. Right click on the href attribute and select Attributes -> copy attribute value
4. Paste the value elsewhere

Expected:
The clipboard value is 

#012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789

(a long string of numbers)

Actual result:
The clipboard value is not the full original string, and has an ellipsis on it:

#01234567890123456789012345678901234567890123456789012345678…012345678901234567890123456789012345678901234567890123456789
Keywords: good-first-bug
The piece of code that gets the value to be copied is the following one:
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/devtools/client/inspector/markup/views/element-editor.js#204-208
As you can see, it gets the textContent of the attribute value node, which is wrong because it's already been shorten there.
It should instead go and get the actual attribute value.
I'll take a look to see where that is stored.
Well, it seems pretty simple.
From that function I linked to before, you have access to this.node.attributes.
This is an array of all attributes. So to fix this, we need to find the right attribute by its name, and then get the value from there.
Instructions to get started with getting the source code from mozilla's repository and building: http://docs.firefox-dev.tools/getting-started/build.html

Once you check out the code, go to devtools/client/inspector/markup/views/element-editor.js and edit it using the instructions above.

When you think you have fixed the problem, you can submit a patch using the instructions here: http://docs.firefox-dev.tools/contributing/making-prs.html
(Assignee)

Updated

a year ago
Assignee: nobody → dwalsh
(Assignee)

Comment 4

a year ago
Posted patch bug-1430143.patch (obsolete) — Splinter Review
Not sure who should be reviewing this so feel free to pass it on :sole!

Please let me know if I can make any changes!  Thank you!
Attachment #8942286 - Flags: review?(spenades)
Comment on attachment 8942286 [details] [diff] [review]
bug-1430143.patch

Review of attachment 8942286 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/markup/views/element-editor.js
@@ +207,5 @@
> +
> +      if (this.node.attributes) {
> +        let currentAttribute = this.node.attributes.find(a => a.name === name);
> +        if (currentAttribute) {
> +          value = currentAttribute.value;

as an alternative, the attreditor dataset stores these values too, so you could do:

  name = attribute.dataset.attr;
  value = attribute.dataset.value;
(Assignee)

Comment 6

a year ago
Posted patch bug-1430143.patch (obsolete) — Splinter Review
Updated per comment.

Also adding :jlast as reviewer as he may know a better person to redirect this to for review.
Attachment #8942286 - Attachment is obsolete: true
Attachment #8942286 - Flags: review?(spenades)
Attachment #8942944 - Flags: review?(spenades)
Attachment #8942944 - Flags: review?(jlaster)
Comment on attachment 8942944 [details] [diff] [review]
bug-1430143.patch

Review of attachment 8942944 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you David. This is a great way to fix this. I like it that the code changes are very small.
It would be great to add a new test case covering this though. To make sure this doesn't regress in the future.

Would you like to do this? If not, I can jump on this.
Here's some guidance for this:

- You can probably just extend an existing test: /devtools/client/inspector/test/browser_inspector_menu-05-attribute-items.js 
- It already has a testCopyAttributeValue function, but that test case doesn't cover long values
- I suggest adding a new test function after that: testCopyLongAttributeValue, and making sure it covers the case where the attribute value is so long that it gets an ellipsis in the inspector

We have some docs about adding/running tests here: http://docs.firefox-dev.tools/tests/
Attachment #8942944 - Flags: review?(spenades)
Attachment #8942944 - Flags: review?(jlaster)
Attachment #8942944 - Flags: feedback+
Sorry for stealing the review, but I think I am one of the best reviewers for this type of changes.
Patrick: No, you are the best reviewer for this! :)

David: thanks for the fix! Onward :)
(Assignee)

Comment 10

a year ago
Posted patch bug-1430143.patch (obsolete) — Splinter Review
Updated patch with test.  Sorry for not providing one on the first go round!
Attachment #8942944 - Attachment is obsolete: true
Attachment #8943342 - Flags: review?(pbrosset)
Comment on attachment 8943342 [details] [diff] [review]
bug-1430143.patch

Review of attachment 8943342 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks David. Looks good. I think you just missed a spot: this test loads a page in a tab: doc_inspector_menu.html
and that page is the one containing the various DOM nodes and attributes that the test uses.
So the element <p id="attributes"> in this page should also have your new long attribute.
Attachment #8943342 - Flags: review?(pbrosset)
(Assignee)

Comment 12

a year ago
Attached is the updated element.

Could you advise on actually using that element to retrieve its attribute values?  Right now the tests all explicitly set what `_openMenu` uses for `inspector.nodeMenuTriggerInfo`, so the test isn't truly testing the node.

Thank you!
Attachment #8943342 - Attachment is obsolete: true
Attachment #8943713 - Flags: review?(pbrosset)
Comment on attachment 8943713 [details] [diff] [review]
bug-1430143.patch

Review of attachment 8943713 [details] [diff] [review]:
-----------------------------------------------------------------

Oh you are right, sorry David, I didn't realize that this nodeMenuTriggerInfo thing was here, and didn't remember what it did. When I saw the element in the test HTML page, I just assumed that the test was doing the right thing and actually tested it.
Right now, this test isn't too interesting. It should really simulate a right-click on the attribute itself and test the clipboard, instead of injecting whatever it expects in the end.
So, I guess the right way to fix this would be to modify the getMenuItem function so it triggered the context menu click on the right attribute depending on what it is we want to test.

I don't want to block this patch from landing on this bad test though. We should fix it, but that shouldn't block your nice and useful change from landing. So I'll R+ this, and will file a new bug to make this test better.
Can you push to try or do you want me to do this?
Attachment #8943713 - Flags: review?(pbrosset) → review+
See Also: → 1431653
Duplicate of this bug: 1413861

Comment 16

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc1c587aa4ed
Ensure full attribute value is copied from within Inspector. r=pbro
Keywords: checkin-needed

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dc1c587aa4ed
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
I have reproduced this bug with  Nightly 59.0a1 (2018-01-12) on Windows 10, 64 Bit!

This Bug's fix is verified with latest Nightly!

Build ID   : 20180206220102
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
QA Whiteboard: [bugday-20180207]

Comment 19

a year ago
The issue is still reproducible on Ubuntu 16.04 

Beta 59.0b10
Build ID 	20180215111455
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0


[testday-20180216]

Comment 20

a year ago
Sorry for previous comment. I tested on Beta version mistakenly.


I have reproduced this bug with Nightly 59.0a1 (2018-01-12) on ubuntu 16.04

The bug's fix is now verified with Latest Nightly 60.0a1

Build ID 	20180216223539
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0


As per comment 18 and this, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20180207] → [bugday-20180207] [testday-20180216]
I can confirm this issue is also fixed on Firefox 60.0a1(2018-02-20)under macOS 10.13. Based on this and comments above (18 and 20), I will mark this issue as verified fixed.

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.