Closed
Bug 1130330
Opened 10 years ago
Closed 9 years ago
Can't copy URL from "Rules"-panel
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox42 verified)
VERIFIED
FIXED
Firefox 42
Tracking | Status | |
---|---|---|
firefox42 | --- | verified |
People
(Reporter: elbart, Assigned: ntim)
References
()
Details
(Whiteboard: [parity-chrome][bugday-20150708])
Attachments
(1 file, 4 obsolete files)
16.92 KB,
patch
|
ntim
:
review+
|
Details | Diff | Splinter Review |
Visit URL, Inspect any of the tiles, and check the "element" under "Rules". Right-clicking on the URL of background-image shows the "Copy"-option disabled.
39.0a1 Copy was disabled http://i.imgur.com/rNxPaWP.png
Flags: needinfo?(thrnarrw)
Assignee | ||
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(thrnarrw)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8628831 -
Flags: review?(pbrosset)
Assignee | ||
Comment 3•9 years ago
|
||
The test I've added didn't get caught in the last patch
Attachment #8628831 -
Attachment is obsolete: true
Attachment #8628831 -
Flags: review?(pbrosset)
Attachment #8628834 -
Flags: review?(pbrosset)
Assignee | ||
Comment 4•9 years ago
|
||
Fixed some nits I didn't catch in my previous patch
Attachment #8628834 -
Attachment is obsolete: true
Attachment #8628834 -
Flags: review?(pbrosset)
Attachment #8628868 -
Flags: review?(pbrosset)
Comment 5•9 years ago
|
||
Comment on attachment 8628868 [details] [diff] [review] Patch v1.2 Review of attachment 8628868 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! Thanks Tim. ::: browser/devtools/styleinspector/test/browser_styleinspector_context-menu-copy-urls.js @@ +1,3 @@ > /* Any copyright is dedicated to the Public Domain. > http://creativecommons.org/publicdomain/zero/1.0/ */ > Since you're changing this test, can you also take this opportunity to add "use strict"; ? @@ +83,5 @@ > + if (type == "data-uri") { > + info("Click Copy Data URI and wait for clipboard"); > + yield waitForClipboard(() => view.menuitemCopyImageDataUrl.click(), expected); > + } > + else { nit: Put else on the same line as the closing } ::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties @@ +111,5 @@ > styleinspector.contextmenu.copyImageDataUrl=Copy Image Data-URL > > # LOCALIZATION NOTE (styleinspector.contextmenu.copyDataUri.accessKey): Access key for > # the rule and computed view context menu "Copy Image Data-URL" entry. > +styleinspector.contextmenu.copyImageDataUrl.accessKey1=I Are access keys localized? Do we need a new key ID for this change?
Attachment #8628868 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #5) > ::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties > @@ +111,5 @@ > > styleinspector.contextmenu.copyImageDataUrl=Copy Image Data-URL > > > > # LOCALIZATION NOTE (styleinspector.contextmenu.copyDataUri.accessKey): Access key for > > # the rule and computed view context menu "Copy Image Data-URL" entry. > > +styleinspector.contextmenu.copyImageDataUrl.accessKey1=I > > Are access keys localized? Do we need a new key ID for this change? I changed the accesskey to accesskey1 in this patch so this will get caught by the l10n team
Assignee | ||
Comment 7•9 years ago
|
||
Fixed nits.
Attachment #8628868 -
Attachment is obsolete: true
Attachment #8629313 -
Flags: review+
Assignee | ||
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ef44e608c9b
Comment 9•9 years ago
|
||
(In reply to Tim Nguyen [:ntim] from comment #6) > (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #5) > > ::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties > > @@ +111,5 @@ > > > styleinspector.contextmenu.copyImageDataUrl=Copy Image Data-URL > > > > > > # LOCALIZATION NOTE (styleinspector.contextmenu.copyDataUri.accessKey): Access key for > > > # the rule and computed view context menu "Copy Image Data-URL" entry. > > > +styleinspector.contextmenu.copyImageDataUrl.accessKey1=I > > > > Are access keys localized? Do we need a new key ID for this change? > > I changed the accesskey to accesskey1 in this patch so this will get caught > by the l10n team Yeah but, do accesskeys need to be translated? From https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices I see: " If you change the entity or property name of a string, and the string has an accompanying access key, command key, or tooltip, you should update their names as well for consistency. This change is fundamental for access keys, since many localization tools use the entity name to connect an access key to its label, for example to check if it's using a character not available in the original string: given entities "useBookmark.label" and "useBookmark.accesskey", if you change to "chooseBookmark.label" due to a string change, change the access key entity to "chooseBookmark.accesskey" to match it. " which makes me think that the word "accesskey" in the property name is important, and so we shouldn't change it. Francesco? Any guidelines here?
Flags: needinfo?(francesco.lodolo)
Comment 10•9 years ago
|
||
Comment on attachment 8629313 [details] [diff] [review] Patch v1.3 Review of attachment 8629313 [details] [diff] [review]: ----------------------------------------------------------------- To answer the questions. 1) Yes, accesskeys are localized. Unlike shortcuts (or commankeys), you need to match the label. "Show" with a fixed accesskey "w" wouldn't work in Italian, since "w" it's not available in the translation "Visualizza". 2) Yes, it's important to keep ".accesskey" to help tools figure out the association between label and accesskey. Not sure if they're smart enough to understand ".accesskey1", probably not. 3) Most important: you don't need to change the ID, simply update the accesskey for English. Given #1, each locale will have a completely different set of accesskeys, and they need to figure out on their own how to avoid conflicts.
Attachment #8629313 -
Flags: feedback-
Updated•9 years ago
|
Flags: needinfo?(francesco.lodolo)
Assignee | ||
Comment 11•9 years ago
|
||
Restored the older accesskey name per previous comment
Attachment #8629313 -
Attachment is obsolete: true
Attachment #8629350 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f7d5df6f37b7
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f7d5df6f37b7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Comment 14•9 years ago
|
||
I found the bug on Firefox nightly windows 32bit- Build ID 20150206030205 User Agent Mozilla/5.0 (Windows NT 6.3; rv:38.0) Gecko/20100101 Firefox/38.0 And this looks fixed on : Build ID 20150708030204 User Agent Mozilla/5.0 (Windows NT 6.3; rv:42.0) Gecko/20100101 Firefox/42.0
Assignee | ||
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
Updated•9 years ago
|
QA Whiteboard: [bugday-20150708]
Whiteboard: [parity-chrome] → [parity-chrome][bugday-20150708]
Comment 15•9 years ago
|
||
Successfully reproduce the bug on Firefox 39 (Build ID 20150630154324) Linux 32 bit. The fix works for me on Firefox 42.0a1, Build ID: 20150703030216, User Agent: Mozilla/5.0 (X11; Linux i686; rv:42.0) Gecko/20100101 Firefox/42.0 . Based on the Comment 14 and my verification, fix is verified.
Updated•9 years ago
|
Keywords: dev-doc-needed
Updated•9 years ago
|
Keywords: dev-doc-needed
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•