Closed Bug 1429763 Opened 2 years ago Closed 2 years ago

Font preview is only shown when hovering the commas between the font names

Categories

(DevTools :: Inspector: Rules, defect, P3)

defect

Tracking

(firefox-esr52 unaffected, firefox58 unaffected, firefox59+ fixed, firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 + fixed
firefox60 --- fixed

People

(Reporter: sebo, Assigned: brennan.brisad)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

With the patch of bug 994559 in place the font preview is now only shown when hovering the commas between the fonts.

It should be shown anywhere on hovering the list of fonts.

Sebastian
The bug is here: https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/devtools/client/inspector/rules/rules.js#318-329
If the node passed also has the class ruleview-font-family, then it should be considered like a node that has the class ruleview-propertyvalue.
This way, the getNodeInfo, will return a VIEW_NODE_VALUE_TYPE and the code in the rule-view that displays tooltips will know that the hovered node is part of a CSS value that can be previewed.

Michael: are you potentially interested in fixing this bug?
It's fine if you aren't, I'll take care of it. FF59 goes to beta in 2 weeks, so we'd like to get this fixed before then, to avoid having to fix it a bit too late in the process and having less time to test. If now is not a good time for you, let me know.
Flags: needinfo?(brennan.brisad)
Sure, I'd be happy to fix it! Thanks for pinpointing the bug.
Flags: needinfo?(brennan.brisad)
Assignee: nobody → brennan.brisad
With this patch, the font preview renders the font name hovered, not just the font used.  So it seems like this should fix Bug 1429761 also.

But if a comma between fonts is hovered, it is still the used font that will be rendered in the font preview.  It might make more sense to prevent the font preview from showing if it is just a comma that is hovered, and just show it when an actual font is hovered.  What do you think?
(In reply to Michael Hoffmann from comment #4)
> It might make more sense to prevent the
> font preview from showing if it is just a comma that is hovered, and just
> show it when an actual font is hovered.  What do you think?

Totally agree. I assume showing the preview when hovering a comma is rather confusing.

Sebastian
(In reply to Michael Hoffmann from comment #4)
> But if a comma between fonts is hovered, it is still the used font that will
> be rendered in the font preview.  It might make more sense to prevent the
> font preview from showing if it is just a comma that is hovered, and just
> show it when an actual font is hovered.  What do you think?
I also agree. I'll hold off reviewing the patch until that's done then. Thanks.
Attached patch bug1429763.patch (obsolete) — Splinter Review
Updated patch which shows the tooltip when a font-family is hovered, but not when hovering the commas.

The bug affected the computed panel also, so I had to make some more changes than I originally thought.

One issue is that a tooltip will be rendered even for fonts that are not present on the system, so the user could get the false impression that the font preview actually shows a rendered version of the font.
Attachment #8942268 - Attachment is obsolete: true
Attachment #8942268 - Flags: review?(pbrosset)
Attachment #8943019 - Flags: feedback?(pbrosset)
Comment on attachment 8943019 [details] [diff] [review]
bug1429763.patch

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

These code changes look pretty good.
I think you're right though in your last comment: it will be confusing when fonts aren't available.
What about we only show the tooltip when hovering over actually used fonts? I think this is the best we can do.
Attachment #8943019 - Flags: feedback?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #8)
> I think you're right though in your last comment: it will be confusing when
> fonts aren't available.
> What about we only show the tooltip when hovering over actually used fonts?
> I think this is the best we can do.

I think for this bug it would be ok to show the tooltip only on used fonts. Or show it everywhere (when hovering the fonts or the commas) like before. But there is still the issue when there are several fonts used. If only the used fonts are displayed, it should show a sample of the hovered font, not the first of the used fonts.

The tooltip functionality can still be extended in bug 1429761.

Sebastian
Attached patch bug1429763.patchSplinter Review
Here's a new version of the patch.  I thought it was a good idea to let bug 1429761 handle the new functionality and let this bug restore the old functionality.  So now everything hovered, fonts or commas, renders the actual font used.

It flickers a little when hovering between fonts because of the spans, hope that is still ok.  Let me know what you think.
Attachment #8943019 - Attachment is obsolete: true
Attachment #8943780 - Flags: review?(pbrosset)
No longer blocks: 994559
Depends on: 994559
Status: NEW → ASSIGNED
Priority: -- → P3
Tracking for 59. We still have a chance to fix this before it gets to release.
Comment on attachment 8943780 [details] [diff] [review]
bug1429763.patch

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

Thanks Michael.
Are you able to push to try or do you want me to do this?
Attachment #8943780 - Flags: review?(pbrosset) → review+
Is everything good to go for checkin?
Flags: needinfo?(pbrosset)
Yes, all green. Let's get this in. I'll also ask if we can uplift this to 59, since it's a regression.
Flags: needinfo?(pbrosset)
Comment on attachment 8943780 [details] [diff] [review]
bug1429763.patch

Approval Request Comment
[Feature/Bug causing the regression]: regression from bug 994559
[User impact if declined]: If declined, users hovering (with the mouse) over font face names inside a font-family property in the DevTools inspector won't see the tooltip they used to see before bug 994559. This is a tooltip that displays a preview of the font as rendered in the page.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet, it hasn't landed in m-c yet. I thought I would request the uplift quickly though. This has been tested locally.
[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?]: Looking at the code, I would say this is not very risky. First of all, this is only in Devtools, so like 1% of Firefox users would see this. It's in the inspector, so most of Devtools users will see this, but again, only a few safe-looking lines. So I think we're fine.
[Why is the change risky/not risky?]: See above.
[String changes made/needed]: None
Attachment #8943780 - Flags: approval-mozilla-beta?
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8a684e84ca
Show font preview when hovering on font names. r=pbro
Keywords: checkin-needed
Comment on attachment 8943780 [details] [diff] [review]
bug1429763.patch

Fix for a regression in dev tools in 59, doesn't look too risky. 
Let's uplift for the beta 4 build.
Attachment #8943780 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/4e8a684e84ca
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.