Closed Bug 738228 Opened 8 years ago Closed 8 years ago

Option to display used font faces

Categories

(Other Applications :: DOM Inspector, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

References

Details

Attachments

(1 file, 3 obsolete files)

inIDOMUtils has a method to extract the fonts used on a particular range. This could be displayed inside DOM Inspector, although we would have to limit it to a node rather than a range.
Oh, I forgot to mention that the method is new in Gecko 7.
Attached patch Proof of concept (obsolete) — Splinter Review
This was mostly copy and paste, so I've probably made some howlers...
Attachment #608313 - Flags: feedback?(Sevenspade)
Does this need a whole new viewer?  Seems like it would be nice if we could shoehorn this into the Computed Style viewer or the CSS Rules viewer.  It wouldn't be very pure, but a lot better for usability I think.

Those viewers map directly onto the underlying object models, which is an ideal that should be maintained, so we don't want to misrepresent what's happening.

What about a child row under the font-family row in the Computed Style viewer, or something similar in CSS Rules?  (Colored red, like anonymous nodes?)  Any other ideas?
Attachment #608313 - Flags: feedback?(Sevenspade)
(In reply to Colby Russell from comment #3)
> Does this need a whole new viewer?  Seems like it would be nice if we could
> shoehorn this into the Computed Style viewer or the CSS Rules viewer.  It
> wouldn't be very pure, but a lot better for usability I think.
I did consider the Computed Style viewer but the idea to use a separate viewer was because it works on text nodes and documents as well as elements.

> What about a child row under the font-family row in the Computed Style
> viewer, or something similar in CSS Rules?  (Colored red, like anonymous
> nodes?)  Any other ideas?
Well, the used font doesn't necessarily relate to the computed font because child elements might change it... I would be happy to move my current tree into a second pane in Computed Style, styled similarly to CSS Rules' panes.
Attached patch Addressed review comments (obsolete) — Splinter Review
Attachment #608451 - Flags: feedback?(Sevenspade)
In that case, I guess I prefer the separate viewer approach.
Comment on attachment 608451 [details] [diff] [review]
Addressed review comments

See comment 6.
Attachment #608451 - Flags: feedback?(Sevenspade)
Attached patch Proposed patch (obsolete) — Splinter Review
As per attachment 608313 [details] [diff] [review] but including l10n.
Assignee: nobody → neil
Attachment #608313 - Attachment is obsolete: true
Attachment #608451 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #623492 - Flags: review?(Sevenspade)
Comment on attachment 623492 [details] [diff] [review]
Proposed patch

> +      ins:uid="usedFontFaces"
> +      ins:panels="bxObjectPanel bxObjPanel"
> +      ins:description="&usedFontFaces.label;">
> +        <ins:filter><![CDATA[
> +          return object instanceof Components.interfaces.nsIDOMNode &&
...
> +function UsedFontFacesView(aObject)
> +{
> +  if (aObject instanceof Components.interfaces.nsIDOMDocumentType) {
> +    this.mFontList = [];
> +    this.mRowCount = 0;

Shouldn't this check just be in the filter, along with some others, like comment nodes and processing instructions?

> diff --git a/resources/content/viewers/usedFontFaces/usedFontFaces.js b/resources/content/viewers/usedFontFaces/usedFontFaces.js
> new file mode 100644
> --- /dev/null
> +++ b/resources/content/viewers/usedFontFaces/usedFontFaces.js

I see you did a git style diff.  Did you do this because you're basing this off styleRules.js, or was there another reason?  If it's the former, you have to do |hg copy| in conjunction with the git diff to get it to pick up on the fact that this is a copy.  Use |--after| to register it after-the-fact.

> @@ -0,0 +1,182 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*****************************************************************************
> +* UsedFontFacesViewer --------------------------------------------------------
> +*  The viewer for the computed CSS styles on a DOM element.

Are you sure?

> +  this.mDOMUtils = XPCU.getService("@mozilla.org/inspector/dom-utils;1", "inIDOMUtils");

This is longer than 80 columns.  There are a few other places where this happens.

> +
> +  this.mTree = document.getElementById("olFonts");
> +}
> +
> +//XXX Don't use anonymous functions

:)

> +  initialize: function CSVr_Initialize(aPane)

You probably meant to change the function names.

> +  getCommand: function CSVr_GetCommand(aCommand)
> +  {
> +    if (aCommand == "cmdEditCopy") {
> +      return new cmdEditCopy(this.mTreeView.getSelectedRowObjects());

You probably want to copy the font name, or something, instead of the object.  This copies the string "[xpconnect wrapped nsIDOMFontFace]".

> +    }
> +    return null;

No support for copying the URI (cmdEditCopyFileURI)?

> +function UsedFontFacesView(aObject)
> +{
> +  if (aObject instanceof Components.interfaces.nsIDOMDocumentType) {
> +    this.mFontList = [];
> +    this.mRowCount = 0;
> +  } else {

else on a new line

> +UsedFontFacesView.prototype.getCellText = function UFFV_GetCellText(aRow, aCol)
> +{
...
> +}
> +
...
> +UsedFontFacesView.prototype.getRowObjectFromIndex =
> +  function UFFV_GetRowObjectFromIndex(aIndex)
> +{
> +  return this.mFontList.item(aIndex);
> +}

Nit: Please put semi-colons at the end of these assignments.  I know styleRules.js doesn't have them, but this has already been taken care of in several of the other viewers.
Attachment #623492 - Flags: review?(Sevenspade) → review-
I also got some errors in the console when trying this patch, and the viewer died once.  It might have had something to do with exotic nodes (comments, processing instructions), since that's what I was testing at the time.
(In reply to Colby Russell from comment #9)
> (From update of attachment 623492 [details] [diff] [review])
> > +      ins:uid="usedFontFaces"
> > +      ins:panels="bxObjectPanel bxObjPanel"
> > +      ins:description="&usedFontFaces.label;">
> > +        <ins:filter><![CDATA[
> > +          return object instanceof Components.interfaces.nsIDOMNode &&
> ...
> > +function UsedFontFacesView(aObject)
> > +{
> > +  if (aObject instanceof Components.interfaces.nsIDOMDocumentType) {
> > +    this.mFontList = [];
> > +    this.mRowCount = 0;
> Shouldn't this check just be in the filter
I could do but I didn't want the page to flip just because you'd click on a document type.

> along with some others, like comment nodes and processing instructions?
The API actually uses a range. For some reason, you can't select the contents of a document type with a range, but you can select comment nodes and PIs.

> I see you did a git style diff.  Did you do this because you're basing this
> off styleRules.js, or was there another reason?
It's the default for mozillabuild. It's also useful when dealing with images.

> If it's the former, you
> have to do |hg copy| in conjunction with the git diff to get it to pick up
> on the fact that this is a copy.  Use |--after| to register it
> after-the-fact.
It was actually based off computedStyle.js (the comment below should have been a giveaway!) but the copy diff would be bigger than an add so unless you really want it I won't bother.

> > +* UsedFontFacesViewer --------------------------------------------------------
> > +*  The viewer for the computed CSS styles on a DOM element.
> Are you sure?
Oops ;-)

> > +  this.mDOMUtils = XPCU.getService("@mozilla.org/inspector/dom-utils;1", "inIDOMUtils");
> This is longer than 80 columns.  There are a few other places where this
> happens.
Yes, that was copied from computedStyle.js too...

> > +//XXX Don't use anonymous functions
> :)
Also copied...

> > +  initialize: function CSVr_Initialize(aPane)
> You probably meant to change the function names.
Oops... I did in some places, but I missed these.

> > +  getCommand: function CSVr_GetCommand(aCommand)
> > +  {
> > +    if (aCommand == "cmdEditCopy") {
> > +      return new cmdEditCopy(this.mTreeView.getSelectedRowObjects());
> You probably want to copy the font name, or something, instead of the
> object.  This copies the string "[xpconnect wrapped nsIDOMFontFace]".
> > +    }
> > +    return null;
> No support for copying the URI (cmdEditCopyFileURI)?
Again, cargo-culted from computedStyles.js; I will investigate.
Attached patch Revised patchSplinter Review
Note that not all points were addressed, so to speak.
Attachment #623492 - Attachment is obsolete: true
Attachment #624879 - Flags: review?(Sevenspade)
Comment on attachment 624879 [details] [diff] [review]
Revised patch

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

::: resources/content/viewers/usedFontFaces/usedFontFaces.js
@@ +199,5 @@
> +  return this.mFontList.item(aIndex);
> +};
> +
> +/**
> + * Copy the URI for a downloaded font onto the clipboard.

s/URI for/name of/

(In reply to neil@parkwaycc.co.uk from comment #11)
> (In reply to Colby Russell from comment #9)
> > (From update of attachment 623492 [details] [diff] [review])
> > > +      ins:uid="usedFontFaces"
> > > +      ins:panels="bxObjectPanel bxObjPanel"
> > > +      ins:description="&usedFontFaces.label;">
> > > +        <ins:filter><![CDATA[
> > > +          return object instanceof Components.interfaces.nsIDOMNode &&
> > ...
> > > +function UsedFontFacesView(aObject)
> > > +{
> > > +  if (aObject instanceof Components.interfaces.nsIDOMDocumentType) {
> > > +    this.mFontList = [];
> > > +    this.mRowCount = 0;
> > Shouldn't this check just be in the filter
> I could do but I didn't want the page to flip just because you'd click on a
> document type.

That's bug 166749, but oh well.
Attachment #624879 - Flags: review?(Sevenspade) → review+
(Colby Russell :crussell wrote in comment #13)
> That's bug 166749, but oh well.

Actually, could you put an XXX comment mentioning bug 166749 in the filter, and a comment in the UsedFontFaceView constructor explaining the doctype/range special case?
Pushed changeset ddaa74ce685c to dom-inspector.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.