Return an ability to edit some font preview text like the "Abc", or remove it from the documentation.

RESOLVED FIXED in Firefox 41

Status

--
minor
RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: Aleksej, Assigned: sjakthol)

Tracking

36 Branch
Firefox 41

Firefox Tracking Flags

(firefox41 fixed)

Details

(URL)

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

4 years ago
In Inspector -> CSS -> Fonts, there is a preview text which is "Abc" by default, and was freely editable originally by being contenteditable.

The patch for bug 886041 made it non-editable (apparently a canvas image):

https://hg.mozilla.org/mozilla-central/rev/2273193cc525

Bug 886041 - Make the font inspector remotable; r=bgrins
author	Heather Arthur <fayearthur@gmail.com>
	Tue, 25 Nov 2014 07:36:44 -0800
changeset 217676	2273193cc525
parent 217675 	81b55d99c422
child 217677 	906e87fee4a6
pushlog:	2273193cc525
Bug 886041 - Make the font inspector remotable; r=bgrins


Regression window:
g 2014-11-26-03-02-07-mozilla-central-firefox-36.0a1.ru.linux-x86_64 ced1402861b8
b 2014-11-27-03-02-08-mozilla-central-firefox-36.0a1.en-US.linux-x86_64 cef590a6f946


The ability to edit the text was a feature, and is still mentioned in the documentation at MDN: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/View_fonts#Fonts_view  I guess it was removed by accident.
(Reporter)

Comment 1

4 years ago
Actually, it was discussed in the bug, but it was assumed the feature would be added again, or at least the documentation would be updated.  Correcting the summary.
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Font preview text "Abc" is not contenteditable any more, but constant instead. → Return an ability to edit some font preview text like the "Abc", or remove it from the documentation.
(Reporter)

Updated

4 years ago
No longer blocks: 886041
Severity: normal → minor
See Also: → bug 886041
(Assignee)

Comment 2

4 years ago
Here's a patch that restores this feature. Tests are coming in a separate patch a bit later as I just came up with few more cases they should check but wanted to put this out for feedback anyway.

It makes the images editable with the shared inplace-editor component and renders the edited previews with NodeActor.getFontFamilyDataURL which I modified to take custom options as a third parameter.

I added a new trait to indicate if getFontFamilyDataURL allows its defaults to be overridden. However, there's also TabTarget.getActorDescription which seems to allow the remote method signature to be inspected and make it possible to detect if the remote getFontFamilyDataURL takes the options or not.

Which of these two is the preferred method for preserving backwards compatibility? The trait seems unnecessary as the same information is implicitly available via getActorDescription but that method is not actually used anywhere to do this type of feature detection and it seems a bit messy.

Then there's the case that this patch makes font-inspector to use two different code paths for preview rendering (PageStyleActor for initial previews, NodeActor for edited previews) but I think that's something which cannot be solved in a backwards compatible way.

I'd like to get feedback at least on the trait vs. getActorDescription matter but other comments are welcome too!
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
Attachment #8605794 - Flags: feedback?(pbrosset)
(Assignee)

Comment 3

4 years ago
Here's a patch that moves the font-inspector initialization from the single existing test to head.js so that it can be used with other tests too. I also removed some unused imports and made some cleaning while at it.
Attachment #8605796 - Flags: review?(pbrosset)
(Assignee)

Comment 4

4 years ago
Here's the test for the editable previews. I'll put this up for review with the feature when the time comes.
Comment on attachment 8605794 [details] [diff] [review]
bug-1158634-editable-font-previews-1-feature.patch

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

Thanks for the patches Sami, I have applied them and the feature works well.

I have a general remark: Why not have an always visible text field header at the top of the panel that allows to change the text instead. Here are the problems I can see now with the UI:

1 - right now, the feature isn't very discoverable (but that could also be fixed just by changing the way the preview text look),
2 - when clicking on preview text to enter edit mode, the whole UI jumps, because the input field is a lot smaller than the preview text (in most cases),
3 - it's not possible to change all preview texts at once.

Having a global text field at the top would solve all of these at once and probably be more recognizable by people that are used to things like the google web font website or similar.

Of course I realize this is a new feature, while you were just reverting to what we had before as the bug suggested.
So I'd normally say: feel free to ignore me and file a follow-up bug for this feature instead, but:

- I feel like we could tackle both at the same time with little overhead,
- I think point #2 is rather problematic because when the feature was there before, that didn't happen (but of course now we have static images),
- and finally, after looking at the code, it seems like the proposal would solve some of the backwards compatibility hoops you had to go through (new trait, new options, comments to keep font-inspector.js + styles.js font weight + style in sync).

So I recommend going with my proposal.

::: browser/devtools/fontinspector/font-inspector.js
@@ +95,5 @@
>  
> +  /**
> +   * Returns a proper preview font color for the current theme.
> +   */
> +  getFillStyle: function() {

Almost all methods in this class are named:
undim: function FI_undim() {}
This is remnant from the past, but it's here anyway. So please, either submit a first cleanup patch that removes all the FI_* function names, or add the corresponding name for your function (I suggest the former: it's quick, and this file needs some cleanup anyway, there's a lot of extra trailing commas and missing semicolons, also DOMUtils is defined but never used).
Attachment #8605794 - Flags: feedback?(pbrosset)
Comment on attachment 8605796 [details] [diff] [review]
bug-1158634-editable-font-previews-2-test-cleanup.patch

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

LGTM
Attachment #8605796 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 7

4 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #5)
> Comment on attachment 8605794 [details] [diff] [review]
> I have a general remark: Why not have an always visible text field header at
> the top of the panel that allows to change the text instead.

That is actually a very good idea which I though about but didn't dare to deviate from the original functionality that much. It would definitely make things much simpler and avoid all those nasty issues mentioned here. It will also enable editing on older server versions so there's no need to worry about backwards compatibility.

I'll make it work like that instead. Thank you Patrick!
(Assignee)

Comment 8

4 years ago
The patch bug-1158634-editable-font-previews-2-test-cleanup.patch is ready to be checked in:

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5531a58c1d28
Keywords: checkin-needed, leave-open
(Assignee)

Comment 9

4 years ago
Oops, forgot to update the commit message. Luckily the sheriffs didn't check this in yet.
Attachment #8605796 - Attachment is obsolete: true
Attachment #8606595 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8605794 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8605891 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Minor cleanups to font-inspector.js as requested. Hope I didn't miss anything.
Attachment #8606597 - Flags: review?(pbrosset)
(Assignee)

Updated

4 years ago
Attachment #8606595 - Attachment description: bug-1158634-editable-font-previews-2-test-cleanup.patch → bug-1158634-editable-font-previews-1-test-cleanup.patch
(Assignee)

Comment 11

4 years ago
Comment on attachment 8606597 [details] [diff] [review]
bug-1158634-editable-font-previews-2-other-cleanups.patch

Updated the patch descriptions to better reflect the order of which these patches should be applied.
Attachment #8606597 - Attachment description: bug-1158634-font-inspector-cleanups.patch → bug-1158634-editable-font-previews-2-other-cleanups.patch
(Assignee)

Comment 12

4 years ago
This patch adds an input box similar to those in rule and computed views (minus the search icon) to the top of the font inspector which can be used to change the preview text.

This is quite a lot more simpler than the previous version and also supports older servers without any extra code. Feedback is welcome!
Attachment #8606730 - Flags: feedback?(pbrosset)
Comment on attachment 8606597 [details] [diff] [review]
bug-1158634-editable-font-previews-2-other-cleanups.patch

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

thanks for cleaning that up.
Attachment #8606597 - Flags: review?(pbrosset) → review+
Attachment #8606595 - Flags: checkin+
Comment on attachment 8606730 [details] [diff] [review]
bug-1158634-editable-font-previews-3-feature.patch

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

This looks great!
I have very little comments about this, and would be happy to R+ with:
- some tests added
- a better way to handle text overflow.

About text overflow: we're dealing with images here, so we can't wrap the text (unless the server did it, but it'd really be complex), so if you enter a long preview text it's going to overflow the container and a horizontal scrollbar is going to appear. That's fine. I think the only little problem is visual: only the text overflows, the rest of the UI doesn't expand with it, which I think is what we should fix. See: https://dl.dropboxusercontent.com/u/714210/bug-1158634-text-overflow.png
It'd be nice if the preview text field container would expand all the way to the right (not the text field itself though), same with the vertical 1px line font separators.
So this is very minor.

Another solution you could try if you want to is putting the font preview images inside scrollable containers instead, so they would have scrollbars, not the whole sidebar panel.

::: browser/devtools/fontinspector/font-inspector.js
@@ +78,5 @@
>  
>    /**
> +   * The text to use for previews. Returns either the value user has typed to
> +   * the preview input or DEFAULT_PREVIEW_TEXT if the input is empty or contains
> +   * only whitespace.

nit: missing @return {String}

::: browser/locales/en-US/chrome/browser/devtools/font-inspector.dtd
@@ +9,5 @@
>  <!ENTITY showAllFonts "See all the fonts used in the page">
>  <!ENTITY usedAs "Used as: ">
>  <!ENTITY system "system">
>  <!ENTITY remote "remote">
> +<!ENTITY previewHint "Preview Text">

This dtd file lacks localization notes: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Add_localization_notes

Can you at least add one for this new entity?
See inspector.dtd for some examples.
Attachment #8606730 - Flags: feedback?(pbrosset) → feedback+
(Assignee)

Comment 17

4 years ago
Here's a revised version of the patch.

It solves the overflow problem by making each preview individually scrollable  and adds the comment to the localization file.

I also noticed that the preview text updates caused flickering if there was a bunch of fonts present as the update method cleared the old previews before calling the server. This gave the browser a chance to temporarily render an empty panel before the new previews arrived. Thus I changed the update method to clear the old previews after the server has responded.
Attachment #8606730 - Attachment is obsolete: true
Attachment #8608123 - Flags: review?(pbrosset)
(Assignee)

Comment 18

4 years ago
Here's a few tests for this feature.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9be1f2e4ca37
Attachment #8608124 - Flags: review?(pbrosset)
Comment on attachment 8608123 [details] [diff] [review]
bug-1158634-editable-font-previews-3-feature.patch

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

Very nice, thanks.
I've got one last comment but I'm happy to R+ this right now:
when there are many fonts displayed and you have a vertical scrollbar, the preview text input should stick to the top and only the list of fonts should scroll (just like the search filter input in the rule-view).
You can either file a follow-up bug for this if you want to, or just fix it and mark the new patch as R+, this won't require a new review from me.
Attachment #8608123 - Flags: review?(pbrosset) → review+
Comment on attachment 8608124 [details] [diff] [review]
bug-1158634-editable-font-previews-4-tests.patch

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

::: browser/devtools/fontinspector/test/browser_fontinspector_edit-previews-show-all.js
@@ +17,5 @@
> +
> +  let normalTextNumPreviews =
> +    viewDoc.querySelectorAll("#all-fonts .font-preview").length;
> +
> +  let updated = inspector.once("fontinspector-updated");

s/updated/onUpdated

::: browser/devtools/fontinspector/test/browser_fontinspector_edit-previews.js
@@ +2,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";
> +
> +// Test that previews change when the preview text changes.

Can you repeat here what you explained in the commit message: that we don't check the content of the preview images, because they are images that can vary across OSs.

@@ +11,5 @@
> +  let { inspector, fontInspector } = yield openFontInspectorForURL(TEST_URI);
> +  let viewDoc = fontInspector.chromeDoc;
> +
> +  let previews = viewDoc.querySelectorAll("#all-fonts .font-preview");
> +  let initialPreviews = Array.prototype.map.call(previews, p => p.src);

simpler this way:
let initialPreviews = [...previews].map(p => p.src);

@@ +19,5 @@
> +  checkPreviewImages(viewDoc, initialPreviews, true);
> +
> +  info("Typing something else to the preview box.");
> +  yield updatePreviewText(fontInspector, "The quick brown");
> +  checkPreviewImages(viewDoc, initialPreviews, false /* previews changed */);

I don't think we need the inline comment /* previews changes */

@@ +40,5 @@
> + *        URI's are different.
> + */
> +function checkPreviewImages(viewDoc, originalURIs, assertIdentical) {
> +  let previews = viewDoc.querySelectorAll("#all-fonts .font-preview");
> +  let newURIs = Array.prototype.map.call(previews, p => p.src);

let newURIs = [...previews].map(p => p.src);

::: browser/devtools/fontinspector/test/head.js
@@ +195,5 @@
> +  info(`Changing the preview text to '${text}'`);
> +
> +  let doc = fontInspector.chromeDoc;
> +  let input = doc.getElementById("preview-text-input");
> +  let update = fontInspector.inspector.once("fontinspector-updated");

s/update/onUpdated
Attachment #8608124 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 21

4 years ago
Here's a patch that places the scrollbar to the correct place.

I have to say the CSS in that panel is rather complex mixing all kinds of different display styles and it took me ages to figure out how to make the scrolling work...
Attachment #8608123 - Attachment is obsolete: true
Attachment #8608806 - Flags: review+
(Assignee)

Comment 22

4 years ago
Here's patch that addresses the comments.
Attachment #8608124 - Attachment is obsolete: true
Attachment #8608807 - Flags: review+
(In reply to Sami Jaktholm from comment #21)
> I have to say the CSS in that panel is rather complex mixing all kinds of
> different display styles and it took me ages to figure out how to make the
> scrolling work...
Maybe file a "Simplify the font-inspector CSS layout structure" bug?
(Assignee)

Comment 24

4 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #23)
> Maybe file a "Simplify the font-inspector CSS layout structure" bug?

Filed as bug 1167263.

Here's another try run just to make sure I didn't break anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=043005f7107d
(Assignee)

Updated

4 years ago
Keywords: leave-open → checkin-needed
https://hg.mozilla.org/mozilla-central/rev/33ced79e0973
https://hg.mozilla.org/mozilla-central/rev/73027d05f9ca
https://hg.mozilla.org/mozilla-central/rev/7b6f93f22d29
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(Reporter)

Comment 27

4 years ago
Works in 2015-05-24-03-02-34-mozilla-central-firefox-41.0a1.ru.linux-x86_64, although characters not available in the font use fallback fonts (for example, "₽" always shows the Unifont's version).

Updated

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