Last Comment Bug 473576 - font-family text attribute should expose actual font used
: font-family text attribute should expose actual font used
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 All
: -- normal with 1 vote (vote)
: mozilla13
Assigned To: alexander :surkov
:
Mentors:
Depends on: 467669
Blocks: textattra11y a11ynext 728907
  Show dependency treegraph
 
Reported: 2009-01-14 08:43 PST by Aaron Leventhal
Modified: 2012-03-02 03:25 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (9.85 KB, patch)
2009-03-04 01:49 PST, alexander :surkov
no flags Details | Diff | Splinter Review
patch (18.54 KB, patch)
2012-02-23 05:29 PST, alexander :surkov
karlt: feedback+
Details | Diff | Splinter Review
patch2 (18.51 KB, patch)
2012-02-27 06:51 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Aaron Leventhal 2009-01-14 08:43:38 PST
If the font-family rule uses a comma separated list of preferred fonts, we only want to expose the font that's actually being used. The user won't want to hear a list of 4 fonts.

We do this currently for ISimpleDOMText::fontFamily, so the code for that is here:
http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsTextAccessibleWrap.cpp#286

It does look a bit long and complex though, we should see if there's a quicker way to get the real font used by the current frame.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-01-14 14:53:29 PST
The text might use a mixture of fonts, when some characters in the text are not supported by some of the fonts.
Comment 2 Aaron Leventhal 2009-01-14 15:03:57 PST
Can we find out the main font used? That's what the screen reader user wants to know. The rest is more like an implementation detail, right?
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-01-14 15:36:37 PST
Depends on what you mean by the main font. If you can define that precisely, then I'm sure we can write code to find it :-).
Comment 4 Aaron Leventhal 2009-01-14 15:41:28 PST
Like, if the author defines
Times, Ariel, Helvetica
and it uses at least some Times, I'll go with that.
Comment 5 Aaron Leventhal 2009-01-14 15:43:14 PST
If that is a lot of code then I'd like to try to find a simpler rule. 

Since the user wants to know what the author's intent was, we could just go with the first font listed if we must.
Comment 6 alexander :surkov 2009-03-04 01:49:34 PST
Created attachment 365402 [details] [diff] [review]
wip

I sent email to IA2 group for clarification. This patch exposes one font-family if several ones were specified. But I guess we shouldn't expose generic font families like "serif" (this patch does it), instead we should expose really used font-family.
Comment 7 alexander :surkov 2009-06-04 22:47:25 PDT
Cc'ing Karl.

How can we obtain real font family when generic font family  is used (like "monospaced", "serif")? So if for example (possibly wrong example) if "serif" is mapped to "Times" font family then we should return "Times" font family. Would gfxFontEntry::Name() return what we need?
Comment 8 Karl Tomlinson (:karlt) 2009-06-04 23:37:26 PDT
(In reply to comment #7)
> How can we obtain real font family when generic font family  is used (like
> "monospaced", "serif")? So if for example (possibly wrong example) if "serif"
> is mapped to "Times" font family then we should return "Times" font family.
> Would gfxFontEntry::Name() return what we need?

gfxFontEntry::Name() is actually identifies a face, a particular font file.
On some platforms, in some situations, this is a family name, but in others it does not.  I don't think this is going to be a good solution.

I wonder whether looking up the pref families from generics might be a good approximation.  This is what happens here:
http://hg.mozilla.org/mozilla-central/file/d2b77efac151/gfx/thebes/src/gfxFont.cpp#l988
Comment 9 Sebastian Zartner 2010-05-03 00:02:58 PDT
Note, that the users can define in the preferences which font to use for the generic styles. I think that is what  intended to say.
Also it shouldn't be too complicated to return the right font, since Firefox already uses the right one for displaying (either a font from the list, for generic fonts the one defined in the preferences or if none of the list is found the default one from the preferences). Can't this function be used for retrieving the right font family calling getComputedStyle()?

This is also mentioned in an issue at Firebug:
http://code.google.com/p/fbug/issues/detail?id=3052
Comment 10 alexander :surkov 2011-03-02 23:47:05 PST
Perhaps a different bug but may have the same fix.

from the report "When I get the text attributes for the "self description" edit field at: http://archive.dojotoolkit.org/nightly/checkout/demos/form/demo.html
The font-family comes back as a \ delimited list of font names."

dbaron: surkov, font fallback is character-by-character
dbaron: surkov, if one font doesn't have a glyph, we look in the next font

(In reply to comment #1)
> The text might use a mixture of fonts, when some characters in the text are not
> supported by some of the fonts.

It sounds textAttributes can handle this and return one font, if the font is changed from character to character then it should split text on ranges where the font is persistent.
Comment 11 alexander :surkov 2011-03-02 23:47:53 PST
(In reply to comment #9)
> Can't this function be used for
> retrieving the right font family calling getComputedStyle()?

No, getComputedStyle returns list of font families.
Comment 12 Tony Mechelynck [:tonymec] 2011-06-24 07:56:24 PDT
On GTK2 (Linux) systems, isn't this problem made more difficult by the fact that Pango will try to find a glyph in any installed font if the requested font has no glyph for the specified character? There is no guarantee that even the system's serif, sans-serif and monospace fonts will each have a glyph for every valid Unicode codepoint.
Comment 13 Jonathan Kew (:jfkthame) 2011-06-24 14:28:01 PDT
Now that bug 467669 has landed, you can use the nsRange::GetUsedFontFaces() API to find out exactly what font(s) we're using for a given DOM range.

However, it's not clear to me whether that is always what you'd want, from an a11y perspective. If the author has simply called for "sans-serif", *that* expresses their intent - the fact that certain characters in the text happened to get drawn from Arial because Helvetica didn't include them may be largely irrelevant, and reflecting that in an audio description introduces apparent complexity that the author didn't intend and may never have noticed in the visual rendering.
Comment 14 Jonathan Kew (:jfkthame) 2011-06-24 14:32:22 PDT
(In reply to comment #12)
> On GTK2 (Linux) systems, isn't this problem made more difficult by the fact
> that Pango will try to find a glyph in any installed font if the requested
> font has no glyph for the specified character?

We do such font fallback on all systems, not just Linux (although the behavior of the Pango + fontconfig version used on Linux may be somewhat different from the implementation we have on Mac & Windows).
Comment 15 James Teh [:Jamie] 2011-08-29 20:37:22 PDT
This is a problem for NVDA users, particularly now that text formatting is exposed in browse mode. This causes NVDA to report all of the possible font families, which is fairly annoying at best and very confusing at worst.
Comment 16 alexander :surkov 2012-02-23 05:29:32 PST
Created attachment 599959 [details] [diff] [review]
patch
Comment 17 Jonathan Kew (:jfkthame) 2012-02-23 05:56:27 PST
Comment on attachment 599959 [details] [diff] [review]
patch

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

A couple of drive-by comments, if you'll excuse me poking my nose in here....

::: accessible/src/base/nsTextAttrs.cpp
@@ +489,5 @@
> +  nsRefPtr<nsFontMetrics> fm;
> +  nsLayoutUtils::GetFontMetricsForFrame(aFrame, getter_AddRefs(fm));
> +
> +  gfxFontGroup* fontGroup = fm->GetThebesFontGroup();
> +  gfxFont* font = fontGroup->GetFontAt(0);

I don't think this is necessarily what you want. GetFontAt(0) will give you the resolved face for the first item in the font-family list, but that may well _not_ be what the user is actually seeing, depending whether that font actually supports the characters present in the text. It's not all that helpful to tell the user that the text is formatted in Times New Roman if it's actually Japanese that we're rendering with Hiragino Mincho Pro.

::: accessible/tests/mochitest/attributes.js
@@ +220,5 @@
> +const kSerifFontFamily = WIN ? "Times New Roman" :
> +  (LINUX ? "DejaVu Serif" : "MacFont");
> +
> +const kCursiveFontFamily = WIN ? "Comic Sans MS" :
> +  (LINUX ? "DejaVu Serif" : "MacFont");

The hardcoded font names here look like a recipe for test-fragility/non-portability.

::: accessible/tests/mochitest/attributes/test_text.html
@@ +541,5 @@
> +
> +  <p id="area16" style="font-family: sans-serif;">
> +    <span style="font-family: monospace;">text</span>text
> +    <span style="font-family: serif;">text</span>text
> +    <span style="font-family: Bodoni;">text</span>text

Some people are likely to have a font called Bodoni installed; if you want to test for an "absent" font, I'd guess it's better to make up a really unlikely-sounding name such as NotAnAvailableFontFamily.
Comment 18 Karl Tomlinson (:karlt) 2012-02-23 15:09:19 PST
Comment on attachment 599959 [details] [diff] [review]
patch

I'm agreeing with Jonathan's comments.

(In reply to Jonathan Kew (:jfkthame) from comment #17)
> > +  nsLayoutUtils::GetFontMetricsForFrame(aFrame, getter_AddRefs(fm));
> > +
> > +  gfxFontGroup* fontGroup = fm->GetThebesFontGroup();
> > +  gfxFont* font = fontGroup->GetFontAt(0);
> 
> I don't think this is necessarily what you want.

These options are available:

* CSS indicates the authors intent.
* The code above indicates the first font requested by the authors that is found on the system.
* nsRange::GetUsedFontFaces() indicates which font is used for the characters (and perhaps the layout strut height?)

> The hardcoded font names here look like a recipe for
> test-fragility/non-portability.

Using @font-face in the test with some simple fonts, perhaps copied from layout/reftests/fonts would provide a means to expect some exact matches.

For other situations, there are other things that can be tested. e.g.
result is not blank, result for sans-serif is not "sans-serif", result for sans-serif differs from result for serif, result for italic is the same as for upright.
Comment 19 alexander :surkov 2012-02-23 21:49:46 PST
(In reply to Jonathan Kew (:jfkthame) from comment #17)

> A couple of drive-by comments, if you'll excuse me poking my nose in here....

please do it, extra nose is always welcome :)

> ::: accessible/src/base/nsTextAttrs.cpp
> @@ +489,5 @@
> > +  nsRefPtr<nsFontMetrics> fm;
> > +  nsLayoutUtils::GetFontMetricsForFrame(aFrame, getter_AddRefs(fm));
> > +
> > +  gfxFontGroup* fontGroup = fm->GetThebesFontGroup();
> > +  gfxFont* font = fontGroup->GetFontAt(0);
> 
> I don't think this is necessarily what you want. GetFontAt(0) will give you
> the resolved face for the first item in the font-family list, but that may
> well _not_ be what the user is actually seeing, depending whether that font
> actually supports the characters present in the text. It's not all that
> helpful to tell the user that the text is formatted in Times New Roman if
> it's actually Japanese that we're rendering with Hiragino Mincho Pro.

that's true but it's certainly improvement over what we have now and it works possibly in 90% of cases. 

The problem is iteration step is an element node thus to support this we'd need to change that logic. Also I'm not sure about perf hit of doing this. I'd go with follow up instead.

> ::: accessible/tests/mochitest/attributes.js
> @@ +220,5 @@
> > +const kSerifFontFamily = WIN ? "Times New Roman" :
> > +  (LINUX ? "DejaVu Serif" : "MacFont");
> > +
> > +const kCursiveFontFamily = WIN ? "Comic Sans MS" :
> > +  (LINUX ? "DejaVu Serif" : "MacFont");
> 
> The hardcoded font names here look like a recipe for
> test-fragility/non-portability.

Right that's bad but it seems working (for example we do the same for font size checks which is platform/distributive dependent). I could make it more friednly for porting by using functions instead of hardcoded constants like

var kCursiveFontFamily = function(val)
{
  if (WIN)
   return val == "Comic Sans MS";
  return val == "something else";
}

Do I have other option?

> ::: accessible/tests/mochitest/attributes/test_text.html
> @@ +541,5 @@
> > +
> > +  <p id="area16" style="font-family: sans-serif;">
> > +    <span style="font-family: monospace;">text</span>text
> > +    <span style="font-family: serif;">text</span>text
> > +    <span style="font-family: Bodoni;">text</span>text
> 
> Some people are likely to have a font called Bodoni installed; if you want
> to test for an "absent" font, I'd guess it's better to make up a really
> unlikely-sounding name such as NotAnAvailableFontFamily.

good point, I'll fix it (after couple years of my original patch I didn't have any idea about Bodoni, I just noticed it doesn't exist on try server platforms).
Comment 20 alexander :surkov 2012-02-23 21:54:36 PST
(In reply to Karl Tomlinson (:karlt) from comment #18)

> These options are available:
> 
> * CSS indicates the authors intent.
> * The code above indicates the first font requested by the authors that is
> found on the system.
> * nsRange::GetUsedFontFaces() indicates which font is used for the
> characters (and perhaps the layout strut height?)

eventually we should end up with 3d option

> > The hardcoded font names here look like a recipe for
> > test-fragility/non-portability.
> 
> Using @font-face in the test with some simple fonts, perhaps copied from
> layout/reftests/fonts would provide a means to expect some exact matches.
> 
> For other situations, there are other things that can be tested. e.g.
> result is not blank, result for sans-serif is not "sans-serif", result for
> sans-serif differs from result for serif, result for italic is the same as
> for upright.

this sounds like a good idea
Comment 21 Trevor Saunders (:tbsaunde) 2012-02-25 16:11:46 PST
Comment on attachment 599959 [details] [diff] [review]
patch

>   nsTArray<nsITextAttr*> textAttrArray(10);

btw why can't we just use a stack allocated C array here? since we know how many things we'll need at build time.

>+FontFamilyTextAttr::GetValueFor(nsIContent* aElm, nsAutoString* aValue)
>+{
>+  nsIFrame* frame = aElm->GetPrimaryFrame();
>+  if (!frame)
>+    return false;

can it be null?

>+
>+  return GetFontFamily(frame, *aValue);
>+}
>+
>+void
>+FontFamilyTextAttr::Format(const nsAutoString& aValue,
>+                           nsAString& aFormattedValue)
>+{
>+  aFormattedValue = aValue;

since nsAutoString is only different in that it has a buffer in the object and this object will be heap allocated I don't think  passing nsAutoString as a template arg helps you over nsString, and it  may hurt you here by forcing an actual copy of the string instead of transfer of the buffer between strings.

>+  gfxFontGroup* fontGroup = fm->GetThebesFontGroup();
>+  gfxFont* font = fontGroup->GetFontAt(0);
>+  gfxFontEntry* fontEntry = font->GetFontEntry();
>+  aFamily = fontEntry->FamilyName();

some reason for all these local vars though I don't think it really hurts anything significant

>+class FontFamilyTextAttr : public nsTextAttr<nsAutoString>

any reason the class declaration can't just be in nsTextAttrs.cpp?

>+  /**
>+   * Return font family for the given frame.
>+   *
>+   * @param aFrame      [in] the given frame to query font weight
>+   * @return            font weight

copy paste issue?

>+const kSerifFontFamily = WIN ? "Times New Roman" :
>+  (LINUX ? "DejaVu Serif" : "MacFont");
>+
>+const kCursiveFontFamily = WIN ? "Comic Sans MS" :

is mac font a real thing or just temp until we run tests there?

>+function fontFamily(aComputedStyle)
>+{
>+  var name = aComputedStyle.fontFamily;
>+  if (name == "monospace")
>+    return kMonospaceFontFamily;
>+  if (name == "sans-serif")
>+    return kSansSerifFontFamily;
>+  if (name == "serif")
>+    return kSerifFontFamily;

js doesn't have switch? :/
Comment 22 alexander :surkov 2012-02-27 06:51:02 PST
Created attachment 600898 [details] [diff] [review]
patch2
Comment 23 alexander :surkov 2012-02-27 21:32:49 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #21)
> Comment on attachment 599959 [details] [diff] [review]
> patch
> 
> >   nsTArray<nsITextAttr*> textAttrArray(10);
> 
> btw why can't we just use a stack allocated C array here? since we know how
> many things we'll need at build time.

I think we can but not sure why, shouldn't it be the same?

> >+FontFamilyTextAttr::GetValueFor(nsIContent* aElm, nsAutoString* aValue)
> >+{
> >+  nsIFrame* frame = aElm->GetPrimaryFrame();
> >+  if (!frame)
> >+    return false;
> 
> can it be null?

it shouldn't be. I think this is sort of crash protection.

> >+
> >+  return GetFontFamily(frame, *aValue);
> >+}
> >+
> >+void
> >+FontFamilyTextAttr::Format(const nsAutoString& aValue,
> >+                           nsAString& aFormattedValue)
> >+{
> >+  aFormattedValue = aValue;
> 
> since nsAutoString is only different in that it has a buffer in the object
> and this object will be heap allocated I don't think  passing nsAutoString
> as a template arg helps you over nsString, and it  may hurt you here by
> forcing an actual copy of the string instead of transfer of the buffer
> between strings.

That's right. I'm going to tweak this code in next work. I hope you're ok with that and I don't need to rebase my queue :)

> >+  gfxFontGroup* fontGroup = fm->GetThebesFontGroup();
> >+  gfxFont* font = fontGroup->GetFontAt(0);
> >+  gfxFontEntry* fontEntry = font->GetFontEntry();
> >+  aFamily = fontEntry->FamilyName();
> 
> some reason for all these local vars though I don't think it really hurts
> anything significant

probably it's a little bit easier to detect a crash cause

> 
> >+class FontFamilyTextAttr : public nsTextAttr<nsAutoString>
> 
> any reason the class declaration can't just be in nsTextAttrs.cpp?

didn't you were fine in bug 728907 to move all of them under nsTextAttrs class?

> >+  /**
> >+   * Return font family for the given frame.
> >+   *
> >+   * @param aFrame      [in] the given frame to query font weight
> >+   * @return            font weight
> 
> copy paste issue?

yes

> >+const kSerifFontFamily = WIN ? "Times New Roman" :
> >+  (LINUX ? "DejaVu Serif" : "MacFont");
> >+
> >+const kCursiveFontFamily = WIN ? "Comic Sans MS" :
> 
> is mac font a real thing or just temp until we run tests there?

reworked in last patch

> >+function fontFamily(aComputedStyle)
> >+{
> >+  var name = aComputedStyle.fontFamily;
> >+  if (name == "monospace")
> >+    return kMonospaceFontFamily;
> >+  if (name == "sans-serif")
> >+    return kSansSerifFontFamily;
> >+  if (name == "serif")
> >+    return kSerifFontFamily;
> 
> js doesn't have switch? :/

I'll change that
Comment 24 Trevor Saunders (:tbsaunde) 2012-02-27 21:42:18 PST
(In reply to alexander :surkov from comment #23)
> (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> > Comment on attachment 599959 [details] [diff] [review]
> > patch
> > 
> > >   nsTArray<nsITextAttr*> textAttrArray(10);
> > 
> > btw why can't we just use a stack allocated C array here? since we know how
> > many things we'll need at build time.
> 
> I think we can but not sure why, shouldn't it be the same?

well, nsTArray puts the buffer on the heap, but if you convert it to nsAutoTArray with the appropriate size I think its effectively the same.

> That's right. I'm going to tweak this code in next work. I hope you're ok
> with that and I don't need to rebase my queue :)

ok, fine by me.

> didn't you were fine in bug 728907 to move all of them under nsTextAttrs
> class?

sure, but I made this comment before that discussion.
Comment 25 alexander :surkov 2012-02-27 22:02:09 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #24)

> well, nsTArray puts the buffer on the heap, but if you convert it to
> nsAutoTArray with the appropriate size I think its effectively the same.

maybe a followup? to not tweak a queue

> > didn't you were fine in bug 728907 to move all of them under nsTextAttrs
> > class?
> 
> sure, but I made this comment before that discussion.

yep, I realized that after I commented
Comment 27 Matt Brubeck (:mbrubeck) 2012-02-29 11:22:48 PST
https://hg.mozilla.org/mozilla-central/rev/20db0f825b05
Comment 28 alexander :surkov 2012-03-02 03:25:34 PST
I filed bug 732366 for correct font-family text attribute implementation

Note You need to log in before you can comment on or make changes to this bug.