Closed Bug 1442001 Opened 6 years ago Closed 6 years ago

Make the fonts panel UI even more compact and in line with the v0 mockups

Categories

(DevTools :: Inspector, enhancement, P2)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1442001 - Remove the expander icon for fonts and the used-as info;
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
screenshot
78.20 KB, image/png
Details 
I had a UI review with Victoria after bug 1437548, and she requested 2 changes:

- Removing the expander icon for fonts. There isn't enough information right now for each font that we'd need to collapse them.
- Removing the "used as" CSS font-family name. This information already is in the @font-face code snippet.

This is in line with the latest version of the mockup:
https://mozilla.invisionapp.com/share/Z3F7OGCTK#/screens/279202133_Font_Edit_Panel_V0

We need to get this simple UI/CSS change in time for 60.
I thought the expander was for the @font-face code snippet toggle.
There are 2 expanders: one that expands the font container itself, and one for the @font-face code.
We want to remove the first one, and keep the latter.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Priority: -- → P2
Comment on attachment 8954928 [details]
Bug 1442001 - Remove the expander icon for fonts and the used-as info;

Sorry, went too fast in asking for review. This isn't ready.
I still need to do 2 things:
- ask for a final review from Victoria (I'll provide her with a try build),
- fix tests
Attachment #8954928 - Flags: review?(jdescottes)
Comment on attachment 8954928 [details]
Bug 1442001 - Remove the expander icon for fonts and the used-as info;

https://reviewboard.mozilla.org/r/224096/#review230072


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/fonts/components/Font.js:144
(Diff revision 1)
>      } = this.props;
>  
>      let { previewText } = fontOptions;
>  
>      let {
>        CSSFamilyName,

Error: 'cssfamilyname' is assigned a value but never used. [eslint: no-unused-vars]
Hey Victoria, you will be able to download a version of Firefox with this change here in a bit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1c907716103896a28fecdcc97199090c292774c
Please let me know if you want to make further CSS alignment/spacing changes, and I'll incorporate them in the patch before asking for review.
Flags: needinfo?(victoria)
Hi Patrick, this is looking great! I suggest these UI tweaks:

Tightening up the spacing a little:

.font
remove `grid-gap: 10px;`

.font-format-url
margin-top: .2em;
color: gray-50

.font-css-code
margin-top: -.8em; (not actually this but I didn't figure out how to do it the correct grid way)

Preview text and input field:

- 0 border radius on focus ring
- Less top/bottom padding to match the side padding in the focus ring, if possible
- Should highlight "Abc" on click so user knows it will be changes if they type - but I see now that this is actually an image. Maybe a on-focus background color could work for now (could be same color as Inspector hover row color).
- Add a tooltip to this that says "Click to edit preview"

RE: System Font: it would be nice if the font preview for these wasn't so tiny

If no fonts found for current element, the "Other fonts in page" accordion header should have a top border

Would be nice if the click target for expanding @font-face code included the ... because it looks like a clickable button. Possibly the entire text there should be clickable.
Flags: needinfo?(victoria)
Thanks a lot!

(In reply to Victoria Wang [:victoria] from comment #7)
> .font
> remove `grid-gap: 10px;`
Done
> .font-format-url
> margin-top: .2em;
> color: gray-50
Done
> .font-css-code
> margin-top: -.8em; (not actually this but I didn't figure out how to do it
> the correct grid way)
Didn't find a way to do this. The problem is that the font preview is 50px high I think, and so I made it span 2 rows of the grid, so that it's as high as the font name and remote/url part. But it doesn't span all the way down to where the font-face rule is. Because if I do this, then the font-face rule can't span 2 columns. And we sort of need it to for when it's expanded.
> Preview text and input field:
> - 0 border radius on focus ring
Done
> - Less top/bottom padding to match the side padding in the focus ring, if
> possible
Done
> - Should highlight "Abc" on click so user knows it will be changes if they
> type - but I see now that this is actually an image. Maybe a on-focus
> background color could work for now (could be same color as Inspector hover
> row color).
Yeah, the complication comes from it being an image, and not text. So ideally, we would need to replicate the whole selection mechanism: blinking cursor, ability to select part of the text, move in the field with arrow keys, etc.
You can do all those things, cause there really is an input field there, but it's only used to capture user events, and the text is hidden. We just re-generate the image every time.
It would be pretty hard to make this feel like an actual input. We'd have to somehow measure the size of each character to know where the cursor should go and how to draw to the user selection.
Some background on why this is an image: the actual font only exists on the server (which could be a remote device). The toolbox itself may not have this font. So we can't use normal text in the toolbox. It has to be an image we render on the server (where the page really runs, and where the font exists), and send this to the toolbox for preview.
> - Add a tooltip to this that says "Click to edit preview"
Done
> RE: System Font: it would be nice if the font preview for these wasn't so
> tiny
Yeah, I don't really know where this is coming from. But it also happens today without this patch. So I'll file another bug for it.
> If no fonts found for current element, the "Other fonts in page" accordion
> header should have a top border
Done
> Would be nice if the click target for expanding @font-face code included the
> ... because it looks like a clickable button. Possibly the entire text there
> should be clickable.
I made both the triangle and ... clickable. I decided against making the text clickable, because that makes it harder to select/copy it when expanded.
Comment on attachment 8954928 [details]
Bug 1442001 - Remove the expander icon for fonts and the used-as info;

https://reviewboard.mozilla.org/r/224096/#review230190

Looks good! Just one nit and one follow up suggestion.

::: devtools/client/inspector/fonts/components/Font.js
(Diff revision 2)
>    }
>  
> -  renderFontCSS(cssFamilyName) {
> -    return dom.p(
> -      {
> -        className: "font-css-name"

There is a reference to font-css-name in devtools/client/inspector/fonts/test/head.js, we should remove it.

::: devtools/client/inspector/fonts/test/browser_fontinspector_expand-css-code.js:28
(Diff revision 2)
>    font-family: "bar";
>    src: url("bad/font/name.ttf"), url("ostrich-regular.ttf") format("truetype");
>  }`;
>    }, "Waiting for the font-face rule");
>  
>    let expander = fontEl.querySelector(".font-css-code .theme-twisty");

We might want to log a follow up to test that clicking on the expander also expands the code snippet.

::: devtools/client/inspector/fonts/test/head.js:91
(Diff revision 2)
> -  let twisty = fontEl.querySelector(".theme-twisty");
> -  twisty.click();
> -  await onExpanded;
> -}
> -
>  function isFontDetailsVisible(fontEl) {

That's the reference to font-css-name I mentioned earlier, but this method is no longer used and can be removed.
Attachment #8954928 - Flags: review?(jdescottes) → review+
While testing, saw a small issue with the theme twisty direction in RTL, filed Bug 1442205
Comment on attachment 8954928 [details]
Bug 1442001 - Remove the expander icon for fonts and the used-as info;

https://reviewboard.mozilla.org/r/224096/#review230192

::: devtools/client/inspector/fonts/components/Font.js
(Diff revision 2)
>      } = this.props;
>  
>      let { previewText } = fontOptions;
>  
>      let {
> -      CSSFamilyName,

Maybe we want to remove this from inspector/fonts/types ? https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/devtools/client/inspector/fonts/types.js#14

CSSFamilyName is still used in other places so the actor should still return it. We could keep documenting it here, or remove it since we no longer use it in the app. Not sure what is the best move here, I'll leave it up to you.
Thanks Julian for the review and for filing this RTL bug.
Here's try build for the commit you reviewed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5bdf87be27b0198c3def4482c59dcd4e55d9bdd
I have addressed your comments and will push a new commit.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4515d83faaa3
Remove the expander icon for fonts and the used-as info; r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/4515d83faaa3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Attached image screenshot — 
I don't know if it was this bug or bug 1437548 but are these large input fields really the expected result? It looks a bit weird. :)
Your screenshot still shows expanders next to the fonts, so it probably doesn't have the changes coming from this bug.
(In reply to Sören Hentzschel from comment #17)
> Created attachment 8955627 [details]
> screenshot
> 
> I don't know if it was this bug or bug 1437548 but are these large input
> fields really the expected result? It looks a bit weird. :)
If you update nightly today, you should see something a bit smaller, although the font preview input fields are still quite big.
And yes, this is intended.
Ah whoops, I missed all these updates till I looked up this bug just now - should've added myself to the CC list (doing this now). Is this intended to be done before the 60 merge? 

Great to see the changes in any case!

(In reply to Patrick Brosset <:pbro> from comment #8)
> Didn't find a way to do this. The problem is that the font preview is 50px
> high I think, and so I made it span 2 rows of the grid, so that it's as high
> as the font name and remote/url part. But it doesn't span all the way down
> to where the font-face rule is. Because if I do this, then the font-face
> rule can't span 2 columns. And we sort of need it to for when it's expanded.

Ah, interesting. I was debugging this a little more and made Firefox and the browser toolbox crash twice :). I think part of the problem is the font preview image has a lot of excess top/bottom spacing, and should be better aligned with the top of the left column. Could we trim that? Otherwise, adding these two styles gives me what I am looking for (focus ring in this example still has too much top/bottom padding). Not sure if this is too hacky:

.font-preview-container
margin-top: -1em;

.font-format-url
margin-bottom: .5em;

> Yeah, the complication comes from it being an image, and not text. So
> ideally, we would need to replicate the whole selection mechanism: blinking
> cursor, ability to select part of the text, move in the field with arrow
> keys, etc.
> ...

Thanks for this info. As I discussed with Razvan in slack, I don't think we should try to perfectly replicate the selection mechanism. I think it just needs some visual tweaking for now, which I will think about some more. One thing I was considering is - add a light blue background color to hint that 'Abc' is 'selected', and show that it will be blown away as soon as the user starts typing. 

> I made both the triangle and ... clickable. I decided against making the
> text clickable, because that makes it harder to select/copy it when expanded.

Sounds good!
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: