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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[[:Aleksej]]

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 <[e-mail]>
	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.

Comment 1

[[:Aleksej]]

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.

Comment 2

[Sami Jaktholm]

Attachment: bug-1158634-editable-font-previews-1-feature.patch

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!

Comment 3

[Sami Jaktholm]

Attachment: bug-1158634-editable-font-previews-2-test-cleanup.patch

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.

Comment 4

[Sami Jaktholm]

Attachment: bug-1158634-editable-font-previews-3-test.patch

Here's the test for the editable previews. I'll put this up for review with the feature when the time comes.

Comment 5

[Patrick Brosset <:pbro>]

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

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8605796 [details] [diff] [review]
bug-1158634-editable-font-previews-2-test-cleanup.patch

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

LGTM

Comment 7

[Sami Jaktholm]

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

Comment 8

[Sami Jaktholm]

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

Comment 9

[Sami Jaktholm]

Attachment: bug-1158634-editable-font-previews-1-test-cleanup.patch

Oops, forgot to update the commit message. Luckily the sheriffs didn't check this in yet.

Comment 10

[Sami Jaktholm]

Attachment: bug-1158634-editable-font-previews-2-other-cleanups.patch

Minor cleanups to font-inspector.js as requested. Hope I didn't miss anything.

Comment 11

[Sami Jaktholm]

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.

Comment 12

[Sami Jaktholm]

Attachment: bug-1158634-editable-font-previews-3-feature.patch

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!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/25cf178c2894

Comment 14

[Patrick Brosset <:pbro>]

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.

Comment 15

[Patrick Brosset <:pbro>]

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.

Comment 16

[Guilherme Lima]

Landed yesterday - https://hg.mozilla.org/mozilla-central/rev/25cf178c2894

Comment 17

[Sami Jaktholm]

Attachment: bug-1158634-editable-font-previews-3-feature.patch

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.

Comment 18

[Sami Jaktholm]

Attachment: bug-1158634-editable-font-previews-4-tests.patch

Here's a few tests for this feature.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9be1f2e4ca37

Comment 19

[Patrick Brosset <:pbro>]

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.

Comment 20

[Patrick Brosset <:pbro>]

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

Comment 21

[Sami Jaktholm]

Attachment: bug-1158634-editable-font-previews-3-feature.patch

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

Comment 22

[Sami Jaktholm]

Attachment: bug-1158634-editable-font-previews-4-tests.patch

Here's patch that addresses the comments.

Comment 23

[Patrick Brosset <:pbro>]

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

Comment 24

[Sami Jaktholm]

(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

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/33ced79e0973
https://hg.mozilla.org/integration/fx-team/rev/73027d05f9ca
https://hg.mozilla.org/integration/fx-team/rev/7b6f93f22d29

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/33ced79e0973
https://hg.mozilla.org/mozilla-central/rev/73027d05f9ca
https://hg.mozilla.org/mozilla-central/rev/7b6f93f22d29

Comment 27

[[:Aleksej]]

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