Closed Bug 1444327 Opened 6 years ago Closed 6 years ago

Firefox web developer no longer showing URL of web font

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Version:
60 Branch
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox59 unaffected, firefox60 fixed, firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: felix-mozilla, Assigned: pbro)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Bug 1444327 - Bring back ability to see and copy font URLs;
59 bytes, text/x-review-board-request
jdescottes
: review+
jcristau
: approval-mozilla-beta+
Details
copy_12px.svg
1.36 KB, image/svg+xml
Details
icon-floating-far-right.png
132.33 KB, image/png
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180302182455

Steps to reproduce:

Went to https://www.heise.de/newsticker/
Used inspect element on one of the headlines



Actual results:

Got a web font display that gives me a link to the web font, but not the url (and won't let me right-click + copy link location on it!)


Expected results:

Earlier versions of Firefox told what the font name was, gave me the CSS that defined it, and also gave me the URL of the web font file.

This is important for debugging. I'm not here to download the web font. I want to know if the link is pointing to the right one. Please give me the URL in text and let me copy&paste it for comparison.
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
20180309003239

https://hg.mozilla.org/integration/autoland/json-pushes?changeset=0ad9403b418ebf2a09e4aebc7412efc14d480da4&full=1

* No context menu for "woff2" so it's not possible to copy the link to the font.
* Expanding the @font-face section shows the rule truncated.
* "src: url(" shows a relative path anyway, so it's not fit for purpose.
* No context menu there either, though at least Ctrl+C works.
Blocks: 1437548
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Font Inspector
Ever confirmed: true
Flags: needinfo?(pbrosset)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Yes this was indeed changed by bug 1437548 on purpose. This was part of a Font panel redesign effort.
There are more things coming to the Font panel very soon, and we needed to redesign it to accommodate for these new changes.

I would advise using the Network panel when it comes to debugging web font download. If a font file wasn't downloaded correctly because of a bad URL, it won't even be part of the Font panel. The Network panel however will give you a 404 (or other error message) for you to debug the problem.

Having said this, having a context menu on the font link that allows copying the URL sounds like a good way to preserve the feature without having the URL back in the UI and therefore compromising on the design changes we made.
Flags: needinfo?(pbrosset)
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Priority: -- → P2
The hope is to uplift this to 60. But it's new feature code and there's a new string. So not sure if it's really upliftable.
Comment on attachment 8958433 [details]
Bug 1444327 - Bring back ability to see and copy font URLs;

https://reviewboard.mozilla.org/r/227390/#review233230

Thanks for the patch Patrick! 
Two suggestions and some comments, let's go for another round based, assuming the test is missing the clipboard part.

::: devtools/client/inspector/fonts/components/Font.js:13
(Diff revision 1)
> +const Menu = require("devtools/client/framework/menu");
> +const MenuItem = require("devtools/client/framework/menu-item");
> +const clipboardHelper = require("devtools/shared/platform/clipboard");

All three dependencies could be lazy requires since they might not even be used at all if the user never right clicks on the link.

::: devtools/client/inspector/fonts/components/Font.js:61
(Diff revision 1)
> +    // XXX Missing menu API for specifying target (anchor)
> +    // and relative position to it. See also:
> +    // https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/openPopup
> +    // https://bugzilla.mozilla.org/show_bug.cgi?id=1274551
> +    let target = event.target;
> +    let rect = target.getBoundingClientRect();
> +    let screenX = target.ownerGlobal.mozInnerScreenX;
> +    let screenY = target.ownerGlobal.mozInnerScreenY;

The anchoring/positioning can be useful if you need to carefully position a popup next to a UI element. 

But for context menus we usually use event.screenX/screenY. Could make for a simpler implementation?

::: devtools/client/inspector/fonts/test/browser_fontinspector_copy-URL.js:8
(Diff revision 1)
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test that a context menu appears when right-clicking on a web font URL, and that the
> +// URL can be copied to the clipboard thanks to it.

The test doesn't check that the clipboard is updated. We should either update the comment (and remove subsuite = clipboard) or complete the test.

::: devtools/client/locales/en-US/font-inspector.properties:30
(Diff revision 1)
>  # tooltip on hover of a font preview string. Clicking on the string opens a text input
>  # where users can type to change the preview text.
>  fontinspector.editPreview=Click to edit preview
> +
> +# LOCALIZATION NOTE (fontinspector.copyURL): This is the text that appears in a menu item
> +# when a user right-click on the link for a remote font file in the font inspector.

nit: when a user right-click -> when a user right-clicks

::: devtools/client/locales/en-US/font-inspector.properties:32
(Diff revision 1)
>  fontinspector.editPreview=Click to edit preview
> +
> +# LOCALIZATION NOTE (fontinspector.copyURL): This is the text that appears in a menu item
> +# when a user right-click on the link for a remote font file in the font inspector.
> +# It allows the user to copy the URL for the font file to the clipboard.
> +fontinspector.copyURL=Copy link

Our context menu items are usually capitalized, so this would be "Copy Link".
Attachment #8958433 - Flags: review?(jdescottes)
Comment on attachment 8958433 [details]
Bug 1444327 - Bring back ability to see and copy font URLs;

https://reviewboard.mozilla.org/r/227390/#review233402

Thanks for the update! Works for me if try is green.
Attachment #8958433 - Flags: review?(jdescottes) → review+
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a88235c19594
Bring back ability to copy font URL by adding context menu; r=jdescottes
Fixing this now. Will push again to autoland soon.
Flags: needinfo?(pbrosset)
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a4f85c76c7c
Bring back ability to copy font URL by adding context menu; r=jdescottes
Approval Request Comment

[Feature/Bug causing the regression]: 1437548

[User impact if declined]: Prior to bug 1437548, the URLs for remote font was displayed in the Fonts panel of DevTools. Now it's not as easily accessible.
We realized however that this was an important information for people to troubleshoot their CSS/fonts.
If this patch is declined, then this feature will be lost in 60 until 61.

[Is this code covered by automated tests?]: Yes

[Has the fix been verified in Nightly?]: Not yet.

[Needs manual test from QE? If yes, steps to reproduce]: No.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: Very self-contained. Only in one panel of DevTools (which itself is used by ~1% of the Firefox users). And the code change only impacts one file. It's not risky.

[Why is the change risky/not risky?]: See above.

[String changes made/needed]: So, I did add a new string in the mozreview commit. But I wasn't sure if this would be making it impossible to uplift.
So if this prevents uplift, here is a similar patch but with the hardcoded string in English.
Attachment #8958846 - Flags: approval-mozilla-beta?
Backed out changeset 7a4f85c76c7c (bug 1444327) for Cl failurs on devtools/client/inspector/fonts/test/browser_fontinspector_copy-URL.js 

Log of the failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=167972392&repo=autoland&lineNumber=355854

Backout:
https://hg.mozilla.org/integration/autoland/rev/e3cf4f31fc7c894b0830c4660091b18ba94c3d69

Push that got backed out:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7a4f85c76c7c955ca581fafee0a47f9e082b2594
Flags: needinfo?(pbrosset)
Comment on attachment 8958846 [details] [diff] [review]
for-beta-uplift-with-hardcoded-string.patch

Let's cancel this for now. Working on the test timeout, and I've also talked to Victoria and we figured out some minor UI changes that I'd better get in the same patch.
Attachment #8958846 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Attachment #8958846 - Flags: approval-mozilla-beta?
Julian, here's the latest changes as discussed. Not huge, but leaving it up to you if you want to re-review.
Flags: needinfo?(jdescottes)
Comment on attachment 8958433 [details]
Bug 1444327 - Bring back ability to see and copy font URLs;

https://reviewboard.mozilla.org/r/227390/#review233830

Code looks good, feature feels harder to discover IMO.

::: devtools/client/inspector/fonts/components/Font.js:54
(Diff revision 4)
>  
> +  copyURL() {
> +    clipboardHelper.copyString(this.props.font.URI);
> +  }
> +
> +  onLinkContextMenu(event) {

no longer a link, onUrlContextMenu?

::: devtools/client/locales/en-US/font-inspector.properties:26
(Diff revision 4)
>  # tooltip on hover of a font preview string. Clicking on the string opens a text input
>  # where users can type to change the preview text.
>  fontinspector.editPreview=Click to edit preview
> +
> +# LOCALIZATION NOTE (fontinspector.copyURL): This is the text that appears in a menu item
> +# when a user right-clicks on the link for a remote font file in the font inspector.

"on the link" -> no longer a link

::: devtools/client/locales/en-US/font-inspector.properties:28
(Diff revision 4)
>  fontinspector.editPreview=Click to edit preview
> +
> +# LOCALIZATION NOTE (fontinspector.copyURL): This is the text that appears in a menu item
> +# when a user right-clicks on the link for a remote font file in the font inspector.
> +# It allows the user to copy the URL for the font file to the clipboard.
> +fontinspector.copyURL=Copy Link

no longer a link: "Copy Link" -> "Copy URL" ?
Depends on Bug 1440609 for test timeouts on Linux.
Depends on: 1440609
Flags: needinfo?(jdescottes)
Victoria, would it be possible to provide the copy icon as specified in https://mozilla.invisionapp.com/share/TUE5VV8MV#/screens/279202133 ?
For now, I'm repurposing a File icon that I display twice (using an ::after pseudo) to make it look like the copy icon, but the CSS is more complex than it should really be.
Flags: needinfo?(victoria)
Attached image copy_12px.svg — 
Attached an svg for the copy icon! (It's a smaller/thinner-lined version of the Photon version)
Flags: needinfo?(victoria)
Thank you Victoria for the quick reply and the icon!
Julian, here is the last version of the commit, ready for a new review.
Flags: needinfo?(jdescottes)
Looks good to me. Suggestions for follow-ups:
- add feedback after clicking on the icon
- icon should float next to the url (only an issue if the panel has a big width available and short urls)
Flags: needinfo?(jdescottes)
(just to illustrate my last followup suggestion)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #28)
Thank you Julian
> - add feedback after clicking on the icon
Agreed. Firefox screenshot uses OS notifications for this. I'll talk with Victoria about this.
> - icon should float next to the url (only an issue if the panel has a big
> width available and short urls)
I will fix this in this commit. It's a simple justify-self: start; CSS change.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/12a46d4d860a
Bring back ability to see and copy font URLs; r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/12a46d4d860a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Backout by nerli@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/efce78e62b6d
Backed out changeset 12a46d4d860a for frequently failing devtools/client/inspector/fonts/test/browser_fontinspector_copy-URL.js (bug 1446595) a=backout
Backed out changeset 12a46d4d860a (bug 1444327) for frequently failing devtools/client/inspector/fonts/test/browser_fontinspector_copy-URL.js (bug 1446595) a=backout

Push that got backed out: https://hg.mozilla.org/mozilla-central/rev/12a46d4d860afbb3bc3a08a961a4e08fe599bb6d

Back out: https://hg.mozilla.org/mozilla-central/rev/efce78e62b6dac195e7cb4898684da54155d1661
Status: RESOLVED → REOPENED
status-firefox61: fixed → ---
Resolution: FIXED → ---
Target Milestone: Firefox 61 → ---
Patrick: you probably want to fold the skip-if from Bug 1446595. See explanation in comments of the other bug.
Flags: needinfo?(pbrosset)
No longer depends on: 1440609
Thank you Julian for the investigation on that other bug. I'll do as you said: fold in your fix and land again here.
Flags: needinfo?(pbrosset)
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d57cfd8c5c39
Bring back ability to see and copy font URLs; r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/d57cfd8c5c39
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment on attachment 8958433 [details]
Bug 1444327 - Bring back ability to see and copy font URLs;

Approval Request Comment

[Feature/Bug causing the regression]: 1437548

[User impact if declined]: Prior to bug 1437548, the URLs for remote font was displayed in the Fonts panel of DevTools. After bug 1437548 (which landed in 60) it became harder to access.
We recently gathered some user feedback and realized that this was an important information for people to troubleshoot their CSS/fonts.
This is what this patch addresses.
If it is declined in beta, then this feature will be lost in 60 and then come back in 61 again.

[Is this code covered by automated tests?]: Yes

[Has the fix been verified in Nightly?]: Not yet.

[Needs manual test from QE? If yes, steps to reproduce]: No.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: Very self-contained. Only in one panel of DevTools (which itself is used by ~1% of the Firefox users). And the code change only impacts one file. It's not risky.

[Why is the change risky/not risky?]: See above.

[String changes made/needed]: I did add a new string in the patch:
fontinspector.copyURL=Copy URL
If it is a problem for uplifting to beta, then let me know, I can provide a patch that has this as hardcoded in english for beta only. It only appears in a tooltip on hover anyway.
Attachment #8958433 - Flags: approval-mozilla-beta?
ni?flod for feedback on the string change
Flags: needinfo?(francesco.lodolo)
It's OK to uplift as is.
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8958433 [details]
Bug 1444327 - Bring back ability to see and copy font URLs;

bring back a feature for the devtools font panel to avoid regressing it in 60, beta60+, with l10n ack from flod
Attachment #8958433 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Patrick, we had to land a follow-up on Beta to make ESLint happy. But the same code appears to pass without complaint on m-c? Any idea why?
https://hg.mozilla.org/releases/mozilla-beta/rev/8be55a86ae3b06ce9edce3df2ee8caafd5188e4b
Flags: needinfo?(pbrosset)
This is because of https://bugzilla.mozilla.org/show_bug.cgi?id=1443081 which landed in 61 and updated a very broad rule for DevTools.
Flags: needinfo?(pbrosset)
Product: Firefox → DevTools
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: